Hello noel,
I'd like you to do a code review. Please execute
g4 diff -c 9500806
or point your web browser to
http://mondrian/9500806
to review the following code:
Change 9500806 by nigel...@nigeltao-srcwingears2 on 2008/12/22 20:59:02
*pending*
On IE, introduce the DropTargetInterceptor class, which allows us
to set the cursor correctly to NONE when JavaScript rejects a file
drag and drop.
R=noel
[email protected]
DELTA=266 (239 added, 11 deleted, 16 changed)
OCL=9500806
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#15
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#14
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#5
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#16
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#10 edit
266 delta lines: 239 added, 11 deleted, 16 changed
Also consider running:
g4 lint -c 9500806
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 9500806 by nigel...@nigeltao-srcwingears2 on 2008/12/22 20:59:02
*pending*
On IE, introduce the DropTargetInterceptor class, which allows us
to set the cursor correctly to NONE when JavaScript rejects a file
drag and drop.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#15
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#14
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#5
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#16
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#10 edit
==== //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#15
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/leak_counter.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/leak_counter.cc
2008-12-22 19:43:14.000000000 +1100
+++ googleclient/gears/opensource/gears/base/common/leak_counter.cc
2008-12-22 12:51:47.000000000 +1100
@@ -42,6 +42,7 @@
static const char16 *leak_counter_names[] = {
STRING16(L"DocumentJsRunner"),
STRING16(L"DropTarget"),
+ STRING16(L"DropTargetInterceptor"),
STRING16(L"FFHttpRequest"),
STRING16(L"IEHttpRequest"),
STRING16(L"JavaScriptWorkerInfo"),
==== //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#14
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/leak_counter.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/leak_counter.h
2008-12-22 19:43:14.000000000 +1100
+++ googleclient/gears/opensource/gears/base/common/leak_counter.h
2008-12-22 12:51:37.000000000 +1100
@@ -42,6 +42,7 @@
enum LeakCounterType {
LEAK_COUNTER_TYPE_DocumentJsRunner,
LEAK_COUNTER_TYPE_DropTarget,
+ LEAK_COUNTER_TYPE_DropTargetInterceptor,
LEAK_COUNTER_TYPE_FFHttpRequest,
LEAK_COUNTER_TYPE_IEHttpRequest,
LEAK_COUNTER_TYPE_JavaScriptWorkerInfo,
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc#6
-
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
2008-12-22 19:43:14.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.cc
2008-12-22 20:58:43.000000000 +1100
@@ -82,13 +82,10 @@
bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
- bool is_in_a_drop,
- JsObject *data_out,
- std::string16 *error_out) {
+ FileDragAndDropMetaData *meta_data_out) {
CComPtr<IHTMLWindow2> html_window_2;
if (FAILED(ActiveXUtils::GetHtmlWindow2(
module_environment->iunknown_site_, &html_window_2))) {
- *error_out = STRING16(L"Could not access the IHtmlWindow2.");
return false;
}
@@ -139,11 +136,8 @@
GlobalUnlock(stg_medium.hGlobal);
ReleaseStgMedium(&stg_medium);
}
-
- FileDragAndDropMetaData meta_data;
- meta_data.SetFilenames(filenames);
- return meta_data.ToJsObject(
- module_environment, is_in_a_drop, data_out, error_out);
+ meta_data_out->SetFilenames(filenames);
+ return true;
}
@@ -219,11 +213,169 @@
return false;
}
- return AddFileDragAndDropData(module_environment,
- type == DRAG_AND_DROP_EVENT_DROP,
- data_out,
- error_out);
-}
+ FileDragAndDropMetaData meta_data;
+ return AddFileDragAndDropData(module_environment, &meta_data) &&
+ meta_data.ToJsObject(
+ module_environment,
+ type == DRAG_AND_DROP_EVENT_DROP,
+ data_out,
+ error_out);
+}
+
+
+STDMETHODIMP DropTargetInterceptor::QueryInterface(REFIID riid, void **object)
{
+ if ((riid == IID_IUnknown) || (riid == IID_IDropTarget)) {
+ *object = this;
+ AddRef();
+ return S_OK;
+ } else {
+ return E_NOINTERFACE;
+ }
+}
+
+
+STDMETHODIMP_(ULONG) DropTargetInterceptor::AddRef() {
+ Ref();
+ return 1;
+}
+
+
+STDMETHODIMP_(ULONG) DropTargetInterceptor::Release() {
+ Unref();
+ return 1;
+}
+
+
+STDMETHODIMP DropTargetInterceptor::DragEnter(
+ IDataObject *object, DWORD state, POINTL point, DWORD *effect) {
+ cached_module_environment_.reset(NULL);
+ will_accept_drop_ = true;
+ HRESULT hr = original_drop_target_->DragEnter(object, state, point, effect);
+ if (SUCCEEDED(hr) && !will_accept_drop_) {
+ *effect = DROPEFFECT_NONE;
+ }
+ return hr;
+}
+
+
+STDMETHODIMP DropTargetInterceptor::DragOver(
+ 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;
+ }
+ return hr;
+}
+
+
+STDMETHODIMP DropTargetInterceptor::DragLeave() {
+ HRESULT hr = original_drop_target_->DragLeave();
+ cached_module_environment_.reset(NULL);
+ return hr;
+}
+
+
+STDMETHODIMP DropTargetInterceptor::Drop(
+ IDataObject *object, DWORD state, POINTL point, DWORD *effect) {
+ HRESULT hr = original_drop_target_->Drop(object, state, point, effect);
+ cached_module_environment_.reset(NULL);
+ return hr;
+}
+
+
+FileDragAndDropMetaData &DropTargetInterceptor::GetFileDragAndDropMetaData(
+ ModuleEnvironment *module_environment) {
+ assert(module_environment);
+ if (module_environment != cached_module_environment_.get()) {
+ cached_meta_data_.Reset();
+ bool ok = AddFileDragAndDropData(module_environment, &cached_meta_data_);
+ cached_module_environment_.reset(ok ? module_environment : NULL);
+ }
+ return cached_meta_data_;
+}
+
+
+void DropTargetInterceptor::SetWillAcceptDrop(bool will_accept_drop) {
+ will_accept_drop_ = will_accept_drop;
+}
+
+
+DropTargetInterceptor::DropTargetInterceptor(
+ HWND hwnd,
+ IDropTarget *original_drop_target)
+ : cached_module_environment_(NULL),
+ hwnd_(hwnd),
+ original_drop_target_(original_drop_target),
+ will_accept_drop_(true) {
+ LEAK_COUNTER_INCREMENT(DropTargetInterceptor);
+ assert(hwnd);
+ assert(original_drop_target);
+ RevokeDragDrop(hwnd_);
+ RegisterDragDrop(hwnd_, this);
+ instances_[hwnd_] = this;
+}
+
+
+DropTargetInterceptor::~DropTargetInterceptor() {
+ LEAK_COUNTER_DECREMENT(DropTargetInterceptor);
+ instances_.erase(hwnd_);
+ RevokeDragDrop(hwnd_);
+ if (original_drop_target_) {
+ RegisterDragDrop(hwnd_, original_drop_target_);
+ }
+}
+
+
+static BOOL CALLBACK EnumChildWindowsProc(HWND hwnd, LPARAM param) {
+ WCHAR class_name[100];
+ GetClassName(hwnd, static_cast<LPTSTR>(class_name), 100);
+ if (_tcscmp(class_name, L"Internet Explorer_Server")) {
+ return TRUE;
+ }
+
+ DWORD process_id = 0;
+ DWORD thread_id = GetWindowThreadProcessId(hwnd, &process_id);
+
+ if (thread_id == GetCurrentThreadId() &&
+ process_id == GetCurrentProcessId()) {
+ *(reinterpret_cast<HWND*>(param)) = hwnd;
+ return FALSE;
+ } else {
+ return TRUE;
+ }
+}
+
+
+std::map<HWND, DropTargetInterceptor*> DropTargetInterceptor::instances_;
+
+
+DropTargetInterceptor *DropTargetInterceptor::Intercept(
+ ModuleEnvironment *module_environment) {
+ HWND browser_window = 0;
+ HWND child = 0;
+ if (GetBrowserWindow(module_environment, &browser_window)) {
+ EnumChildWindows(browser_window, EnumChildWindowsProc, (LPARAM)&child);
+ }
+ if (!child) {
+ return NULL;
+ }
+
+ std::map<HWND, DropTargetInterceptor*>::iterator i =
+ instances_.find(child);
+ DropTargetInterceptor *interceptor = NULL;
+ if (i != instances_.end()) {
+ interceptor = i->second;
+ } else {
+ IDropTarget *original_drop_target =
+ static_cast<IDropTarget*>(GetProp(child, L"OleDropTargetInterface"));
+ if (original_drop_target) {
+ interceptor = new DropTargetInterceptor(child, original_drop_target);
+ }
+ }
+ return interceptor;
+}
+
#endif // GEARS_DRAG_AND_DROP_API_IS_SUPPORTED_FOR_THIS_PLATFORM
#endif // OFFICIAL_BUILD
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h#5
-
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
2008-12-22 19:43:15.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_utils_ie.h
2008-12-22 20:54:11.000000000 +1100
@@ -26,18 +26,63 @@
#ifndef GEARS_DESKTOP_DRAG_AND_DROP_UTILS_IE_H__
#define GEARS_DESKTOP_DRAG_AND_DROP_UTILS_IE_H__
+#include <map>
+
#include "gears/base/common/base_class.h"
#include "gears/desktop/drag_and_drop_utils_common.h"
+
+
+// A DropTargetInterceptor is a IDropTarget implementation that sits in front
+// of (and mostly delegates to) Internet Explorer's default IDropTarget. Its
+// purpose is to allow Gears to respond last to drag and drop events, which
+// allows us to set an effect (i.e. cursor) of DROPEFFECT_NONE, when rejecting
+// a file drop. In comparison, trying to set event.dropEffect to 'none', via
+// IE's JavaScript is overridden by IE's default behavior for file drops, which
+// is to show DROPEFFECT_COPY or DROPEFFECT_LINK, but not DROPEFFECT_NONE.
+//
+// This is also where we cache file drag-and-drop metadata so that, for
+// example, during the numerous DRAGOVER events, we only query the clipboard
+// (and the filesystem) once.
+class DropTargetInterceptor : public IDropTarget, public RefCounted {
+ public:
+ static DropTargetInterceptor *Intercept(
+ ModuleEnvironment *module_environment);
+
+ STDMETHODIMP QueryInterface(REFIID riid, void **object);
+ STDMETHODIMP_(ULONG) AddRef();
+ STDMETHODIMP_(ULONG) Release();
+ STDMETHODIMP DragEnter(
+ IDataObject *object, DWORD state, POINTL point, DWORD *effect);
+ STDMETHODIMP DragOver(DWORD state, POINTL point, DWORD *effect);
+ STDMETHODIMP DragLeave();
+ STDMETHODIMP Drop(
+ IDataObject *object, DWORD state, POINTL point, DWORD *effect);
+
+ FileDragAndDropMetaData &GetFileDragAndDropMetaData(
+ ModuleEnvironment *module_environment);
+
+ void SetWillAcceptDrop(bool will_accept_drop);
+
+ private:
+ static std::map<HWND, DropTargetInterceptor*> instances_;
+
+ FileDragAndDropMetaData cached_meta_data_;
+ scoped_refptr<ModuleEnvironment> cached_module_environment_;
+ HWND hwnd_;
+ CComPtr<IDropTarget> original_drop_target_;
+ bool will_accept_drop_;
+
+ DropTargetInterceptor(HWND hwnd, IDropTarget *original_drop_target);
+ ~DropTargetInterceptor();
+
+ DISALLOW_EVIL_CONSTRUCTORS(DropTargetInterceptor);
+};
+
HRESULT GetHtmlDataTransfer(
IHTMLWindow2 *html_window_2,
CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer);
-
-bool AddFileDragAndDropData(ModuleEnvironment *module_environment,
- 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_ie.cc#16 -
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-12-22 19:43:15.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-12-22 19:02:40.000000000 +1100
@@ -84,6 +84,12 @@
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; }
@@ -103,6 +109,7 @@
if (FAILED(hr)) { return NULL; }
drop_target->Ref(); // Balanced by a Unref() call during UnregisterSelf.
drop_target->ProvideDebugVisualFeedback(false);
+
return drop_target.get();
}
@@ -133,6 +140,11 @@
HRESULT DropTarget::HandleOnDragEnter()
{
+ // TODO(nigeltao): Does will_accept_drop_ need to be a member variable, or
+ // can it be just a local variable for HandleOnDragEnter and HandleOnDragOver
+ // (and hence we can eliminate DropTarget::CancelEventBubble), since fiddling
+ // around with things like event.cancelBubble is probably overridden by
+ // whatever we do in the DropTargetInterceptor.
will_accept_drop_ = false;
ProvideDebugVisualFeedback(true);
CComPtr<IHTMLEventObj> html_event_obj;
@@ -167,8 +179,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- AddFileDragAndDropData(module_environment_.get(), false,
- context_object.get(), &error);
+ interceptor_->GetFileDragAndDropMetaData(
+ module_environment_.get()).ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -187,6 +200,7 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
+ interceptor_->SetWillAcceptDrop(will_accept_drop_);
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -209,8 +223,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- AddFileDragAndDropData(module_environment_.get(), false,
- context_object.get(), &error);
+ interceptor_->GetFileDragAndDropMetaData(
+ module_environment_.get()).ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -224,6 +239,7 @@
will_accept_drop_ = return_value.get() &&
V_VT(&return_value->token()) == VT_BOOL &&
V_BOOL(&return_value->token()) == false;
+ interceptor_->SetWillAcceptDrop(will_accept_drop_);
}
hr = CancelEventBubble(html_event_obj, html_data_transfer);
@@ -247,8 +263,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- AddFileDragAndDropData(module_environment_.get(), false,
- context_object.get(), &error);
+ interceptor_->GetFileDragAndDropMetaData(
+ module_environment_.get()).ToJsObject(
+ module_environment_.get(), false, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -277,8 +294,9 @@
module_environment_->js_runner_->NewObject());
AddEventToJsObject(context_object.get());
std::string16 error;
- AddFileDragAndDropData(module_environment_.get(), true,
- context_object.get(), &error);
+ interceptor_->GetFileDragAndDropMetaData(
+ module_environment_.get()).ToJsObject(
+ module_environment_.get(), true, context_object.get(), &error);
if (!error.empty()) {
return E_FAIL;
}
@@ -304,6 +322,7 @@
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#10 -
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-12-22 19:43:15.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2008-12-22 20:40:19.000000000 +1100
@@ -36,6 +36,8 @@
#include <windows.h>
#include "gears/desktop/drop_target_base.h"
+
+class DropTargetInterceptor;
// The 1 in the IDispEventSimpleImpl line is just an arbitrary positive
// integer, as says the IDispEventSimpleImpl documentation at
@@ -96,6 +98,14 @@
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
