Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9427257
or point your web browser to
http://mondrian/9427257
to review the following code:
Change 9427257 by nigel...@nigeltao-srcgears2 on 2008/12/16 17:19:36 *pending*
In Firefox/Linux drag and drop, provide (aggregate) file metadata
during dragenter and dragover, just like IE and Safari.
PRESUBMIT=passed
R=noel
[email protected]
DELTA=251 (221 added, 15 deleted, 15 changed)
OCL=9427257
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#12
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#6
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#19
edit
... //depot/googleclient/gears/opensource/gears/factory/factory_ff.cc#3 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#8
edit
251 delta lines: 221 added, 15 deleted, 15 changed
Also consider running:
g4 lint -c 9427257
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 9427257 by nigel...@nigeltao-srcgears2 on 2008/12/16 17:19:36 *pending*
In Firefox/Linux drag and drop, provide (aggregate) file metadata
during dragenter and dragover, just like IE and Safari.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#12
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#6
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#19
edit
... //depot/googleclient/gears/opensource/gears/factory/factory_ff.cc#3 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#8
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc#3
-
/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-15 19:49:04.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.cc
2008-12-15 19:33:15.000000000 +1100
@@ -92,6 +92,10 @@
bool is_in_a_drop,
JsObject *object_out,
std::string16 *error_out) {
+ if (filenames_.empty()) {
+ return false;
+ }
+
// TODO(nigeltao): Error checking. We should return empty (or 0) instead of
// partial results, in case of failure.
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#12
-
/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-15 19:49:04.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2008-12-16 17:17:55.000000000 +1100
@@ -35,11 +35,15 @@
#include <gecko_sdk/include/nsILocalFile.h>
#include <gecko_sdk/include/nsISupportsPrimitives.h>
#include <gecko_sdk/include/nsXPCOM.h>
+#if defined(LINUX) && !defined(OS_MACOSX)
+#include <gtk/gtk.h>
+#endif
#include "gears/desktop/drag_and_drop_utils_ff.h"
#include "gears/base/common/file.h"
#include "gears/base/common/mime_detect.h"
+#include "gears/base/common/string_utils.h"
#include "gears/base/firefox/ns_file_utils.h"
#include "gears/desktop/file_dialog.h"
@@ -335,39 +339,208 @@
// 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.
+ // Note that, on Linux, we do cache, due to g_file_drag_and_drop_meta_data,
+ // but this doesn't apply for Firefox/Windows and Firefox/Mac.
return AddFileDragAndDropData(module_environment,
drag_session.get(),
- type == DRAG_AND_DROP_EVENT_DROP,
+ type,
data_out,
error_out);
}
+#if defined(LINUX) && !defined(OS_MACOSX)
+// On Firefox2 / GTK / Linux, nsIDragSession->GetData is only valid during
+// dragover and drop events (and in particular not during dragenter 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.
+//
+// Nonetheless, we still want to provide (aggregate) file metadata via Gears'
+// JavaScript API druing dragenter and dragover. To workaround the lack of a
+// nsIDragSession, we interrogate the X server directly about its
+// XdndSelection, via GTK / GDK.
+//
+// GTK emits "signals" for UI events such as drag_motion and drag_leave.
+// Gecko adds itself as the first handler of such events on the main GTK
+// window (in mozilla/widget/src/gtk2/nsWindow.cpp), and signal handlers
+// can return FALSE to stop the signal propagating to later handlers that
+// have been connected via g_signal_connect. Instead, we attach an "emission
+// hook", rather than a conventional signal handler, which is not tied to
+// any particular GtkWidget (and hence we install our emission hook exactly
+// once), and also should never be removed (and hence it always returns
+// TRUE). The emission hook is run before conventional signal handlers,
+// and so we get to query the X server for what's being dragged (i.e. the
+// XdndSelection) just before Gears needs it (in response to a JavaScript-
+// level dragenter, dragover or dragexit event).
+static enum {
+ XDND_SELECTION_STATE_NONE,
+ XDND_SELECTION_STATE_ABOUT_TO_CONVERT,
+ XDND_SELECTION_STATE_WAITING,
+ XDND_SELECTION_STATE_TIMED_OUT,
+ XDND_SELECTION_STATE_REPLY_RECEIVED
+} g_xdnd_selection_state = XDND_SELECTION_STATE_NONE;
+
+
+static scoped_ptr<FileDragAndDropMetaData> g_file_drag_and_drop_meta_data;
+
+
+static gboolean on_selection(GtkWidget *widget,
+ GtkSelectionData *selection_data,
+ guint time,
+ gpointer user_data) {
+ if (g_xdnd_selection_state != XDND_SELECTION_STATE_WAITING) {
+ if (g_xdnd_selection_state == XDND_SELECTION_STATE_ABOUT_TO_CONVERT) {
+ g_xdnd_selection_state = XDND_SELECTION_STATE_REPLY_RECEIVED;
+ }
+ return TRUE;
+ }
+ g_xdnd_selection_state = XDND_SELECTION_STATE_REPLY_RECEIVED;
+ if (selection_data->data == NULL) {
+ return TRUE;
+ }
+ g_file_drag_and_drop_meta_data.reset(new FileDragAndDropMetaData);
+
+ std::vector<std::string> filenames_as_ascii_urls;
+ std::string uri_list(reinterpret_cast<const char*>(selection_data->data));
+ // Even though Unix line endings are traditionally \n and not \r\n, the
+ // text-uri format specifies \r\n as delimeters.
+ Tokenize<std::string>(uri_list, "\r\n", &filenames_as_ascii_urls);
+
+ // Convert from file:// URLs to an actual filename (which also involves
+ // decoding %20s into spaces, for example).
+ bool success = true;
+ std::vector<std::string16> filenames;
+ for (std::vector<std::string>::iterator i = filenames_as_ascii_urls.begin();
+ i != filenames_as_ascii_urls.end(); ++i) {
+ std::string16 filename16;
+ UTF8ToString16((*i).c_str(), &filename16);
+
+ nsString filename_nsstring(filename16.c_str(), filename16.length());
+ nsCOMPtr<nsIFile> file;
+ nsresult nr = NSFileUtils::GetFileFromURLSpec(
+ filename_nsstring, getter_AddRefs(file));
+ if (NS_FAILED(nr)) { success = false; break; }
+ nsString filename;
+ nr = file->GetPath(filename);
+ if (NS_FAILED(nr)) { success = false; break; }
+ filenames.push_back(std::string16(filename.get()));
+ }
+
+ if (success) {
+ g_file_drag_and_drop_meta_data->SetFilenames(filenames);
+ }
+ return TRUE;
+}
+
+
+static gboolean on_drag_motion_signal_hook(GSignalInvocationHint *ihint,
+ guint n_param_values,
+ const GValue *param_values,
+ gpointer data) {
+ if (g_xdnd_selection_state != XDND_SELECTION_STATE_NONE) {
+ return TRUE;
+ }
+
+ static GtkWidget *selection_widget = NULL;
+ static GdkAtom atom1 = GDK_NONE;
+ static GdkAtom atom2 = GDK_NONE;
+ if (!selection_widget) {
+ selection_widget = gtk_window_new(GTK_WINDOW_POPUP);
+ gtk_window_set_screen(GTK_WINDOW(selection_widget),
+ gtk_widget_get_screen(selection_widget));
+ gtk_window_resize(GTK_WINDOW(selection_widget), 1, 1);
+ gtk_window_move(GTK_WINDOW(selection_widget), -100, -100);
+ gtk_widget_show(selection_widget);
+ g_signal_connect(
+ selection_widget, "selection_received", G_CALLBACK(on_selection),
NULL);
+ atom1 = gdk_atom_intern("XdndSelection", FALSE);
+ atom2 = gdk_atom_intern("text/uri-list", FALSE);
+ }
+
+ g_xdnd_selection_state = XDND_SELECTION_STATE_ABOUT_TO_CONVERT;
+ gtk_selection_convert(selection_widget, atom1, atom2, GDK_CURRENT_TIME);
+
+ // If the gtk_selection_convert call above immediately triggers the
+ // selection_received signal, then the drag source is in the same process
+ // (i.e. it is from the Firefox chrome, or it is from a web page), and we
+ // don't trust the source (in case an arbitrary (evil) web page can add a
+ // "file:///etc/passwd" URL as a drag source, in order to try to read the
+ // user's file system). Instead, we only accept a drag source if it is
+ // from a separate process, such as a file manager.
+ if (g_xdnd_selection_state != XDND_SELECTION_STATE_ABOUT_TO_CONVERT) {
+ return TRUE;
+ }
+
+ g_xdnd_selection_state = XDND_SELECTION_STATE_WAITING;
+ PRTime entryTime = PR_Now();
+ while (g_xdnd_selection_state == XDND_SELECTION_STATE_WAITING) {
+ // We're copying Gecko, which sleeps for 20ms between iterations (as seen
+ // in mozilla/widget/src/gtk2/nsDragService.cpp).
+ usleep(20);
+ // Gecko's NS_DND_TIMEOUT is 500ms, or equivalently, 500000 microseconds.
+ if (PR_Now() - entryTime > 500000) {
+ g_xdnd_selection_state = XDND_SELECTION_STATE_TIMED_OUT;
+ break;
+ }
+ gtk_main_iteration();
+ }
+
+ return TRUE;
+}
+
+static gboolean on_drag_leave_signal_hook(GSignalInvocationHint *ihint,
+ guint n_param_values,
+ const GValue *param_values,
+ gpointer data) {
+ g_xdnd_selection_state = XDND_SELECTION_STATE_NONE;
+ return TRUE;
+}
+
+
+void InitializeGtkSignalEmissionHooks() {
+ static bool initialized = false;
+ if (initialized) {
+ return;
+ }
+ initialized = true;
+ g_signal_add_emission_hook(
+ g_signal_lookup("drag_motion", GTK_TYPE_WIDGET), 0,
+ on_drag_motion_signal_hook, NULL, NULL);
+ g_signal_add_emission_hook(
+ g_signal_lookup("drag_leave", GTK_TYPE_WIDGET), 0,
+ on_drag_leave_signal_hook, NULL, NULL);
+}
+#endif // defined(LINUX) && !defined(OS_MACOSX)
+
+
bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
nsIDragSession *drag_session,
- bool is_in_a_drop,
+ DragAndDropEventType type,
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 (type == DRAG_AND_DROP_EVENT_DRAGLEAVE) {
+ return false;
}
#if defined(LINUX) && !defined(OS_MACOSX)
+ // For DRAG_AND_DROP_EVENT_DRAGENTER and DRAG_AND_DROP_EVENT_DRAGOVER
+ // events, we ignore the nsIDragSession, and instead query the
+ // g_file_drag_and_drop_meta_data object that was set up earlier, during
+ // our signal emission hooks for GTK's "drag-motion" signal.
+ //
+ // For DRAG_AND_DROP_EVENT_DROP, we base our answer on the nsIDragSession,
+ // as g_file_drag_and_drop_meta_data will be empty at this time (even in
+ // the case of a valid file drop), since the GTK's "drag_leave" signal is
+ // emitted (which clears out g_file_drag_and_drop_meta_data) before Mozilla
+ // fires its dragdrop event.
+ if (type == DRAG_AND_DROP_EVENT_DRAGENTER ||
+ type == DRAG_AND_DROP_EVENT_DRAGOVER) {
+ return g_file_drag_and_drop_meta_data.get() &&
+ g_file_drag_and_drop_meta_data->ToJsObject(
+ module_environment, false, data_out, error_out);
+ }
+
// Although Firefox's underlying XPCOM widget library aims to present a
// consistent cross-platform interface, there are still significant
// differences in drag-and-drop. In particular, different OSes support
@@ -452,5 +625,8 @@
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);
-}
+ module_environment,
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out);
+}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#6
-
/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-15 19:49:05.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2008-12-15 18:43:44.000000000 +1100
@@ -36,7 +36,7 @@
bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
nsIDragSession *drag_session,
- bool is_in_a_drop,
+ DragAndDropEventType type,
JsObject *data_out,
std::string16 *error_out);
@@ -50,4 +50,8 @@
JsObject *data_out,
std::string16 *error_out);
+#if defined(LINUX) && !defined(OS_MACOSX)
+void InitializeGtkSignalEmissionHooks();
+#endif
+
#endif // GEARS_DESKTOP_DRAG_AND_DROP_UTILS_FF_H__
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#19 -
/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-15 19:49:05.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-12-11 18:48:52.000000000 +1100
@@ -225,6 +225,17 @@
nsString event_type;
event->GetType(event_type);
+ DragAndDropEventType type = DRAG_AND_DROP_EVENT_INVALID;
+ if (event_type.Equals(kDragOverAsString)) {
+ type = DRAG_AND_DROP_EVENT_DRAGOVER;
+ } else if (event_type.Equals(kDragEnterAsString)) {
+ type = DRAG_AND_DROP_EVENT_DRAGENTER;
+ } else if (event_type.Equals(kDragExitAsString)) {
+ type = DRAG_AND_DROP_EVENT_DRAGLEAVE;
+ } else if (event_type.Equals(kDragDropAsString)) {
+ type = DRAG_AND_DROP_EVENT_DROP;
+ }
+
std::string16 ignored;
scoped_ptr<JsObject> context_object(
@@ -232,13 +243,13 @@
AddEventToJsObject(context_object.get(), event);
if (!AddFileDragAndDropData(module_environment_.get(),
drag_session.get(),
- event_type.Equals(kDragDropAsString) == PR_TRUE,
+ type,
context_object.get(),
&ignored)) {
return NS_ERROR_FAILURE;
}
- if (on_drop_.get() && event_type.Equals(kDragDropAsString)) {
+ if (on_drop_.get() && (type == DRAG_AND_DROP_EVENT_DROP)) {
ProvideDebugVisualFeedback(false);
if (will_accept_drop_) {
will_accept_drop_ = false;
@@ -259,12 +270,13 @@
} else {
bool is_drag_exit = false;
JsRootedCallback *callback = NULL;
- if (on_drag_enter_.get() && event_type.Equals(kDragEnterAsString)) {
+ if (on_drag_enter_.get() && (type == DRAG_AND_DROP_EVENT_DRAGENTER)) {
ProvideDebugVisualFeedback(true);
callback = on_drag_enter_.get();
- } else if (on_drag_over_.get() && event_type.Equals(kDragOverAsString)) {
+ } else if (on_drag_over_.get() && (type == DRAG_AND_DROP_EVENT_DRAGOVER)) {
callback = on_drag_over_.get();
- } else if (on_drag_leave_.get() && event_type.Equals(kDragExitAsString)) {
+ } else if (on_drag_leave_.get() &&
+ (type == DRAG_AND_DROP_EVENT_DRAGLEAVE)) {
ProvideDebugVisualFeedback(false);
callback = on_drag_leave_.get();
is_drag_exit = true;
==== //depot/googleclient/gears/opensource/gears/factory/factory_ff.cc#3 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/factory/factory_ff.cc
====
# action=edit type=xtext
--- googleclient/gears/opensource/gears/factory/factory_ff.cc 2008-11-20
09:21:08.000000000 +1100
+++ googleclient/gears/opensource/gears/factory/factory_ff.cc 2008-12-15
18:44:10.000000000 +1100
@@ -32,6 +32,7 @@
#include "gears/base/common/base_class.h"
#include "gears/base/common/js_types.h"
+#include "gears/desktop/drag_and_drop_utils_ff.h"
// Boilerplate. == NS_IMPL_ISUPPORTS + ..._MAP_ENTRY_EXTERNAL_DOM_CLASSINFO
@@ -58,6 +59,9 @@
NULL, &factory_impl_)) {
return NS_ERROR_FAILURE;
}
+#if defined(LINUX) && !defined(OS_MACOSX)
+ InitializeGtkSignalEmissionHooks();
+#endif
unload_monitor_.reset(new JsEventMonitor(module_environment->js_runner_,
JSEVENT_UNLOAD, this));
return NS_OK;
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#8 -
/home/nigeltao/srcgears2/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
2008-12-16 15:33:50.000000000 +1100
+++ googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
2008-12-16 17:05:56.000000000 +1100
@@ -45,8 +45,10 @@
function willReject(context) {
var result = context.event.clientX >= 200;
- document.getElementById('rejectOutput').innerHTML =
- result ? 'Drop <b>REJECTED</b>.' : 'Drop <b>OK</b>.';
+ var html = result ? 'Drop <b>REJECTED</b>.' : 'Drop <b>OK</b>.';
+ html += '<br>' + context.count + ' files, ' +
+ context.totalBytes + ' bytes.';
+ document.getElementById('rejectOutput').innerHTML = html;
return result;
}
@@ -97,9 +99,13 @@
for (i = 0; i < context.files.length; i++) {
var file = context.files[i];
s += '<b>' + file.name + '</b> has length <b>' +
- file.blob.length + '</b>.<br/>';
+ file.blob.length + '</b><br>';
}
}
+ s += 'count: <b>' + context.count + '</b><br>';
+ s += 'totalBytes: <b>' + context.totalBytes + '</b><br>';
+ s += 'mimeTypes: <b>' + context.mimeTypes + '</b><br>';
+ s += 'extensions: <b>' + context.extensions + '</b><br>';
document.getElementById('dropOutput').innerHTML = s;
document.getElementById('rejectOutput').innerHTML = ' ';
}
