Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9955210
or point your web browser to
http://mondrian/9955210
to review the following code:
Change 9955210 by nigel...@nigeltao-srcwingears2 on 2009/02/02 17:42:28
*pending*
Rename desktop.acceptDrag(event, boolean) to
desktop.setDragCursor(event, string),
where the string is something like "copy" or "none".
This moves the onus for calling things like cancelBubble,
stopPropagation from
Gears to the web-app, for the "v2" or "web-app-calls-Gears"
drag-and-drop API.
R=noel
[email protected]
DELTA=151 (61 added, 42 deleted, 48 changed)
OCL=9955210
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#73 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#28 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#13
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#7
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#10
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#7
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#18
edit
151 delta lines: 61 added, 42 deleted, 48 changed
Also consider running:
g4 lint -c 9955210
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 9955210 by nigel...@nigeltao-srcwingears2 on 2009/02/02 17:42:28
*pending*
Rename desktop.acceptDrag(event, boolean) to
desktop.setDragCursor(event, string),
where the string is something like "copy" or "none".
This moves the onus for calling things like cancelBubble,
stopPropagation from
Gears to the web-app, for the "v2" or "web-app-calls-Gears"
drag-and-drop API.
Affected files ...
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#73 edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.h#28 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#4
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#13
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#7
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#10
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#7
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#18
edit
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#73 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/desktop.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2009-02-02
17:28:43.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2009-02-02
17:41:40.000000000 +1100
@@ -74,10 +74,9 @@
// The Drag-and-Drop API has not been finalized for official builds.
#else
#if GEARS_DRAG_AND_DROP_API_IS_SUPPORTED_FOR_THIS_PLATFORM
- // TODO(nigeltao): should acceptDrag be renamed finishDrag??
- RegisterMethod("acceptDrag", &GearsDesktop::AcceptDrag);
RegisterMethod("getDragData", &GearsDesktop::GetDragData);
RegisterMethod("registerDropTarget", &GearsDesktop::RegisterDropTarget);
+ RegisterMethod("setDragCursor", &GearsDesktop::SetDragCursor);
#endif
#endif // OFFICIAL_BUILD
}
@@ -828,33 +827,43 @@
// The Drag-and-Drop API has not been finalized for official builds.
#else
#if GEARS_DRAG_AND_DROP_API_IS_SUPPORTED_FOR_THIS_PLATFORM
-void GearsDesktop::AcceptDrag(JsCallContext *context) {
+void GearsDesktop::SetDragCursor(JsCallContext *context) {
if (EnvIsWorker()) {
context->SetException(
- STRING16(L"acceptDrag is not supported in workers."));
+ STRING16(L"setDragCursor is not supported in workers."));
return;
}
scoped_ptr<JsObject> event_as_js_object;
- bool acceptance;
+ std::string16 cursor;
JsArgument argv[] = {
{ JSPARAM_REQUIRED, JSPARAM_OBJECT, &event_as_js_object },
- { JSPARAM_REQUIRED, JSPARAM_BOOL, &acceptance },
+ { JSPARAM_REQUIRED, JSPARAM_STRING16, &cursor },
};
context->GetArguments(ARRAYSIZE(argv), argv);
if (context->is_exception_set()) return;
+ DragAndDropCursorType cursor_type = DRAG_AND_DROP_CURSOR_INVALID;
std::string16 error;
#if BROWSER_FF || BROWSER_IE || BROWSER_WEBKIT
- ::AcceptDrag(module_environment_.get(),
- event_as_js_object.get(),
- acceptance,
- &error);
+ if (cursor == STRING16(L"copy")) {
+ cursor_type = DRAG_AND_DROP_CURSOR_COPY;
+ } else if (cursor == STRING16(L"none")) {
+ cursor_type = DRAG_AND_DROP_CURSOR_NONE;
+ } else {
+ error = STRING16(L"Unsupported cursor type passed to setDragCursor.");
+ }
#else
// TODO(nigeltao): implement on Chromium.
- error = STRING16(L"acceptDrag is not supported for this platform.");
+ error = STRING16(L"setDragCursor is not supported for this platform.");
#endif
+ if (cursor_type != DRAG_AND_DROP_CURSOR_INVALID) {
+ ::SetDragCursor(module_environment_.get(),
+ event_as_js_object.get(),
+ cursor_type,
+ &error);
+ }
if (!error.empty()) {
context->SetException(error);
return;
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.h#28 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/desktop.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.h 2009-02-02
17:28:44.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/desktop.h 2009-02-02
16:48:06.000000000 +1100
@@ -174,16 +174,16 @@
// The Drag-and-Drop API has not been finalized for official builds.
#else
// TODO(nigeltao): decide on exactly one model for the DnD API:
- // * The "Web-app calls Gears" model (GetDragData followed by AcceptDrag), or
+ // * The "Web-app calls Gears" model (GetDragData and SetDragCursor), or
// * The "Gears calls Web-app" model (RegisterDropTarget).
-
- // IN: Event event, bool acceptance
- // OUT: -
- void AcceptDrag(JsCallContext *context);
// IN: Event event, string flavor
// OUT: object data
void GetDragData(JsCallContext *context);
+
+ // IN: Event event, string cursor
+ // OUT: -
+ void SetDragCursor(JsCallContext *context);
// IN: DomElement div, object options
// OUT: -
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h#4
-
c:\devel\srcwingears2/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
2009-02-02 17:28:45.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_common.h
2009-02-02 15:06:05.000000000 +1100
@@ -29,6 +29,12 @@
#include <set>
#include <vector>
#include "gears/base/common/js_types.h"
+
+enum DragAndDropCursorType {
+ DRAG_AND_DROP_CURSOR_NONE,
+ DRAG_AND_DROP_CURSOR_COPY,
+ DRAG_AND_DROP_CURSOR_INVALID
+};
enum DragAndDropEventType {
DRAG_AND_DROP_EVENT_DRAGENTER,
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc#13
-
c:\devel\srcwingears2/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
2009-02-02 17:28:45.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.cc
2009-02-02 17:40:31.000000000 +1100
@@ -260,10 +260,10 @@
}
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event_as_js_object,
- bool acceptance,
- std::string16 *error_out) {
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event_as_js_object,
+ DragAndDropCursorType cursor_type,
+ std::string16 *error_out) {
nsCOMPtr<nsIDOMEvent> dom_event;
DragAndDropEventType type = GetDragAndDropEventType(
module_environment, event_as_js_object, &dom_event);
@@ -271,12 +271,12 @@
*error_out = STRING16(L"The drag-and-drop event is invalid.");
return;
}
-
if (type == DRAG_AND_DROP_EVENT_DROP) {
- dom_event->StopPropagation();
-
- } else if (type == DRAG_AND_DROP_EVENT_DRAGENTER ||
- type == DRAG_AND_DROP_EVENT_DRAGOVER) {
+ return;
+ }
+
+ // TODO(nigeltao): Check that this works on all three OSes (Win/Mac/Linux).
+ if (cursor_type != DRAG_AND_DROP_CURSOR_INVALID) {
nsCOMPtr<nsIDragService> drag_service =
do_GetService("@mozilla.org/widget/dragservice;1");
if (!drag_service) {
@@ -291,9 +291,13 @@
return;
}
- nr = drag_session->SetDragAction(acceptance
- ? static_cast<int>(nsIDragService::DRAGDROP_ACTION_COPY)
- : static_cast<int>(nsIDragService::DRAGDROP_ACTION_NONE));
+ if (cursor_type == DRAG_AND_DROP_CURSOR_COPY) {
+ nr = drag_session->SetDragAction(nsIDragService::DRAGDROP_ACTION_COPY);
+ } else if (cursor_type == DRAG_AND_DROP_CURSOR_NONE) {
+ nr = drag_session->SetDragAction(nsIDragService::DRAGDROP_ACTION_NONE);
+ } else {
+ assert(false);
+ }
if (NS_FAILED(nr)) {
*error_out = GET_INTERNAL_ERROR_MESSAGE();
return;
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h#7
-
c:\devel\srcwingears2/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
2009-02-02 17:28:45.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ff.h
2009-02-02 16:12:56.000000000 +1100
@@ -40,15 +40,15 @@
JsObject *data_out,
std::string16 *error_out);
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
- bool acceptance,
- std::string16 *error_out);
-
bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
+
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event,
+ DragAndDropCursorType cursor_type,
+ std::string16 *error_out);
#if defined(LINUX) && !defined(OS_MACOSX)
void InitializeGtkSignalEmissionHooks();
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#10
-
c:\devel\srcwingears2/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
2009-02-02 17:28:46.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2009-02-02 17:46:38.000000000 +1100
@@ -177,38 +177,18 @@
}
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
- bool acceptance,
- std::string16 *error_out) {
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event,
+ DragAndDropCursorType cursor_type,
+ std::string16 *error_out) {
CComPtr<IHTMLEventObj> window_event;
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 (type != DRAG_AND_DROP_EVENT_DROP) {
- // TODO(nigeltao): Should acceptance be a string (e.g. "copy" or "none")
- // rather than a boolean, for future expansibility.
-
module_environment->drop_target_interceptor_->SetWillAcceptDrop(acceptance);
-
- // TODO(nigeltao): Should Gears even be responsible for cancelBubble /
- // preventDefault / stopPropagation of the event, since that can be done
- // from JavaScript (unlike setting the cursor to "none" without being
- // over-ridden by the browser's default cursor settings)? If not (and
- // hence if we remove the paragraph of code below), should we rename
- // acceptDrag to setDragCursor?
- CComQIPtr<IHTMLEventObj2> window_event2(window_event);
- CComPtr<IHTMLDataTransfer> data_transfer;
- if (!window_event2 ||
- FAILED(window_event->put_returnValue(CComVariant(false))) ||
- FAILED(window_event->put_cancelBubble(VARIANT_TRUE)) ||
- FAILED(window_event2->get_dataTransfer(&data_transfer)) ||
- FAILED(data_transfer->put_dropEffect(acceptance ? L"copy" : L"none")))
{
- *error_out = GET_INTERNAL_ERROR_MESSAGE();
- return;
- }
+ module_environment->drop_target_interceptor_->SetDragCursor(cursor_type);
}
}
@@ -264,10 +244,14 @@
STDMETHODIMP DropTargetInterceptor::DragEnter(
IDataObject *object, DWORD state, POINTL point, DWORD *effect) {
cached_meta_data_is_valid_ = false;
- will_accept_drop_ = true;
+ cursor_type_ = DRAG_AND_DROP_CURSOR_INVALID;
HRESULT hr = original_drop_target_->DragEnter(object, state, point, effect);
- if (SUCCEEDED(hr) && !will_accept_drop_) {
- *effect = DROPEFFECT_NONE;
+ if (SUCCEEDED(hr)) {
+ if (cursor_type_ == DRAG_AND_DROP_CURSOR_COPY) {
+ *effect = DROPEFFECT_COPY;
+ } else if (cursor_type_ == DRAG_AND_DROP_CURSOR_NONE) {
+ *effect = DROPEFFECT_NONE;
+ }
}
return hr;
}
@@ -275,10 +259,14 @@
STDMETHODIMP DropTargetInterceptor::DragOver(
DWORD state, POINTL point, DWORD *effect) {
- will_accept_drop_ = true;
+ cursor_type_ = DRAG_AND_DROP_CURSOR_INVALID;
HRESULT hr = original_drop_target_->DragOver(state, point, effect);
- if (SUCCEEDED(hr) && !will_accept_drop_) {
- *effect = DROPEFFECT_NONE;
+ if (SUCCEEDED(hr)) {
+ if (cursor_type_ == DRAG_AND_DROP_CURSOR_COPY) {
+ *effect = DROPEFFECT_COPY;
+ } else if (cursor_type_ == DRAG_AND_DROP_CURSOR_NONE) {
+ *effect = DROPEFFECT_NONE;
+ }
}
return hr;
}
@@ -327,8 +315,8 @@
}
-void DropTargetInterceptor::SetWillAcceptDrop(bool will_accept_drop) {
- will_accept_drop_ = will_accept_drop;
+void DropTargetInterceptor::SetDragCursor(DragAndDropCursorType cursor_type) {
+ cursor_type_ = cursor_type;
}
@@ -340,7 +328,7 @@
module_environment_(module_environment),
hwnd_(hwnd),
original_drop_target_(original_drop_target),
- will_accept_drop_(true),
+ cursor_type_(DRAG_AND_DROP_CURSOR_INVALID),
is_revoked_(false) {
LEAK_COUNTER_INCREMENT(DropTargetInterceptor);
LOG16((L"DropTargetInterceptor::ctor %p\n", this));
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#6
-
c:\devel\srcwingears2/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
2009-01-29 14:37:44.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2009-02-02 15:41:05.000000000 +1100
@@ -66,7 +66,7 @@
FileDragAndDropMetaData &GetFileDragAndDropMetaData();
virtual void HandleEvent(JsEventType event_type);
- void SetWillAcceptDrop(bool will_accept_drop);
+ void SetDragCursor(DragAndDropCursorType cursor_type);
private:
static std::map<HWND, DropTargetInterceptor*> instances_;
@@ -77,7 +77,7 @@
scoped_refptr<ModuleEnvironment> module_environment_;
HWND hwnd_;
CComPtr<IDropTarget> original_drop_target_;
- bool will_accept_drop_;
+ DragAndDropCursorType cursor_type_;
bool is_revoked_;
DropTargetInterceptor(
@@ -95,14 +95,14 @@
CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer);
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
- bool acceptance,
- std::string16 *error_out);
-
bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event,
+ DragAndDropCursorType cursor_type,
+ std::string16 *error_out);
+
#endif // GEARS_DESKTOP_DRAG_AND_DROP_UTILS_IE_H__
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h#7
-
c:\devel\srcwingears2/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
2009-02-02 17:28:46.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.h
2009-02-02 16:52:15.000000000 +1100
@@ -59,14 +59,14 @@
bool IsInADragOperation();
bool IsInADropOperation();
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
- bool acceptance,
- std::string16 *error_out);
-
bool GetDragData(ModuleEnvironment *module_environment,
JsObject *event,
JsObject *data_out,
std::string16 *error_out);
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event,
+ DragAndDropCursorType cursor_type,
+ 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#9
-
c:\devel\srcwingears2/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
2009-02-02 17:28:46.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_sf.mm
2009-02-02 17:39:20.000000000 +1100
@@ -217,19 +217,27 @@
module_environment, IsInADropOperation(), data_out, error_out);
};
-void AcceptDrag(ModuleEnvironment *module_environment,
- JsObject *event,
- bool acceptance,
- std::string16 *error_out) {
+void SetDragCursor(ModuleEnvironment *module_environment,
+ JsObject *event,
+ DragAndDropCursorType cursor_type,
+ std::string16 *error_out) {
// As per GetDragData below, 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;
}
- event->SetPropertyBool(STRING16(L"returnValue"), false);
- g_drag_operation_has_been_set = true;
- g_drag_operation = acceptance ? NSDragOperationCopy : NSDragOperationNone;
+ if (!IsInADropOperation()) {
+ if (cursor_type == DRAG_AND_DROP_CURSOR_COPY) {
+ g_drag_operation_has_been_set = true;
+ g_drag_operation = NSDragOperationCopy;
+ } else if (cursor_type == DRAG_AND_DROP_CURSOR_NONE) {
+ g_drag_operation_has_been_set = true;
+ g_drag_operation = NSDragOperationNone;
+ } else {
+ assert(cursor_type == DRAG_AND_DROP_CURSOR_INVALID);
+ }
+ }
}
bool GetDragData(ModuleEnvironment *module_environment,
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#18 -
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
2009-02-02 17:28:46.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2009-02-02 15:42:44.000000000 +1100
@@ -193,8 +193,10 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
- module_environment_->drop_target_interceptor_->
- SetWillAcceptDrop(will_accept_drop_);
+ if (!will_accept_drop_) {
+ module_environment_->drop_target_interceptor_->
+ SetDragCursor(DRAG_AND_DROP_CURSOR_NONE);
+ }
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -233,8 +235,10 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
- module_environment_->drop_target_interceptor_->
- SetWillAcceptDrop(will_accept_drop_);
+ if (!will_accept_drop_) {
+ module_environment_->drop_target_interceptor_->
+ SetDragCursor(DRAG_AND_DROP_CURSOR_NONE);
+ }
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
