Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9207912
or point your web browser to
http://mondrian/9207912
to review the following code:
Change 9207912 by [EMAIL PROTECTED] on 2008/12/01 12:41:44 *pending*
Implement desktop.acceptDrag on Firefox.
PRESUBMIT=passed
R=noel
[EMAIL PROTECTED]
DELTA=79 (51 added, 9 deleted, 19 changed)
OCL=9207912
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#61 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#24 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#2
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#4
edit
79 delta lines: 51 added, 9 deleted, 19 changed
Also consider running:
g4 lint -c 9207912
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 9207912 by [EMAIL PROTECTED] on 2008/12/01 12:41:44 *pending*
Implement desktop.acceptDrag on Firefox.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#61 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#24 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#2
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#4
edit
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#61 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/desktop.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2008-12-01
12:34:19.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2008-12-01
12:10:39.000000000 +1100
@@ -86,6 +86,7 @@
// The Drag-and-Drop API has not been finalized for official builds.
#else
#if GEARS_DRAG_AND_DROP_API_IS_SUPPORTED_FOR_THIS_PLATFORM
+ // TODO(nigeltao): should acceptDrag be renamed finishDrag??
RegisterMethod("acceptDrag", &GearsDesktop::AcceptDrag);
RegisterMethod("getDragData", &GearsDesktop::GetDragData);
RegisterMethod("registerDropTarget", &GearsDesktop::RegisterDropTarget);
@@ -1039,14 +1040,21 @@
}
scoped_ptr<JsObject> event_as_js_object;
+ bool acceptance;
JsArgument argv[] = {
{ JSPARAM_REQUIRED, JSPARAM_OBJECT, &event_as_js_object },
+ { JSPARAM_REQUIRED, JSPARAM_BOOL, &acceptance },
};
context->GetArguments(ARRAYSIZE(argv), argv);
if (context->is_exception_set()) return;
std::string16 error;
-#if BROWSER_FF || (BROWSER_IE && !defined(OS_WINCE)) || BROWSER_WEBKIT
+#if BROWSER_FF
+ ::AcceptDrag(module_environment_.get(),
+ event_as_js_object.get(),
+ acceptance,
+ &error);
+#elif (BROWSER_IE && !defined(OS_WINCE)) || BROWSER_WEBKIT
::AcceptDrag(module_environment_.get(),
event_as_js_object.get(),
&error);
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.h#24 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/desktop.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.h 2008-12-01
12:34:19.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.h 2008-11-28
17:24:17.000000000 +1100
@@ -209,7 +209,7 @@
// * The "Web-app calls Gears" model (GetDragData followed by AcceptDrag), or
// * The "Gears calls Web-app" model (RegisterDropTarget).
- // IN: Event event
+ // IN: Event event, bool acceptance
// OUT: -
void AcceptDrag(JsCallContext *context);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#3
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-01 12:34:19.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-01 12:36:18.000000000 +1100
@@ -112,11 +112,22 @@
static const nsString kDragDropAsString(STRING16(L"dragdrop"));
-// Given a DOMEvent (as an nsISupports), return what type of event we are in,
+// Given a DOMEvent (as a JsObject), return what type of event we are in,
// also checking that we are in event dispatch (and not, for example, a
// situation where the JS keeps a reference to a previously issued event
// and passes that back to Gears outside of event dispatch).
-DragAndDropEventType GetDragAndDropEventType(nsISupports *event_as_supports) {
+DragAndDropEventType GetDragAndDropEventType(
+ ModuleEnvironment *module_environment,
+ JsObject *event_as_js_object,
+ nsCOMPtr<nsIDOMEvent> *dom_event_out) {
+ nsCOMPtr<nsISupports> event_as_supports;
+ if (!JsvalToISupports(
+ module_environment->js_runner_->GetContext(),
+ event_as_js_object->token(),
+ getter_AddRefs(event_as_supports))) {
+ return DRAG_AND_DROP_EVENT_INVALID;
+ }
+
nsCOMPtr<nsIPrivateDOMEvent> private_dom_event(
do_QueryInterface(event_as_supports));
if (!private_dom_event) { return DRAG_AND_DROP_EVENT_INVALID; }
@@ -186,9 +197,9 @@
return DRAG_AND_DROP_EVENT_INVALID;
}
- nsCOMPtr<nsIDOMEvent> dom_event(do_QueryInterface(event_as_supports));
+ *dom_event_out = do_QueryInterface(event_as_supports);
nsString event_type;
- nr = dom_event->GetType(event_type);
+ nr = (*dom_event_out)->GetType(event_type);
if (NS_FAILED(nr)) { return DRAG_AND_DROP_EVENT_INVALID; }
if (event_type.Equals(kDragOverAsString)) {
@@ -205,12 +216,43 @@
void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
+ JsObject *event_as_js_object,
+ bool acceptance,
std::string16 *error_out) {
- // TODO(nigeltao): port the following JavaScript to C++.
- // if (isDrop) {
- // evt.stopPropagation();
- // }
+ nsCOMPtr<nsIDOMEvent> dom_event;
+ DragAndDropEventType type = GetDragAndDropEventType(
+ module_environment, event_as_js_object, &dom_event);
+ if (type == DRAG_AND_DROP_EVENT_INVALID) {
+ *error_out = STRING16(L"The drag-and-drop event is invalid.");
+ return;
+ }
+
+ if (type == DRAG_AND_DROP_EVENT_DROP) {
+ dom_event->StopPropagation();
+
+ } else if (type == DRAG_AND_DROP_EVENT_DRAGOVER) {
+ nsCOMPtr<nsIDragService> drag_service =
+ do_GetService("@mozilla.org/widget/dragservice;1");
+ if (!drag_service) {
+ *error_out = GET_INTERNAL_ERROR_MESSAGE();
+ return;
+ }
+
+ nsCOMPtr<nsIDragSession> drag_session;
+ nsresult nr =
drag_service->GetCurrentSession(getter_AddRefs(drag_session));
+ if (NS_FAILED(nr) || !drag_session.get()) {
+ *error_out = GET_INTERNAL_ERROR_MESSAGE();
+ return;
+ }
+
+ nr = drag_session->SetDragAction(acceptance
+ ? static_cast<int>(nsIDragService::DRAGDROP_ACTION_COPY)
+ : static_cast<int>(nsIDragService::DRAGDROP_ACTION_NONE));
+ if (NS_FAILED(nr)) {
+ *error_out = GET_INTERNAL_ERROR_MESSAGE();
+ return;
+ }
+ }
}
@@ -218,16 +260,9 @@
JsObject *event_as_js_object,
JsObject *data_out,
std::string16 *error_out) {
- nsCOMPtr<nsISupports> event_as_supports;
- if (!JsvalToISupports(
- module_environment->js_runner_->GetContext(),
- event_as_js_object->token(),
- getter_AddRefs(event_as_supports))) {
- *error_out = STRING16(L"The drag-and-drop event is invalid.");
- return;
- }
-
- DragAndDropEventType type = GetDragAndDropEventType(event_as_supports.get());
+ nsCOMPtr<nsIDOMEvent> dom_event;
+ DragAndDropEventType type = GetDragAndDropEventType(
+ module_environment, event_as_js_object, &dom_event);
if (type == DRAG_AND_DROP_EVENT_INVALID) {
*error_out = STRING16(L"The drag-and-drop event is invalid.");
return;
@@ -236,14 +271,14 @@
nsCOMPtr<nsIDragService> drag_service =
do_GetService("@mozilla.org/widget/dragservice;1");
if (!drag_service) {
- *error_out = STRING16(L"Could not access the DragService.");
+ *error_out = GET_INTERNAL_ERROR_MESSAGE();
return;
}
nsCOMPtr<nsIDragSession> drag_session;
nsresult nr = drag_service->GetCurrentSession(getter_AddRefs(drag_session));
if (NS_FAILED(nr) || !drag_session.get()) {
- *error_out = STRING16(L"Could not access the DragSession.");
+ *error_out = GET_INTERNAL_ERROR_MESSAGE();
return;
}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#2
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-01 12:34:19.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-11-28 17:25:18.000000000 +1100
@@ -37,6 +37,7 @@
void AcceptDrag(ModuleEnvironment *module_environment,
JsObject *event,
+ bool acceptance,
std::string16 *error_out);
void GetDragData(ModuleEnvironment *module_environment,
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#4
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html
====
# action=edit type=text
---
googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html
2008-12-01 12:40:30.000000000 +1100
+++
googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html
2008-12-01 12:40:17.000000000 +1100
@@ -71,17 +71,15 @@
for (i = 0; i < data.files.length; i++) {
var file = data.files[i];
s += '<b>' + file.name + '</b> has length <b>' +
- file.blob.length + '</b>.<br/>';
+ file.blob.length + '</b>.<br>';
}
}
document.getElementById('dropOutput').innerHTML = s;
}
// TODO(nigeltao): replace all of these browser-specific quirks with
- // desktop.acceptDrag(evt);
+ // desktop.acceptDrag(evt, true);
if (isFirefox) {
- if (isDrop) {
- evt.stopPropagation();
- }
+ desktop.acceptDrag(evt, true);
} else if (isIE) {
if (isDragEnterOrDragOver) {
evt.returnValue = false;
