Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9309925
or point your web browser to
http://mondrian/9309925
to review the following code:
Change 9309925 by [EMAIL PROTECTED] on 2008/12/08 17:59:37 *pending*
Refactor drag_and_drop_utils_ff to use the FileDragAndDropMetaData
class from drag_and_drop_utils_common.
Also, add a comment that nsIDragSession->GetData may cause assertion
failures when called outside of a drop event, and hence note that
Gears will probably have to work around this, in order to provide
aggregate file drag-and-drop metadata during dragenter and dragover,
for at least Firefox2 / GTK / Linux.
PRESUBMIT=passed
R=noel
[EMAIL PROTECTED]
DELTA=150 (20 added, 84 deleted, 46 changed)
OCL=9309925
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#10
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#4
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#17
edit
150 delta lines: 20 added, 84 deleted, 46 changed
Also consider running:
g4 lint -c 9309925
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 9309925 by [EMAIL PROTECTED] on 2008/12/08 17:59:37 *pending*
Refactor drag_and_drop_utils_ff to use the FileDragAndDropMetaData
class from drag_and_drop_utils_common.
Also, add a comment that nsIDragSession->GetData may cause assertion
failures when called outside of a drop event, and hence note that
Gears will probably have to work around this, in order to provide
aggregate file drag-and-drop metadata during dragenter and dragover,
for at least Firefox2 / GTK / Linux.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#10
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#4
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#17
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#1
-
/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 15:47:47.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2008-12-08 10:48:36.000000000 +1100
@@ -68,7 +68,14 @@
for (std::vector<std::string16>::iterator i = filenames_.begin();
i != filenames_.end(); ++i) {
std::string16 &filename = *i;
+ // TODO(nigeltao): Should we also keep an array of per-file MIME types,
+ // not just the overall set of MIME types? If so, the mimeType should
+ // probably be a property of the file JavaScript object (i.e. the thing
+ // with a name and blob property), not a separate array to the files array.
mime_types_.insert(DetectMimeTypeOfFile(filename));
+ // TODO(nigeltao): Decide whether we should insert ".txt" or "txt" -
+ // that is, does the file extension include the dot at the start.
+ // We should do whatever desktop.openFiles does.
std::string16 extension(File::GetFileExtension(filename.c_str()));
if (!extension.empty()) {
extensions_.insert(extension);
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#10
-
/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 15:47:47.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-08 18:00:55.000000000 +1100
@@ -299,23 +299,6 @@
}
-static void JsObjectSetPropertyStringArray(
- ModuleEnvironment *module_environment,
- JsObject *data_out,
- const char16 *property_key,
- std::set<std::string16> &strings) {
- scoped_ptr<JsArray> array(module_environment->js_runner_->NewArray());
- int i = 0;
- for (std::set<std::string16>::iterator iter = strings.begin();
- iter != strings.end();
- ++iter) {
- // TODO(nigeltao): Should we skip over empty string values?
- array->SetElementString(i++, *iter);
- }
- data_out->SetPropertyArray(property_key, array.get());
-}
-
-
void GetDragData(ModuleEnvironment *module_environment,
JsObject *event_as_js_object,
JsObject *data_out,
@@ -353,59 +336,40 @@
// 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.
- std::vector<std::string16> filenames;
- std::set<std::string16> file_extensions;
- std::set<std::string16> file_mime_types;
- int64 file_total_bytes = 0;
- if (!GetDroppedFiles(module_environment,
- drag_session.get(),
- &filenames,
- &file_extensions,
- &file_mime_types,
- &file_total_bytes)) {
- // If GetDroppedFiles fails, then GetDroppedFiles may have added partial
- // results to the filenames vector, and other accumulators, which we
- // should clear out.
- // TODO(nigeltao): Should we throw an error here, or just happily return
- // empty? How should we behave if the user drags and drops Text or a URL?
- filenames.clear();
- file_extensions.clear();
- file_mime_types.clear();
- file_total_bytes = 0;
- }
-
- data_out->SetPropertyInt(STRING16(L"fileCount"), filenames.size());
- data_out->SetPropertyDouble(STRING16(L"fileTotalBytes"),
- static_cast<double>(file_total_bytes));
- JsObjectSetPropertyStringArray(module_environment, data_out,
- STRING16(L"fileExtensions"), file_extensions);
- JsObjectSetPropertyStringArray(module_environment, data_out,
- STRING16(L"fileMimeTypes"), file_mime_types);
-
- if (type == DRAG_AND_DROP_EVENT_DROP) {
- scoped_ptr<JsArray> file_array(
- module_environment->js_runner_->NewArray());
- if (!FileDialog::FilesToJsObjectArray(filenames, module_environment,
- file_array.get(), error_out)) {
- // FilesToJsObjectArray will set error_out.
- return;
- }
- data_out->SetPropertyArray(STRING16(L"files"), file_array.get());
- }
-}
-
-
-bool GetDroppedFiles(
- ModuleEnvironment *module_environment,
- nsIDragSession *drag_session,
- std::vector<std::string16> *filenames_out,
- std::set<std::string16> *file_extensions_out,
- std::set<std::string16> *file_mime_types_out,
- int64 *file_total_bytes_out) {
- filenames_out->clear();
- file_extensions_out->clear();
- file_mime_types_out->clear();
- *file_total_bytes_out = 0;
+ if (!AddFileDragAndDropData(module_environment,
+ drag_session.get(),
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out)) {
+ assert(!error_out->empty());
+ }
+}
+
+
+bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
+ nsIDragSession *drag_session,
+ bool is_in_a_drop,
+ JsObject *data_out,
+ std::string16 *error_out) {
+ if (!is_in_a_drop) {
+ // TODO(nigeltao): On Firefox2 / GTK / Linux, nsIDragSession->GetData
+ // is only valid during dragover and drop events, and not during dragenter
+ // dragleave events, since it is only during the first two events that
+ // Gecko calls dragSessionGTK->TargetSetLastContext with a valid GTK
+ // widget, in widget/src/gtk2/nsWindow.cpp.
+ //
+ // Thus, we return early, for non-drop events, to avoid a "Gtk-CRITICAL **:
+ // gtk_drag_get_data: assertion `GTK_IS_WIDGET (widget)' failed" error
+ // when calling nsIDragSession->GetNumDropItems.
+ //
+ // However, to be consistent with the other browsers / OSes, we should
+ // provide aggregate file drag and drop data during dragenter, which
+ // might mean that we (Gears) have to bypass Gecko's allegedly cross-
+ // platform nsIDragSession interface and go lower down into OS-level
+ // (e.g. Win32 or GTK) drag and drop APIs.
+ return true;
+ }
+
#if defined(LINUX) && !defined(OS_MACOSX)
// Although Firefox's underlying XPCOM widget library aims to present a
// consistent cross-platform interface, there are still significant
@@ -428,6 +392,7 @@
nsresult nr = drag_session->GetNumDropItems(&num_drop_items);
if (NS_FAILED(nr) || num_drop_items <= 0) { return false; }
+ std::vector<std::string16> filenames;
for (int i = 0; i < static_cast<int>(num_drop_items); i++) {
nsCOMPtr<nsITransferable> transferable =
do_CreateInstance("@mozilla.org/widget/transferable;1", &nr);
@@ -455,9 +420,6 @@
nsString filename;
nr = file->GetPath(filename);
if (NS_FAILED(nr)) { return false; }
- PRInt64 file_size = 0;
- nr = file->GetFileSize(&file_size);
- if (NS_FAILED(nr)) { return false; }
#else
nsCOMPtr<nsIFile> file(do_QueryInterface(data));
if (!file) { return false; }
@@ -486,23 +448,12 @@
nr = local_file->GetPath(filename);
}
if (NS_FAILED(nr)) { return false; }
- // TODO(nigeltao): Check that this gets the size of the link target,
- // in the case of .lnk (Windows) or alias (OSX) files.
- PRInt64 file_size = 0;
- nr = local_file->GetFileSize(&file_size);
- if (NS_FAILED(nr)) { return false; }
#endif
- filenames_out->push_back(std::string16(filename.get()));
- // TODO(nigeltao): Decide whether we should insert ".txt" or "txt" -
- // that is, does the file extension include the dot at the start.
- file_extensions_out->insert(File::GetFileExtension(filename.get()));
- // TODO(nigeltao): Should we also keep an array of per-file MIME types,
- // not just the overall set of MIME types. If so, the mimeType should
- // probably be a property of the file JavaScript object (i.e. the thing
- // with a name and blob property), not a separate array to the files array.
- file_mime_types_out->insert(DetectMimeTypeOfFile(filename.get()));
- *file_total_bytes_out += file_size;
- }
- return true;
-}
+ filenames.push_back(std::string16(filename.get()));
+ }
+ FileDragAndDropMetaData file_drag_and_drop_meta_data;
+ file_drag_and_drop_meta_data.SetFilenames(filenames);
+ return file_drag_and_drop_meta_data.ToJsObject(
+ module_environment, is_in_a_drop, data_out, error_out);
+}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#4
-
/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 15:47:47.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-08 16:46:07.000000000 +1100
@@ -33,12 +33,11 @@
class nsIDragSession;
-bool GetDroppedFiles(ModuleEnvironment *module_environment,
- nsIDragSession *drag_session,
- std::vector<std::string16> *filenames_out,
- std::set<std::string16> *file_extensions_out,
- std::set<std::string16> *file_mime_types_out,
- int64 *file_total_bytes_out);
+bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
+ nsIDragSession *drag_session,
+ bool is_in_a_drop,
+ JsObject *data_out,
+ std::string16 *error_out);
void AcceptDrag(ModuleEnvironment *module_environment,
JsObject *event,
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#17 -
/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-08 15:47:47.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-08 17:45:55.000000000 +1100
@@ -223,40 +223,22 @@
nsString event_type;
event->GetType(event_type);
+ 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(),
+ event_type.Equals(kDragDropAsString),
+ context_object.get(),
+ &ignored)) {
+ return NS_ERROR_FAILURE;
+ }
+
if (on_drop_.get() && event_type.Equals(kDragDropAsString)) {
ProvideDebugVisualFeedback(false);
if (will_accept_drop_) {
will_accept_drop_ = false;
- std::vector<std::string16> filenames;
- std::set<std::string16> file_extensions;
- std::set<std::string16> file_mime_types;
- int64 file_total_bytes;
- // TODO(nigeltao): We should call GetDroppedFiles (and provide the
- // aggregate file metadata) during dragenter, dragover and dragleave,
- // not just during drop.
- if (!GetDroppedFiles(module_environment_.get(),
- drag_session.get(),
- &filenames,
- &file_extensions,
- &file_mime_types,
- &file_total_bytes)) {
- return NS_ERROR_FAILURE;
- }
-
- scoped_ptr<JsObject> context_object(
- module_environment_->js_runner_->NewObject());
- scoped_ptr<JsArray> file_array(
- module_environment_->js_runner_->NewArray());
- std::string16 ignored;
- if (!FileDialog::FilesToJsObjectArray(filenames,
- module_environment_.get(),
- file_array.get(),
- &ignored)) {
- return NS_ERROR_FAILURE;
- }
- context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
- AddEventToJsObject(context_object.get(), event);
-
// Prevent the default browser behavior of navigating away from the
// current page to the file being dropped.
event->StopPropagation();
@@ -285,9 +267,6 @@
is_drag_exit = true;
}
if (callback) {
- scoped_ptr<JsObject> context_object(
- module_environment_->js_runner_->NewObject());
- AddEventToJsObject(context_object.get(), event);
const int argc = 1;
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
