Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9908094
or point your web browser to
http://mondrian/9908094
to review the following code:
Change 9908094 by nigel...@nigeltao-srcwingears2 on 2009/01/29 17:49:07
*pending*
In drag-and-drop, move the "web-app-calls-gears" aka "v2" API over to
use a DropTargetInterceptor, to get the same cursor control as the
"gears-calls-web-app" aka "v1" API.
This involves creating a DropTargetInterceptor per ModuleEnvironment,
on IE, since there is no drag-and-drop initialization call (such as
the "v1" API's registerDropTarget) to otherwise attach the hook.
R=noel
[email protected]
DELTA=55 (21 added, 18 deleted, 16 changed)
OCL=9908094
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/base_class.cc#21
edit
... //depot/googleclient/gears/opensource/gears/base/common/base_class.h#25 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#17
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#11 edit
55 delta lines: 21 added, 18 deleted, 16 changed
Also consider running:
g4 lint -c 9908094
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 9908094 by nigel...@nigeltao-srcwingears2 on 2009/01/29 17:49:07
*pending*
In drag-and-drop, move the "web-app-calls-gears" aka "v2" API over to
use a DropTargetInterceptor, to get the same cursor control as the
"gears-calls-web-app" aka "v1" API.
This involves creating a DropTargetInterceptor per ModuleEnvironment,
on IE, since there is no drag-and-drop initialization call (such as
the "v1" API's registerDropTarget) to otherwise attach the hook.
OCL=9908094
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/base_class.cc#21
edit
... //depot/googleclient/gears/opensource/gears/base/common/base_class.h#25 edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#9
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#17
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#11 edit
==== //depot/googleclient/gears/opensource/gears/base/common/base_class.cc#21 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/base_class.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/base_class.cc
2009-01-29 14:37:43.000000000 +1100
+++ googleclient/gears/opensource/gears/base/common/base_class.cc
2009-01-29 15:11:19.000000000 +1100
@@ -36,6 +36,7 @@
#include "gears/base/firefox/dom_utils.h"
#elif BROWSER_IE || BROWSER_IEMOBILE
#include "gears/base/ie/activex_utils.h"
+#include "gears/desktop/drag_and_drop_utils_ie.h"
#elif BROWSER_NPAPI
#include "gears/base/npapi/browser_utils.h"
#include "gears/base/npapi/np_utils.h"
@@ -61,6 +62,8 @@
LEAK_COUNTER_INCREMENT(ModuleEnvironment);
#if BROWSER_FF
assert(js_context_ != NULL);
+#elif BROWSER_IE
+ drop_target_interceptor_.reset(DropTargetInterceptor::Intercept(this));
#endif
js_runner_->OnModuleEnvironmentAttach();
}
==== //depot/googleclient/gears/opensource/gears/base/common/base_class.h#25 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/base_class.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/base_class.h
2009-01-29 14:37:43.000000000 +1100
+++ googleclient/gears/opensource/gears/base/common/base_class.h
2009-01-29 15:10:03.000000000 +1100
@@ -41,6 +41,7 @@
#include "gears/base/common/string16.h" // for string16
#include "third_party/scoped_ptr/scoped_ptr.h"
+class DropTargetInterceptor;
class ModuleWrapperBaseClass;
#if defined(BROWSER_IEMOBILE)
@@ -93,6 +94,10 @@
CComPtr<IUnknown> iunknown_site_;
#endif
+#if BROWSER_IE
+ scoped_refptr<DropTargetInterceptor> drop_target_interceptor_;
+#endif
+
bool is_worker_;
JsRunnerInterface *js_runner_;
scoped_refptr<BrowsingContext> browsing_context_;
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#9
-
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-01-29 17:43:38.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2009-01-29 17:45:14.000000000 +1100
@@ -188,17 +188,16 @@
return;
}
- if (type != DRAG_AND_DROP_EVENT_DROP) {
+ if (type == DRAG_AND_DROP_EVENT_DRAGENTER ||
+ type == DRAG_AND_DROP_EVENT_DRAGOVER) {
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")))
{
+ FAILED(window_event->put_returnValue(CComVariant(false)))) {
*error_out = GET_INTERNAL_ERROR_MESSAGE();
return;
}
+
module_environment->drop_target_interceptor_->SetWillAcceptDrop(acceptance);
}
}
@@ -256,8 +255,8 @@
cached_meta_data_is_valid_ = false;
will_accept_drop_ = true;
HRESULT hr = original_drop_target_->DragEnter(object, state, point, effect);
- if (SUCCEEDED(hr) && !will_accept_drop_) {
- *effect = DROPEFFECT_NONE;
+ if (SUCCEEDED(hr)) {
+ *effect = will_accept_drop_ ? DROPEFFECT_COPY : DROPEFFECT_NONE;
}
return hr;
}
@@ -267,8 +266,8 @@
DWORD state, POINTL point, DWORD *effect) {
will_accept_drop_ = true;
HRESULT hr = original_drop_target_->DragOver(state, point, effect);
- if (SUCCEEDED(hr) && !will_accept_drop_) {
- *effect = DROPEFFECT_NONE;
+ if (SUCCEEDED(hr)) {
+ *effect = will_accept_drop_ ? DROPEFFECT_COPY : DROPEFFECT_NONE;
}
return hr;
}
@@ -302,6 +301,11 @@
void DropTargetInterceptor::HandleEvent(JsEventType event_type) {
assert(event_type == JSEVENT_UNLOAD);
module_environment_->js_runner_->RemoveEventHandler(JSEVENT_UNLOAD, this);
+
+ // Break the ModuleEnvironment <--> DropTargetInterceptor reference cycle.
+ assert(module_environment_->drop_target_interceptor_ == this);
+ module_environment_->drop_target_interceptor_.reset(NULL);
+
is_revoked_ = true;
// TODO(nigeltao): Should we check if we are still the hwnd's droptarget?
RevokeDragDrop(hwnd_);
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#17 -
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-01-29 14:37:44.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2009-01-29 17:54:42.000000000 +1100
@@ -84,12 +84,6 @@
return NULL;
}
- drop_target->interceptor_.reset(
- DropTargetInterceptor::Intercept(module_environment));
- if (!drop_target->interceptor_.get()) {
- return NULL;
- }
-
hr = drop_target->DispEventAdvise(dom_element_dispatch,
&DIID_HTMLElementEvents2);
if (FAILED(hr)) { return NULL; }
@@ -178,8 +172,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- interceptor_->GetFileDragAndDropMetaData().ToJsObject(
- module_environment_.get(), false, context_object.get(), &error);
+ module_environment_->drop_target_interceptor_->
+ GetFileDragAndDropMetaData().ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -198,7 +193,8 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
- interceptor_->SetWillAcceptDrop(will_accept_drop_);
+ module_environment_->drop_target_interceptor_->
+ SetWillAcceptDrop(will_accept_drop_);
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -221,8 +217,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- interceptor_->GetFileDragAndDropMetaData().ToJsObject(
- module_environment_.get(), false, context_object.get(), &error);
+ module_environment_->drop_target_interceptor_->
+ GetFileDragAndDropMetaData().ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -236,7 +233,8 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
- interceptor_->SetWillAcceptDrop(will_accept_drop_);
+ module_environment_->drop_target_interceptor_->
+ SetWillAcceptDrop(will_accept_drop_);
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -263,8 +261,9 @@
// TODO(nigeltao): To match the behavior on other browsers, we should not
// pass up file drag-and-drop metadata on DRAGLEAVE, but only on DRAGENTER,
// DRAGOVER and DROP.
- interceptor_->GetFileDragAndDropMetaData().ToJsObject(
- module_environment_.get(), false, context_object.get(), &error);
+ module_environment_->drop_target_interceptor_->
+ GetFileDragAndDropMetaData().ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -293,8 +292,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- interceptor_->GetFileDragAndDropMetaData().ToJsObject(
- module_environment_.get(), true, context_object.get(), &error);
+ module_environment_->drop_target_interceptor_->
+ GetFileDragAndDropMetaData().ToJsObject(
+ module_environment_.get(), true, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -320,7 +320,6 @@
if (event_source_) {
DispEventUnadvise(event_source_, &DIID_HTMLElementEvents2);
}
- interceptor_.reset(NULL);
Unref(); // Balanced by an Ref() call during CreateDropTarget.
}
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#11 -
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
2009-01-29 14:37:44.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2009-01-29 17:47:05.000000000 +1100
@@ -98,14 +98,6 @@
bool unregister_self_has_been_called_;
bool will_accept_drop_;
- // TODO(nigeltao): This is an interceptor for the "gears-calls-the-webapp"
- // or "version 1" DnD API, but we will also need an interceptor for the
- // "the-webapp-calls-gears" or "version 2" DnD API, which will probably be
- // a member variable of the ModuleEnvironment. If that happens, then we
- // will not need a per-DropTarget interceptor, and can therefore eliminate
- // this member variable.
- scoped_refptr<DropTargetInterceptor> interceptor_;
-
#ifdef DEBUG
CComPtr<IHTMLStyle> html_style_;
#endif
