Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 8930787
or point your web browser to
http://mondrian/8930787
to review the following code:
Change 8930787 by [EMAIL PROTECTED] on 2008/11/10 16:11:56 *pending*
ondragenter and ondragover now need to return false to accept Gears'
DnD.
R=noel
[EMAIL PROTECTED]
DELTA=112 (69 added, 3 deleted, 40 changed)
OCL=8930787
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#6 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#7 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#7
edit
112 delta lines: 69 added, 3 deleted, 40 changed
Also consider running:
g4 lint -c 8930787
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 8930787 by [EMAIL PROTECTED] on 2008/11/10 16:11:56 *pending*
ondragenter and ondragover now need to return false to accept Gears'
DnD.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#6 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#10
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#7 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#7
edit
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#10 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-11-10 15:29:04.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-11-10 15:38:04.000000000 +1100
@@ -57,7 +57,8 @@
JsObject *options,
std::string16 *error_out)
: DropTargetBase(module_environment, options, error_out),
- unregister_self_has_been_called_(false) {
+ unregister_self_has_been_called_(false),
+ will_accept_drop_(false) {
LEAK_COUNTER_INCREMENT(DropTarget);
}
@@ -213,46 +214,46 @@
NS_IMETHODIMP DropTarget::HandleEvent(nsIDOMEvent *event) {
nsCOMPtr<nsIDragService> drag_service =
do_GetService("@mozilla.org/widget/dragservice;1");
- if (!drag_service) { return false; }
+ if (!drag_service) { return NS_ERROR_FAILURE; }
nsCOMPtr<nsIDragSession> drag_session;
nsresult nr = drag_service->GetCurrentSession(getter_AddRefs(drag_session));
- if (NS_FAILED(nr) || !drag_session.get()) { return false; }
- nr = drag_session->SetDragAction(nsIDragService::DRAGDROP_ACTION_COPY);
- if (NS_FAILED(nr)) { return false; }
+ if (NS_FAILED(nr) || !drag_session.get()) { return NS_ERROR_FAILURE; }
nsString event_type;
event->GetType(event_type);
if (on_drop_.get() && event_type.Equals(kDragDropAsString)) {
ProvideDebugVisualFeedback(false);
- std::string16 error;
- scoped_ptr<JsArray> file_array(
- module_environment_->js_runner_->NewArray());
- if (!GetDroppedFiles(drag_session.get(), file_array.get(), &error)) {
- return NS_ERROR_FAILURE;
- }
-
- // If we've got this far, then the drag-and-dropped data was indeed one or
- // more files, so we will notify our callback. We also stop the event
- // propagation, to avoid the browser doing the default action, which is to
- // load that file (and navigate away from the current page). I (nigeltao)
- // would have thought that event->PreventDefault() would be the way to do
- // that, but that doesn't seem to work, whilst StopPropagation() does.
- event->StopPropagation();
-
- scoped_ptr<JsObject> context_object(
- module_environment_->js_runner_->NewObject());
- context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
- AddEventToJsObject(context_object.get(), event);
-
- const int argc = 1;
- JsParamToSend argv[argc] = {
- { JSPARAM_OBJECT, context_object.get() }
- };
- module_environment_->js_runner_->InvokeCallback(
- on_drop_.get(), argc, argv, NULL);
+ if (will_accept_drop_) {
+ will_accept_drop_ = false;
+ std::string16 error;
+ scoped_ptr<JsArray> file_array(
+ module_environment_->js_runner_->NewArray());
+ if (!GetDroppedFiles(drag_session.get(), file_array.get(), &error)) {
+ return NS_ERROR_FAILURE;
+ }
+
+ // Prevent the default browser behavior of navigating away from the
+ // current page to the file being dropped.
+ event->StopPropagation();
+
+ scoped_ptr<JsObject> context_object(
+ module_environment_->js_runner_->NewObject());
+ context_object->SetPropertyArray(STRING16(L"files"), file_array.get());
+ AddEventToJsObject(context_object.get(), event);
+
+ const int argc = 1;
+ JsParamToSend argv[argc] = {
+ { JSPARAM_OBJECT, context_object.get() }
+ };
+ module_environment_->js_runner_->InvokeCallback(
+ on_drop_.get(), argc, argv, NULL);
+ nr = drag_session->SetDragAction(nsIDragService::DRAGDROP_ACTION_COPY);
+ if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
+ }
} else {
+ bool is_drag_exit = false;
JsRootedCallback *callback = NULL;
if (on_drag_enter_.get() && event_type.Equals(kDragEnterAsString)) {
ProvideDebugVisualFeedback(true);
@@ -262,6 +263,7 @@
} else if (on_drag_leave_.get() && event_type.Equals(kDragExitAsString)) {
ProvideDebugVisualFeedback(false);
callback = on_drag_leave_.get();
+ is_drag_exit = true;
}
if (callback) {
scoped_ptr<JsObject> context_object(
@@ -271,8 +273,27 @@
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
};
- module_environment_->js_runner_->InvokeCallback(
- callback, argc, argv, NULL);
+ if (is_drag_exit) {
+ module_environment_->js_runner_->InvokeCallback(
+ callback, argc, argv, NULL);
+ will_accept_drop_ = false;
+ } else {
+ scoped_ptr<JsRootedToken> return_value;
+ module_environment_->js_runner_->InvokeCallback(
+ callback, argc, argv, as_out_parameter(return_value));
+ // The HTML5 specification (section 5.4.5) says that an event handler
+ // returning *false* means that event processing is complete, and we
+ // should not perform the default action (i.e. the web-app wants
+ // Gears' file drop behavior, and not the default browser behavior
+ // of navigating away from the current page to the file being dropped).
+ will_accept_drop_ = return_value.get() &&
+ JSVAL_IS_BOOLEAN(return_value->token()) &&
+ JSVAL_TO_BOOLEAN(return_value->token()) == false;
+ nr = drag_session->SetDragAction(will_accept_drop_
+ ? static_cast<int>(nsIDragService::DRAGDROP_ACTION_COPY)
+ : static_cast<int>(nsIDragService::DRAGDROP_ACTION_NONE));
+ if (NS_FAILED(nr)) { return NS_ERROR_FAILURE; }
+ }
}
}
return NS_OK;
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#6 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drop_target_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-11-10 15:29:04.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-11-10 15:29:13.000000000 +1100
@@ -74,6 +74,7 @@
nsCOMPtr<nsIDOMEventTarget> event_target_;
nsCOMPtr<nsIXPConnect> xp_connect_;
bool unregister_self_has_been_called_;
+ bool will_accept_drop_;
DropTarget(ModuleEnvironment *module_environment,
JsObject *options,
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#10 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-11-10 16:03:11.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-11-10 15:56:20.000000000 +1100
@@ -46,7 +46,8 @@
JsObject *options,
std::string16 *error_out)
: DropTargetBase(module_environment, options, error_out),
- unregister_self_has_been_called_(false) {
+ unregister_self_has_been_called_(false),
+ will_accept_drop_(false) {
LEAK_COUNTER_INCREMENT(DropTarget);
}
@@ -130,6 +131,7 @@
CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer)
{
+ if (!will_accept_drop_) return S_OK;
HRESULT hr;
hr = html_data_transfer->put_dropEffect(L"copy");
if (FAILED(hr)) return hr;
@@ -142,6 +144,7 @@
HRESULT DropTarget::HandleOnDragEnter()
{
+ will_accept_drop_ = false;
ProvideDebugVisualFeedback(true);
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
@@ -177,8 +180,17 @@
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
};
+ scoped_ptr<JsRootedToken> return_value;
module_environment_->js_runner_->InvokeCallback(
- on_drag_enter_.get(), argc, argv, NULL);
+ on_drag_enter_.get(), argc, argv, as_out_parameter(return_value));
+ // The HTML5 specification (section 5.4.5) says that an event handler
+ // returning *false* means that event processing is complete, and we
+ // should not perform the default action (i.e. the web-app wants
+ // Gears' file drop behavior, and not the default browser behavior
+ // of navigating away from the current page to the file being dropped).
+ will_accept_drop_ = return_value.get() &&
+ V_VT(&return_value->token()) == VT_BOOL &&
+ V_BOOL(&return_value->token()) == false;
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -189,6 +201,7 @@
HRESULT DropTarget::HandleOnDragOver()
{
+ will_accept_drop_ = false;
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
@@ -202,8 +215,12 @@
JsParamToSend argv[argc] = {
{ JSPARAM_OBJECT, context_object.get() }
};
+ scoped_ptr<JsRootedToken> return_value;
module_environment_->js_runner_->InvokeCallback(
- on_drag_over_.get(), argc, argv, NULL);
+ on_drag_over_.get(), argc, argv, as_out_parameter(return_value));
+ will_accept_drop_ = return_value.get() &&
+ V_VT(&return_value->token()) == VT_BOOL &&
+ V_BOOL(&return_value->token()) == false;
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -215,6 +232,7 @@
HRESULT DropTarget::HandleOnDragLeave()
{
ProvideDebugVisualFeedback(false);
+ will_accept_drop_ = false;
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
@@ -241,6 +259,8 @@
HRESULT DropTarget::HandleOnDragDrop()
{
ProvideDebugVisualFeedback(false);
+ if (!will_accept_drop_) return S_OK;
+ will_accept_drop_ = false;
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#7 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drop_target_ie.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2008-11-10 16:03:11.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2008-11-10 15:34:49.000000000 +1100
@@ -95,9 +95,10 @@
virtual void HandleEvent(JsEventType event_type);
private:
- bool unregister_self_has_been_called_;
CComPtr<IDispatch> event_source_;
CComPtr<IHTMLWindow2> html_window_2_;
+ bool unregister_self_has_been_called_;
+ bool will_accept_drop_;
#ifdef DEBUG
CComPtr<IHTMLStyle> html_style_;
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#7 -
c:\devel\srcwingears2/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-11-10 15:29:04.000000000 +1100
+++ googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
2008-11-10 16:09:56.000000000 +1100
@@ -1,6 +1,6 @@
<html><head><title>Gears Drag and Drop</title></head>
<body>
-<div id="dropZone" style="background-color: yellow; width:20em; height:15em">
+<div id="dropZone" style="background-color:yellow; width:20em; height:15em">
<ol>
<li>Drag some files from the desktop.</li>
<li>Drop them on this DIV.</li>
@@ -10,11 +10,16 @@
<button onclick="toggleEnabledDisabled()">Toggle Enabled / Disabled</button>
<div id="enabledDisabled" style="color:red; font-weight:bold"> </div>
<div id="eventOutput"> </div>
+<div id="rejectOutput"> </div>
</div>
<div id="dragEnterOutput"> </div>
<div id="dragOverOutput"> </div>
<div id="dragLeaveOutput"> </div>
<div id="dropOutput"> </div>
+
+<div style="position:absolute; left:200px; top:13em;">
+ |← Drop to the left of this mark</div>
+
<script type="text/javascript" src="../../sdk/gears_init.js"></script>
<script type="text/javascript">
@@ -29,8 +34,20 @@
var dropTargetRegistration = null;
function updateEventOutput(event) {
+ if (!event) {
+ return;
+ }
document.getElementById('eventOutput').innerHTML =
- 'clientX: ' + event.clientX + ', ' + 'clientY: ' + event.clientY;
+ 'clientX: <b>' + event.clientX + '</b>, ' +
+ 'clientY: <b>' + event.clientY + '</b>, ' +
+ 'shiftKey: <b>' + event.shiftKey + '</b>';
+}
+
+function willReject(context) {
+ var result = context.event.clientX >= 200;
+ document.getElementById('rejectOutput').innerHTML =
+ result ? 'Drop <b>REJECTED</b>.' : 'Drop <b>OK</b>.';
+ return result;
}
function toggleEnabledDisabled() {
@@ -50,6 +67,7 @@
document.getElementById('dragEnterOutput').innerHTML =
'Got <b>dragenter</b>: ' + dragEnterCount +
' times, most recent sequence number is ' + eventCount;
+ return willReject(context);
},
'ondragover': function(context) {
dragOverCount++;
@@ -58,6 +76,7 @@
document.getElementById('dragOverOutput').innerHTML =
'Got <b>dragover</b>: ' + dragOverCount +
' times, most recent sequence number is ' + eventCount;
+ return willReject(context);
},
'ondragleave': function(context) {
dragLeaveCount++;
@@ -66,6 +85,7 @@
document.getElementById('dragLeaveOutput').innerHTML =
'Got <b>dragleave</b>: ' + dragLeaveCount +
' times, most recent sequence number is ' + eventCount;
+ document.getElementById('rejectOutput').innerHTML = ' ';
},
'ondrop': function(context) {
dropCount++;
@@ -73,12 +93,15 @@
updateEventOutput(context.event);
var s = 'Got <b>drop</b>: ' + dropCount +
' times, most recent sequence number is ' + eventCount + '<hr/>';
- 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/>';
+ if (context.files) {
+ 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/>';
+ }
}
document.getElementById('dropOutput').innerHTML = s;
+ document.getElementById('rejectOutput').innerHTML = ' ';
}
});
}
