Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9311198
or point your web browser to
http://mondrian/9311198
to review the following code:
Change 9311198 by [EMAIL PROTECTED] on 2008/12/08 21:13:09 *pending*
Add an explicit "Files" argument to desktop.getDragData.
This lets us remove the now-superfluous "file" prefix to the
properties on the returned object (e.g. s/fileCount/count/
and s/fileTotalBytes/totalBytes/).
This CL also modifies getDragData(evt, "Files") to return
undefined, rather than zeroes and empty arrays, when dropping
non-File data, such as a URL, or text.
PRESUBMIT=passed
R=noel
[EMAIL PROTECTED]
DELTA=81 (28 added, 14 deleted, 39 changed)
OCL=9311198
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#66 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#25 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#11
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#5
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#5
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#8
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#7
edit
81 delta lines: 28 added, 14 deleted, 39 changed
Also consider running:
g4 lint -c 9311198
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 9311198 by [EMAIL PROTECTED] on 2008/12/08 21:13:09 *pending*
Add an explicit "Files" argument to desktop.getDragData.
This lets us remove the now-superfluous "file" prefix to the
properties on the returned object (e.g. s/fileCount/count/
and s/fileTotalBytes/totalBytes/).
This CL also modifies getDragData(evt, "Files") to return
undefined, rather than zeroes and empty arrays, when dropping
non-File data, such as a URL, or text.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#66 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#25 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#11
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#5
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#5
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#8
edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#7
edit
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#66 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/desktop.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2008-12-08
21:08:50.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2008-12-08
21:05:44.000000000 +1100
@@ -1076,32 +1076,47 @@
}
scoped_ptr<JsObject> event_as_js_object;
+ std::string16 flavor_as_string;
JsArgument argv[] = {
{ JSPARAM_REQUIRED, JSPARAM_OBJECT, &event_as_js_object },
+ { JSPARAM_REQUIRED, JSPARAM_STRING16, &flavor_as_string },
};
context->GetArguments(ARRAYSIZE(argv), argv);
if (context->is_exception_set()) return;
+ // Currently, we only support one flavor: "Files". In the future, we may
+ // support others, such as "Text" or "URL".
+ DragAndDropFlavorType flavor = DRAG_AND_DROP_FLAVOR_INVALID;
+ if (flavor_as_string == STRING16(L"Files")) {
+ flavor = DRAG_AND_DROP_FLAVOR_FILES;
+ }
+ if (flavor == DRAG_AND_DROP_FLAVOR_INVALID) {
+ context->SetException(STRING16(L"Unsupported flavor type."));
+ return;
+ }
+
std::string16 error;
scoped_ptr<JsObject> result(
module_environment_->js_runner_->NewObject());
#if BROWSER_FF || (BROWSER_IE && !defined(OS_WINCE)) || BROWSER_WEBKIT
- ::GetDragData(module_environment_.get(),
- event_as_js_object.get(),
- result.get(),
- &error);
+ bool data_available = ::GetDragData(module_environment_.get(),
+ event_as_js_object.get(),
+ result.get(),
+ &error);
#else
// TODO(nigeltao): implement on Chromium.
+ bool data_available = false;
error = STRING16(L"getDragData is not supported for this platform.");
#endif
- if (!error.empty()) {
+ if (data_available) {
+ context->SetReturnValue(JSPARAM_OBJECT, result.get());
+ } else if (error.empty()) {
+ context->SetReturnValue(JSPARAM_UNDEFINED, NULL);
+ } else {
context->SetException(error);
- return;
- }
-
- context->SetReturnValue(JSPARAM_OBJECT, result.get());
+ }
}
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.h#25 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/desktop.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.h 2008-12-08
21:08:51.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.h 2008-12-08
20:49:13.000000000 +1100
@@ -213,7 +213,7 @@
// OUT: -
void AcceptDrag(JsCallContext *context);
- // IN: Event event
+ // IN: Event event, string flavor
// OUT: object data
void GetDragData(JsCallContext *context);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#2
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2008-12-08 21:08:51.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2008-12-08 20:56:14.000000000 +1100
@@ -95,22 +95,20 @@
// TODO(nigeltao): Error checking. We should return empty (or 0) instead of
// partial results, in case of failure.
- object_out->SetPropertyInt(STRING16(L"fileCount"), filenames_.size());
+ object_out->SetPropertyInt(STRING16(L"count"), filenames_.size());
object_out->SetPropertyDouble(
- STRING16(L"fileTotalBytes"),
+ STRING16(L"totalBytes"),
static_cast<double>(total_bytes_));
scoped_ptr<JsArray> extensions_array(
module_environment->js_runner_->NewArray());
AppendStringsToJsArray(extensions_, extensions_array.get());
- object_out->SetPropertyArray(STRING16(L"fileExtensions"),
- extensions_array.get());
+ object_out->SetPropertyArray(STRING16(L"extensions"),
extensions_array.get());
scoped_ptr<JsArray> mime_types_array(
module_environment->js_runner_->NewArray());
AppendStringsToJsArray(mime_types_, mime_types_array.get());
- object_out->SetPropertyArray(STRING16(L"fileMimeTypes"),
- mime_types_array.get());
+ object_out->SetPropertyArray(STRING16(L"mimeTypes"), mime_types_array.get());
if (is_in_a_drop) {
scoped_ptr<JsArray> file_array(
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#3
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2008-12-08 21:08:51.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2008-12-08 21:10:00.000000000 +1100
@@ -38,6 +38,13 @@
DRAG_AND_DROP_EVENT_INVALID
};
+enum DragAndDropFlavorType {
+ DRAG_AND_DROP_FLAVOR_FILES,
+ DRAG_AND_DROP_FLAVOR_TEXT,
+ DRAG_AND_DROP_FLAVOR_URL,
+ DRAG_AND_DROP_FLAVOR_INVALID
+};
+
class FileDragAndDropMetaData {
public:
FileDragAndDropMetaData();
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#11
-
/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-08 21:08:51.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-08 21:06:55.000000000 +1100
@@ -41,7 +41,6 @@
#include "gears/base/common/file.h"
#include "gears/base/common/mime_detect.h"
#include "gears/base/firefox/ns_file_utils.h"
-#include "gears/desktop/drag_and_drop_utils_common.h"
#include "gears/desktop/file_dialog.h"
@@ -299,7 +298,7 @@
}
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event_as_js_object,
JsObject *data_out,
std::string16 *error_out) {
@@ -308,7 +307,7 @@
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;
+ return false;
}
// TODO(nigeltao): Should we early exit for DRAG_AND_DROP_EVENT_DRAGLEAVE
@@ -320,14 +319,14 @@
do_GetService("@mozilla.org/widget/dragservice;1");
if (!drag_service) {
*error_out = GET_INTERNAL_ERROR_MESSAGE();
- return;
+ return false;
}
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;
+ return false;
}
// TODO(nigeltao): cache the filenames (and their MIME types, extensions,
@@ -336,13 +335,11 @@
// drag session ends and a new one begins, which might not be trivial if
// you get the same nsIDragSession pointer (since it's just the singleton
// DragService that's been QueryInterface'd) for two separate sessions.
- if (!AddFileDragAndDropData(module_environment,
- drag_session.get(),
- type == DRAG_AND_DROP_EVENT_DROP,
- data_out,
- error_out)) {
- assert(!error_out->empty());
- }
+ return AddFileDragAndDropData(module_environment,
+ drag_session.get(),
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out);
}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#5
-
/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-08 21:08:51.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-08 21:00:47.000000000 +1100
@@ -30,6 +30,7 @@
#include <vector>
#include "gears/base/common/base_class.h"
+#include "gears/desktop/drag_and_drop_utils_common.h"
class nsIDragSession;
@@ -44,7 +45,7 @@
bool acceptance,
std::string16 *error_out);
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#5
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2008-12-08 21:08:52.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2008-12-08 21:06:35.000000000 +1100
@@ -37,7 +37,6 @@
#include "gears/desktop/drag_and_drop_utils_ie.h"
#include "gears/base/ie/activex_utils.h"
-#include "gears/desktop/drag_and_drop_utils_common.h"
#include "gears/desktop/file_dialog.h"
@@ -209,7 +208,7 @@
}
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event_as_js_object,
JsObject *data_out,
std::string16 *error_out) {
@@ -217,15 +216,13 @@
DragAndDropEventType type = GetWindowEvent(module_environment, window_event);
if (type == DRAG_AND_DROP_EVENT_INVALID) {
*error_out = STRING16(L"The drag-and-drop event is invalid.");
- return;
- }
-
- if (!AddFileDragAndDropData(module_environment,
- type == DRAG_AND_DROP_EVENT_DROP,
- data_out,
- error_out)) {
- assert(!error_out->empty());
- }
+ return false;
+ }
+
+ return AddFileDragAndDropData(module_environment,
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out);
}
#endif // GEARS_DRAG_AND_DROP_API_IS_SUPPORTED_FOR_THIS_PLATFORM
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#4
-
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2008-12-08 21:08:52.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2008-12-08 21:00:46.000000000 +1100
@@ -27,6 +27,7 @@
#define GEARS_DESKTOP_DRAG_AND_DROP_UTILS_IE_H__
#include "gears/base/common/base_class.h"
+#include "gears/desktop/drag_and_drop_utils_common.h"
HRESULT GetHtmlDataTransfer(
IHTMLWindow2 *html_window_2,
@@ -43,7 +44,7 @@
bool acceptance,
std::string16 *error_out);
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#6
-
/home/nigeltao/srcgears2/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-12-08 21:08:52.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h
2008-12-08 21:00:45.000000000 +1100
@@ -27,6 +27,7 @@
#define GEARS_DESKTOP_DRAG_AND_DROP_UTILS_SF_H__
#include "gears/base/common/base_class.h"
+#include "gears/desktop/drag_and_drop_utils_common.h"
// Gears Drag and Drop needs to get access to more than what regular JavaScript
// driven drag and drop can see (and hence, what JavaScript via the NPAPI
@@ -63,7 +64,7 @@
bool acceptance,
std::string16 *error_out);
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#8
-
/home/nigeltao/srcgears2/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-12-08 21:08:52.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2008-12-08 21:06:39.000000000 +1100
@@ -32,7 +32,6 @@
#import "gears/base/common/common.h"
#import "gears/base/safari/nsstring_utils.h"
-#import "gears/desktop/drag_and_drop_utils_common.h"
#import "gears/desktop/file_dialog.h"
static FileDragAndDropMetaData *g_file_drag_and_drop_meta_data = NULL;
@@ -233,7 +232,7 @@
g_drag_operation = acceptance ? NSDragOperationCopy : NSDragOperationNone;
}
-void GetDragData(ModuleEnvironment *module_environment,
+bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event_as_js_object,
JsObject *data_out,
std::string16 *error_out) {
@@ -245,9 +244,7 @@
// 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 (!AddFileDragAndDropData(module_environment, data_out, error_out)) {
- assert(!error_out->empty());
- }
-}
+ return false;
+ }
+ return AddFileDragAndDropData(module_environment, data_out, error_out);
+}
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html#7
-
/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-08 21:08:52.000000000 +1100
+++
googleclient/gears/opensource/gears/test/manual/drag_and_drop_fake_event_attack.html
2008-12-08 20:55:23.000000000 +1100
@@ -1,5 +1,5 @@
<html><head>
-<title>Gears Drag and Drop, getDragData(fakeEvent)</title></head>
+<title>Gears Drag and Drop, getDragData(fakeEvent, 'Files')</title></head>
<body>
<ol>
<li>Drag some files from the desktop.</li>
@@ -51,7 +51,7 @@
invalidEvent = evt;
if (isDrop) {
try {
- var data = desktop.getDragData(evt);
+ var data = desktop.getDragData(evt, 'Files');
// 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) {
@@ -106,7 +106,7 @@
function tryGetDragData(evt) {
document.getElementById('eventTypeOutput').innerHTML = evt.type;
try {
- desktop.getDragData(evt);
+ desktop.getDragData(evt, 'Files');
// The line above should always throw an exception.
alert('Gears drag and drop has a potential security hole.');
} catch (ex) {
