Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9477904
or point your web browser to
http://mondrian/9477904
to review the following code:
Change 9477904 by nigel...@nigeltao-srcgears2 on 2008/12/19 19:04:57 *pending*
Clean up drop_target_ff code.
PRESUBMIT=passed
R=noel
[email protected]
DELTA=203 (97 added, 86 deleted, 20 changed)
OCL=9477904
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#20
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#9 edit
203 delta lines: 97 added, 86 deleted, 20 changed
Also consider running:
g4 lint -c 9477904
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 9477904 by nigel...@nigeltao-srcgears2 on 2008/12/19 19:04:57 *pending*
Clean up drop_target_ff code.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#20
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#9 edit
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#20 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-19 18:45:40.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-19 19:02:02.000000000 +1100
@@ -108,22 +108,14 @@
drop_target->html_element_ = dom_element.dom_html_element();
drop_target->AddRef(); // Balanced by a Release() call during
UnregisterSelf.
- if (drop_target->on_drag_enter_.get()) {
- event_target->AddEventListener(kDragEnterAsString,
- drop_target.get(), false);
- }
- if (drop_target->on_drag_over_.get()) {
- event_target->AddEventListener(kDragOverAsString,
- drop_target.get(), false);
- }
- if (drop_target->on_drag_leave_.get()) {
- event_target->AddEventListener(kDragExitAsString,
- drop_target.get(), false);
- }
- if (drop_target->on_drop_.get()) {
- event_target->AddEventListener(kDragDropAsString,
- drop_target.get(), false);
- }
+ event_target->AddEventListener(kDragEnterAsString,
+ drop_target.get(), false);
+ event_target->AddEventListener(kDragOverAsString,
+ drop_target.get(), false);
+ event_target->AddEventListener(kDragExitAsString,
+ drop_target.get(), false);
+ event_target->AddEventListener(kDragDropAsString,
+ drop_target.get(), false);
drop_target->ProvideDebugVisualFeedback(false);
return drop_target.get();
@@ -137,18 +129,10 @@
unregister_self_has_been_called_ = true;
if (event_target_) {
- if (on_drag_enter_.get()) {
- event_target_->RemoveEventListener(kDragEnterAsString, this, false);
- }
- if (on_drag_over_.get()) {
- event_target_->RemoveEventListener(kDragOverAsString, this, false);
- }
- if (on_drag_leave_.get()) {
- event_target_->RemoveEventListener(kDragExitAsString, this, false);
- }
- if (on_drop_.get()) {
- event_target_->RemoveEventListener(kDragDropAsString, this, false);
- }
+ event_target_->RemoveEventListener(kDragEnterAsString, this, false);
+ event_target_->RemoveEventListener(kDragOverAsString, this, false);
+ event_target_->RemoveEventListener(kDragExitAsString, this, false);
+ event_target_->RemoveEventListener(kDragDropAsString, this, false);
}
Release(); // Balanced by an AddRef() call during CreateDropTarget.
@@ -212,17 +196,65 @@
}
+bool DropTarget::ExecJsCallback(DragAndDropEventType type,
+ nsIDragSession *drag_session,
+ nsIDOMEvent *event) {
+ JsRootedCallback *callback = NULL;
+ switch (type) {
+ case DRAG_AND_DROP_EVENT_DRAGENTER:
+ callback = on_drag_enter_.get();
+ break;
+ case DRAG_AND_DROP_EVENT_DRAGOVER:
+ callback = on_drag_over_.get();
+ break;
+ case DRAG_AND_DROP_EVENT_DRAGLEAVE:
+ callback = on_drag_leave_.get();
+ break;
+ case DRAG_AND_DROP_EVENT_DROP:
+ callback = on_drop_.get();
+ break;
+ default:
+ assert(false);
+ break;
+ }
+ if (!callback) {
+ return false;
+ }
+
+ std::string16 ignored;
+ scoped_ptr<JsObject> context_object(
+ module_environment_->js_runner_->NewObject());
+ if (will_accept_drop_ &&
+ type != DRAG_AND_DROP_EVENT_DRAGLEAVE &&
+ !AddFileDragAndDropData(module_environment_.get(),
+ drag_session,
+ type,
+ context_object.get(),
+ &ignored)) {
+ context_object.reset(module_environment_->js_runner_->NewObject());
+ }
+ AddEventToJsObject(context_object.get(), event);
+
+ scoped_ptr<JsRootedToken> return_value;
+ const int argc = 1;
+ JsParamToSend argv[argc] = {
+ { JSPARAM_OBJECT, context_object.get() }
+ };
+ module_environment_->js_runner_->InvokeCallback(
+ callback, NULL, argc, argv, as_out_parameter(return_value));
+
+ // The HTML5 specification (section 5.4.5) says that an event handler
+ // returning *false* means that we should not perform the default
+ // action (i.e. the web-app wants Gears' file drop behavior, and not
+ // the default browser behavior of navigating away from the current
+ // page to the file being dropped).
+ return return_value.get() &&
+ JSVAL_IS_BOOLEAN(return_value->token()) &&
+ JSVAL_TO_BOOLEAN(return_value->token()) == false;
+}
+
+
NS_IMETHODIMP DropTarget::HandleEvent(nsIDOMEvent *event) {
- nsCOMPtr<nsIDragService> drag_service =
- do_GetService("@mozilla.org/widget/dragservice;1");
- if (!drag_service) { return NS_ERROR_FAILURE; }
- // TODO(nigeltao): Do we have to do the Firefox2/Linux workaround of
- // explicitly calling drag_service->StartDragSession(), just like we do in
- // drag_and_drop_utils_ff.cc's GetDragAndDropEventType.
- nsCOMPtr<nsIDragSession> drag_session;
- nsresult nr = drag_service->GetCurrentSession(getter_AddRefs(drag_session));
- if (NS_FAILED(nr) || !drag_session.get()) { return NS_ERROR_FAILURE; }
-
nsString event_type;
event->GetType(event_type);
DragAndDropEventType type = DRAG_AND_DROP_EVENT_INVALID;
@@ -230,85 +262,60 @@
type = DRAG_AND_DROP_EVENT_DRAGOVER;
} else if (event_type.Equals(kDragEnterAsString)) {
type = DRAG_AND_DROP_EVENT_DRAGENTER;
+ ProvideDebugVisualFeedback(true);
} else if (event_type.Equals(kDragExitAsString)) {
type = DRAG_AND_DROP_EVENT_DRAGLEAVE;
+ ProvideDebugVisualFeedback(false);
} else if (event_type.Equals(kDragDropAsString)) {
type = DRAG_AND_DROP_EVENT_DROP;
+ ProvideDebugVisualFeedback(false);
+ } else {
+ assert(false);
+ return NS_ERROR_FAILURE;
}
-
- std::string16 ignored;
- scoped_ptr<JsObject> context_object(
- module_environment_->js_runner_->NewObject());
- AddEventToJsObject(context_object.get(), event);
- if (!AddFileDragAndDropData(module_environment_.get(),
- drag_session.get(),
- type,
- context_object.get(),
- &ignored)) {
- return NS_ERROR_FAILURE;
- }
-
- if (on_drop_.get() && (type == DRAG_AND_DROP_EVENT_DROP)) {
- ProvideDebugVisualFeedback(false);
+ nsCOMPtr<nsIDragService> drag_service =
+ do_GetService("@mozilla.org/widget/dragservice;1");
+ if (!drag_service) { return NS_ERROR_FAILURE; }
+#if BROWSER_FF2 && defined(LINUX) && !defined(OS_MACOSX)
+ if (type == DRAG_AND_DROP_EVENT_DRAGENTER) {
+ // On Firefox2/Linux, the DragSession is incorrectly started *after* the
+ // first dragenter event. We workaround this by explicitly starting the
+ // drag session. For more information, see the similar workaround in the
+ // drag_and_drop_utils_ff code.
+ drag_service->StartDragSession();
+ }
+#endif
+ nsCOMPtr<nsIDragSession> drag_session;
+ nsresult nr = drag_service->GetCurrentSession(getter_AddRefs(drag_session));
+ if (NS_FAILED(nr) || !drag_session.get()) { return NS_ERROR_FAILURE; }
+
+ bool callback_returned_false =
+ ExecJsCallback(type, drag_session.get(), event);
+
+ if (type == DRAG_AND_DROP_EVENT_DRAGENTER ||
+ type == DRAG_AND_DROP_EVENT_DRAGOVER) {
+ nr = drag_session->SetDragAction(will_accept_drop_
+ ? static_cast<int>(nsIDragService::DRAGDROP_ACTION_COPY)
+ : static_cast<int>(nsIDragService::DRAGDROP_ACTION_NONE));
+ if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
+ will_accept_drop_ = callback_returned_false;
+
+ } else if (type == DRAG_AND_DROP_EVENT_DRAGLEAVE) {
+ will_accept_drop_ = false;
+
+ } else if (type == DRAG_AND_DROP_EVENT_DROP) {
+ // Prevent the default browser behavior of navigating away from the
+ // current page to the file being dropped.
+ nr = event->StopPropagation();
+ if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
if (will_accept_drop_) {
- will_accept_drop_ = false;
- // Prevent the default browser behavior of navigating away from the
- // current page to the file being dropped.
- event->StopPropagation();
-
- const int argc = 1;
- JsParamToSend argv[argc] = {
- { JSPARAM_OBJECT, context_object.get() }
- };
- module_environment_->js_runner_->InvokeCallback(
- on_drop_.get(), NULL, argc, argv, NULL);
nr = drag_session->SetDragAction(nsIDragService::DRAGDROP_ACTION_COPY);
if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
}
-
- } else {
- bool is_drag_exit = false;
- JsRootedCallback *callback = NULL;
- if (on_drag_enter_.get() && (type == DRAG_AND_DROP_EVENT_DRAGENTER)) {
- ProvideDebugVisualFeedback(true);
- callback = on_drag_enter_.get();
- } else if (on_drag_over_.get() && (type == DRAG_AND_DROP_EVENT_DRAGOVER)) {
- callback = on_drag_over_.get();
- } else if (on_drag_leave_.get() &&
- (type == DRAG_AND_DROP_EVENT_DRAGLEAVE)) {
- ProvideDebugVisualFeedback(false);
- callback = on_drag_leave_.get();
- is_drag_exit = true;
- }
- if (callback) {
- const int argc = 1;
- JsParamToSend argv[argc] = {
- { JSPARAM_OBJECT, context_object.get() }
- };
- if (is_drag_exit) {
- module_environment_->js_runner_->InvokeCallback(
- callback, NULL, argc, argv, NULL);
- will_accept_drop_ = false;
- } else {
- scoped_ptr<JsRootedToken> return_value;
- module_environment_->js_runner_->InvokeCallback(
- callback, NULL, argc, argv, as_out_parameter(return_value));
- // The HTML5 specification (section 5.4.5) says that an event handler
- // returning *false* means that we should not perform the default
- // action (i.e. the web-app wants Gears' file drop behavior, and not
- // the default browser behavior of navigating away from the current
- // page to the file being dropped).
- will_accept_drop_ = return_value.get() &&
- JSVAL_IS_BOOLEAN(return_value->token()) &&
- JSVAL_TO_BOOLEAN(return_value->token()) == false;
- nr = drag_session->SetDragAction(will_accept_drop_
- ? static_cast<int>(nsIDragService::DRAGDROP_ACTION_COPY)
- : static_cast<int>(nsIDragService::DRAGDROP_ACTION_NONE));
- if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
- }
- }
- }
+ will_accept_drop_ = false;
+ }
+
return NS_OK;
}
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#9 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drop_target_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-12-19 18:45:40.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-12-19 18:46:50.000000000 +1100
@@ -33,6 +33,7 @@
#include <gecko_internal/nsIXPConnect.h>
#include <gecko_sdk/include/nsIDOMEventListener.h>
+#include "gears/desktop/drag_and_drop_utils_common.h"
#include "gears/desktop/drop_target_base.h"
class DropTarget
@@ -67,6 +68,9 @@
std::string16 *error_out);
void AddEventToJsObject(JsObject *js_object, nsIDOMEvent *event);
+ bool ExecJsCallback(DragAndDropEventType type,
+ nsIDragSession *drag_session,
+ nsIDOMEvent *event);
void ProvideDebugVisualFeedback(bool is_drag_enter);
DISALLOW_EVIL_CONSTRUCTORS(DropTarget);
