Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9167425
or point your web browser to
http://mondrian/9167425
to review the following code:
Change 9167425 by [EMAIL PROTECTED] on 2008/11/26 20:35:54 *pending*
Implement desktop.getDragAndDropData(event) on Safari.
PRESUBMIT=passed
R=noel
[EMAIL PROTECTED]
DELTA=103 (87 added, 3 deleted, 13 changed)
OCL=9167425
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#58 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#1
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#1
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#1
edit
103 delta lines: 87 added, 3 deleted, 13 changed
Also consider running:
g4 lint -c 9167425
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 9167425 by [EMAIL PROTECTED] on 2008/11/26 20:35:54 *pending*
Implement desktop.getDragAndDropData(event) on Safari.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#58 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#1
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#1
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#1
edit
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#58 -
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/desktop/desktop.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2008-11-26
20:33:18.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2008-11-26
17:27:05.000000000 +1100
@@ -57,6 +57,8 @@
#if BROWSER_FF
#include "gears/desktop/drag_and_drop_utils_ff.h"
+#elif BROWSER_SF
+#include "gears/desktop/drag_and_drop_utils_sf.h"
#endif
DECLARE_DISPATCHER(GearsDesktop);
@@ -1055,8 +1057,13 @@
result.get(),
&error);
#endif
-#else
- // TODO(nigeltao): implement on IE, Safari and Chromium.
+#elif BROWSER_SF
+ ::GetDragAndDropData(module_environment_.get(),
+ event_as_js_object.get(),
+ result.get(),
+ &error);
+#else
+ // TODO(nigeltao): implement on IE and Chromium.
error = STRING16(L"getDragAndDropData is not supported for this platform.");
#endif
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#1
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h
2008-11-26 20:33:18.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h
2008-11-26 17:28:37.000000000 +1100
@@ -58,4 +58,9 @@
bool IsInADragOperation();
bool IsInADropOperation();
+void GetDragAndDropData(ModuleEnvironment *module_environment,
+ JsObject *event,
+ JsObject *data_out,
+ std::string16 *error_out);
+
#endif // GEARS_DESKTOP_DRAG_AND_DROP_UTILS_SF_H__
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#1
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2008-11-26 20:33:18.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2008-11-26 20:38:29.000000000 +1100
@@ -172,3 +172,32 @@
}
return result;
};
+
+void GetDragAndDropData(ModuleEnvironment *module_environment,
+ JsObject *event_as_js_object,
+ JsObject *data_out,
+ std::string16 *error_out) {
+ // We ignore event_as_js_object, since Gears (being restricted to the NPAPI
+ // interface) cannot distinguish between a drag and drop event genuinely
+ // inside dispatch, and an event that has had dispatchEvent called on,
+ // outside of a user drag and drop action.
+ // Instead, we rely on our IsInAXxxxOperation tests to tell whether or not
+ // we are called during a user drag and drop action.
+ if (!IsInADragOperation() && !IsInADropOperation()) {
+ *error_out = STRING16(L"The drag-and-drop event is invalid.");
+ return;
+ }
+
+ if (IsInADropOperation()) {
+ scoped_ptr<JsArray> file_array(
+ module_environment->js_runner_->NewArray());
+ if (!GetDroppedFiles(module_environment, file_array.get(), error_out)) {
+ return;
+ }
+ data_out->SetPropertyArray(STRING16(L"files"), file_array.get());
+ }
+
+ // TODO(nigeltao): For all DnD events (including the non-drop events
+ // dragenter and dragover), we still should provide (1) MIME types,
+ // (2) file extensions, (3) total file size, and (4) file count.
+}
\ No newline at end of file
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#1
-
/Users/nigeltao/devel/srcmacgears1/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-11-26 20:33:18.000000000 +1100
+++
googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html
2008-11-26 20:32:43.000000000 +1100
@@ -21,6 +21,13 @@
<script type="text/javascript" src="../../sdk/gears_init.js"></script>
<script type="text/javascript">
+var isIE = google.gears.factory.getBuildInfo().indexOf(';ie') > -1;
+var isFirefox = google.gears.factory.getBuildInfo().indexOf(';firefox') > -1;
+var isSafari = google.gears.factory.getBuildInfo().indexOf(';safari') > -1;
+var isNPAPI = google.gears.factory.getBuildInfo().indexOf(';npapi') > -1;
+
+var isInManualDispatch = false;
+
var desktop = google.gears.factory.create('beta.desktop');
// We initialize this global variable to a fake event, to try and spoof the
@@ -35,9 +42,6 @@
// first genuine drag and drop event), invalidEvent is set to that event and
// we tryGetDragAndDropData that event, every 2 seconds.
var invalidEvent = fakeEvent;
-// TODO(nigeltao): check that, on IE, Safari and Chromium, calling
-// window.event = fakeEvent;
-// has no effect.
function handler(isDrop) {
return function(evt) {
@@ -46,7 +50,22 @@
// dispatched.
invalidEvent = evt;
if (isDrop) {
- var data = desktop.getDragAndDropData(evt);
+ try {
+ var data = desktop.getDragAndDropData(evt);
+ // The line above should always throw an exception, if we are in
+ // manual dispatch (as opposed to a genuine drag and drop event).
+ if (isInManualDispatch) {
+ alert('Gears drag and drop has a potential security hole.');
+ }
+ } catch (ex) {
+ if (!isInManualDispatch) {
+ alert('desktop.getDragAndDropData failed during genuine dispatch.');
+ } else {
+ // Getting an exception during manual dispatch is exactly what we
+ // expect.
+ return;
+ }
+ }
var s = '';
if (data.files) {
for (i = 0; i < data.files.length; i++) {
@@ -56,17 +75,33 @@
}
}
document.getElementById('dropOutput').innerHTML = s;
- evt.stopPropagation();
+ if (isFirefox) {
+ evt.stopPropagation();
+ }
+ }
+ if (!isFirefox) {
+ evt.preventDefault();
}
};
}
-// TODO(nigeltao): addEventListener is Firefox-only. I should extend this
-// (manual) test case to also test IE, Safari and Chromium.
-document.addEventListener('dragenter', handler(false), false);
-document.addEventListener('dragover', handler(false), false);
-document.addEventListener('dragleave', handler(false), false);
-document.addEventListener('dragdrop', handler(true), false);
+// For a discussion of various browser's event models, see
+// http://developer.apple.com/internet/webcontent/eventmodels.html
+if (isFirefox) {
+ // Firefox uses different event names than IE, WebKit, and HTML5.
+ // Specifically, dragexit instead of dragleave, and dragdrop for drop.
+ document.addEventListener('dragenter', handler(false), false);
+ document.addEventListener('dragover', handler(false), false);
+ document.addEventListener('dragexit', handler(false), false);
+ document.addEventListener('dragdrop', handler(true), false);
+} else if (isSafari) {
+ document.addEventListener('dragenter', handler(false), false);
+ document.addEventListener('dragover', handler(false), false);
+ document.addEventListener('dragleave', handler(false), false);
+ document.addEventListener('drop', handler(true), false);
+} else {
+ // TODO(nigeltao): implement (and test) on IE and Chromium.
+}
function tryGetDragAndDropData(evt) {
document.getElementById('eventTypeOutput').innerHTML = evt.type;
@@ -76,7 +111,7 @@
alert('Gears drag and drop has a potential security hole.');
} catch (ex) {
if (window.console) {
- window.console.debug(ex.message);
+ window.console.info(ex.message);
}
}
}
@@ -87,12 +122,20 @@
evt.initMouseEvent('mouseover', true, true, window,
0, 0, 0, 0, 0, false, false, false, false, 0, null);
}
- document.dispatchEvent(evt);
+ isInManualDispatch = true;
+ try {
+ document.dispatchEvent(evt);
+ } catch (ex) {
+ // Silently ignore exceptions.
+ }
+ isInManualDispatch = false;
}
// This event listener will catch both natural mouseover events, and those
// mouseover events dispatched by the tryDispatchEvent function above.
-document.addEventListener('mouseover', tryGetDragAndDropData, false);
+if (isFirefox || isSafari) {
+ document.addEventListener('mouseover', tryGetDragAndDropData, false);
+}
window.setInterval(function() { tryGetDragAndDropData(invalidEvent); }, 2000);
