Hello aa,
I'd like you to do a code review. Please execute
g4 diff -c 8653382
or point your web browser to
http://mondrian/8653382
to review the following code:
Change 8653382 by [EMAIL PROTECTED] on 2008/10/20 19:11:26 *pending*
Make IE's DropTarget implementation use IDispEventSimpleImpl
instead of IDispatchImpl. This avoids the situation where the
IDispatch of the DropTarget interferes with the IDispatch of the
DOM element, and the latter loses its scripted properties (and
therefore things like document.getElementById fails).
R=aa
[EMAIL PROTECTED]
DELTA=190 (46 added, 93 deleted, 51 changed)
OCL=8653382
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc#6
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#3 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#2 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#1
edit
190 delta lines: 46 added, 93 deleted, 51 changed
Also consider running:
g4 lint -c 8653382
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 8653382 by [EMAIL PROTECTED] on 2008/10/20 19:11:26 *pending*
Make IE's DropTarget implementation use IDispEventSimpleImpl
instead of IDispatchImpl. This avoids the situation where the
IDispatch of the DropTarget interferes with the IDispatch of the
DOM element, and the latter loses its scripted properties (and
therefore things like document.getElementById fails).
OCL=8653382
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc#6
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#3 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#2 edit
...
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#1
edit
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc#6
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc
2008-10-20 18:28:30.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc
2008-10-21 13:46:55.000000000 +1100
@@ -105,6 +105,7 @@
if (!InitializeDropTarget(
sibling_module, js_callbacks, drop_target, error_out)) {
delete drop_target;
+ *error_out = STRING16(L"Could not initialize a DropTarget.");
return NULL;
}
@@ -112,39 +113,17 @@
return drop_target;
#elif BROWSER_IE && !defined(OS_WINCE)
- CComQIPtr<IHTMLElement2> html_element_2(dom_element.dispatch());
- if (!html_element_2) return NULL;
- CComBSTR behavior_url(L"#Google" PRODUCT_SHORT_NAME L"#DropTarget");
-
- CComObject<DropTarget> *drop_target;
- if (FAILED(CComObject<DropTarget>::CreateInstance(&drop_target))) {
+ scoped_ptr<DropTarget>
drop_target(DropTarget::CreateDropTarget(dom_element));
+ if (!drop_target.get()) {
+ *error_out = STRING16(L"Could not create a DropTarget.");
return NULL;
}
- CComPtr<CComObject<DropTarget> >
- reference_adder_drop_target(drop_target);
if (!InitializeDropTarget(
- sibling_module, js_callbacks, drop_target, error_out)) {
+ sibling_module, js_callbacks, drop_target.get(), error_out)) {
+ *error_out = STRING16(L"Could not initialize a DropTarget.");
return NULL;
}
-
- // DropTarget overrides IDispatch::Invoke to react to drag and drop events.
- // To intercept those events, we have to attach an IElementBehavior (which
- // also happens to be the same DropTarget object) to the html_element_2, and
- // to do that, we have to introduce that html_element_2 to an
- // IElementBehaviorFactory (which also happens to be the same DropTarget).
- // Conceptually, the IDispatch, IElementBehavior and IElementBehaviorFactory
- // could be separate instances of separate classes, but for convenience, all
- // three roles are played by the one instance of DropTarget.
- CComQIPtr<IElementBehaviorFactory> factory(drop_target);
- if (!factory) return NULL;
- CComVariant factory_as_variant(static_cast<IUnknown*>(factory));
- LONG ignored_cookie = 0;
- if (FAILED(html_element_2->addBehavior(behavior_url,
- &factory_as_variant,
- &ignored_cookie))) {
- return NULL;
- }
- return drop_target;
+ return drop_target.release();
#else
*error_out = STRING16(L"Desktop.registerDropTarget is not implemented.");
@@ -157,10 +136,7 @@
#if BROWSER_FF
drop_target->RemoveSelfAsEventListeners();
#elif BROWSER_IE && !defined(OS_WINCE)
- // On IE, DropTarget is a COM object, which is ref-counted, so it should
- // automatically delete itself when no longer referred to.
- // In the future, if we allow explicit unregistration of a DropTarget, then
- // we might need to AddRef it during RegisterDropTarget, and Unref it here.
+ delete drop_target;
#endif
}
#endif // OFFICIAL_BUILD
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#3 -
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-10-20 18:28:30.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-10-20 19:12:18.000000000 +1100
@@ -32,76 +32,50 @@
#else
#include "gears/desktop/drop_target_ie.h"
-#include <mshtmdid.h>
+#include "gears/base/common/leak_counter.h"
#include "gears/base/common/string16.h"
#include "gears/base/ie/activex_utils.h"
#include "gears/desktop/drag_and_drop_registry.h"
#include "gears/desktop/file_dialog.h"
-HRESULT DropTarget::Detach(void) {
- return S_OK;
-}
-
-
-HRESULT DropTarget::Init(IElementBehaviorSite *element_behavior_site) {
- element_behavior_site_ = element_behavior_site;
- return S_OK;
-}
-
-
-HRESULT DropTarget::Notify(long lEvent, VARIANT *var) {
+_ATL_FUNC_INFO DropTarget::atl_func_info_ =
+ { CC_STDCALL, VT_I4, 1, { VT_EMPTY } };
+
+
+DropTarget::DropTarget() {
+ LEAK_COUNTER_INCREMENT(DropTarget);
+}
+
+
+DropTarget::~DropTarget() {
+ LEAK_COUNTER_DECREMENT(DropTarget);
+ if (event_source_for_disp_event_unadvise_) {
+ DispEventUnadvise(event_source_for_disp_event_unadvise_,
+ &DIID_HTMLElementEvents2);
+ }
+}
+
+
+DropTarget *DropTarget::CreateDropTarget(JsDomElement &dom_element) {
HRESULT hr;
- if (lEvent != BEHAVIOREVENT_CONTENTREADY) {
- return S_OK;
- }
- CComPtr<IHTMLElement> html_element;
- hr = element_behavior_site_->GetElement(&html_element);
- if (FAILED(hr)) return hr;
-
- // TODO(nigeltao): Keep the cookie somewhere, and Unadvise (i.e. detach
- // the behavior from the element) at an appropriate time.
- DWORD ignored_cookie;
- hr = AtlAdvise(html_element, reinterpret_cast<IUnknown*>(this),
- DIID_HTMLElementEvents2, &ignored_cookie);
- if (FAILED(hr)) return hr;
-
- return S_OK;
-}
-
-
-HRESULT DropTarget::FindBehavior(
- BSTR name,
- BSTR url,
- IElementBehaviorSite *behavior_site,
- IElementBehavior **behavior_out) {
- if (!name || (_wcsicmp(name, L"DropTarget") != 0)) {
- return E_FAIL;
- }
- return QueryInterface(__uuidof(IElementBehavior),
- reinterpret_cast<void**>(behavior_out));
-}
-
-
-HRESULT DropTarget::Invoke(DISPID dispidMember, REFIID riid,
- LCID lcid, WORD wFlags, DISPPARAMS *pdispparams, VARIANT *pvarResult,
- EXCEPINFO *pexcepinfo, UINT *puArgErr)
-{
- switch (dispidMember) {
- case DISPID_HTMLELEMENTEVENTS_ONDRAGENTER:
- return HandleOnDragEnter();
- case DISPID_HTMLELEMENTEVENTS_ONDRAGOVER:
- return HandleOnDragOver();
- case DISPID_HTMLELEMENTEVENTS_ONDRAGLEAVE:
- return HandleOnDragLeave();
- case DISPID_HTMLELEMENTEVENTS_ONDROP:
- return HandleOnDragDrop();
- default:
- // Do nothing;
- break;
- }
- return IDispatchImpl<IDispatch>::Invoke(dispidMember, riid, lcid, wFlags,
- pdispparams, pvarResult, pexcepinfo, puArgErr);
+ IDispatch *dom_element_dispatch = dom_element.dispatch();
+ scoped_ptr<DropTarget> drop_target(new DropTarget);
+
+ hr = drop_target->DispEventAdvise(dom_element_dispatch,
+ &DIID_HTMLElementEvents2);
+ if (FAILED(hr)) { return NULL; }
+ drop_target->event_source_for_disp_event_unadvise_ = dom_element_dispatch;
+ CComQIPtr<IHTMLElement> html_element(dom_element_dispatch);
+ if (!html_element) { return NULL; }
+ CComPtr<IDispatch> document_dispatch;
+ hr = html_element->get_document(&document_dispatch);
+ if (FAILED(hr)) { return NULL; }
+ CComQIPtr<IHTMLDocument2> html_document_2(document_dispatch);
+ if (!html_document_2) { return NULL; }
+ hr = html_document_2->get_parentWindow(&drop_target->html_window_2_);
+ if (FAILED(hr)) { return NULL; }
+ return drop_target.release();
}
@@ -110,18 +84,6 @@
CComPtr<IHTMLDataTransfer> &html_data_transfer)
{
HRESULT hr;
- if (!html_window_2_) {
- CComPtr<IHTMLElement> html_element;
- hr = element_behavior_site_->GetElement(&html_element);
- if (FAILED(hr)) return hr;
- CComPtr<IDispatch> dispatch;
- hr = html_element->get_document(&dispatch);
- if (FAILED(hr)) return hr;
- CComQIPtr<IHTMLDocument2> html_document_2(dispatch);
- if (!html_document_2) return E_FAIL;
- hr = html_document_2->get_parentWindow(&html_window_2_);
- if (FAILED(hr)) return hr;
- }
hr = html_window_2_->get_event(&html_event_obj);
if (FAILED(hr)) return hr;
CComQIPtr<IHTMLEventObj2> html_event_obj_2(html_event_obj);
@@ -151,7 +113,6 @@
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
if (FAILED(hr)) return hr;
-
CComPtr<IServiceProvider> service_provider;
hr = html_data_transfer->QueryInterface(&service_provider);
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.h#2 -
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-10-20 18:28:31.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2008-10-20 19:11:54.000000000 +1100
@@ -32,31 +32,43 @@
// The Drag-and-Drop API is not implemented on Windows CE.
#else
+#include <mshtmdid.h>
#include <windows.h>
+
#include "gears/base/common/base_class.h"
+#include "gears/base/common/js_dom_element.h"
#include "gears/base/common/js_runner.h"
#include "gears/base/common/js_types.h"
#include "gears/base/common/scoped_refptr.h"
#include "third_party/scoped_ptr/scoped_ptr.h"
-class ATL_NO_VTABLE DropTarget
- : public CComObjectRootEx<CComMultiThreadModel>,
- public CComCoClass<DropTarget>,
- public IDispatchImpl<IDispatch>,
- public IElementBehavior,
- public IElementBehaviorFactory,
+// The 1 in the IDispEventSimpleImpl line is just an arbitrary positive
+// integer, as says the IDispEventSimpleImpl documentation at
+// http://msdn.microsoft.com/en-us/library/fwy24613(VS.80).aspx
+// This 1 is the same nID as used in the SINK_ENTRY_INFO calls below.
+class DropTarget
+ : public IDispEventSimpleImpl<1, DropTarget, &DIID_HTMLElementEvents2>,
public JsEventHandlerInterface {
public:
- DECLARE_NOT_AGGREGATABLE(DropTarget)
- DECLARE_PROTECT_FINAL_CONSTRUCT()
+ BEGIN_SINK_MAP(DropTarget)
+ SINK_ENTRY_INFO(1, DIID_HTMLElementEvents2,
+ DISPID_HTMLELEMENTEVENTS_ONDRAGENTER,
+ &HandleOnDragEnter,
+ &atl_func_info_)
+ SINK_ENTRY_INFO(1, DIID_HTMLElementEvents2,
+ DISPID_HTMLELEMENTEVENTS_ONDRAGOVER,
+ &HandleOnDragOver,
+ &atl_func_info_)
+ SINK_ENTRY_INFO(1, DIID_HTMLElementEvents2,
+ DISPID_HTMLELEMENTEVENTS_ONDRAGLEAVE,
+ &HandleOnDragLeave,
+ &atl_func_info_)
+ SINK_ENTRY_INFO(1, DIID_HTMLElementEvents2,
+ DISPID_HTMLELEMENTEVENTS_ONDROP,
+ &HandleOnDragDrop,
+ &atl_func_info_)
+ END_SINK_MAP()
- BEGIN_COM_MAP(DropTarget)
- COM_INTERFACE_ENTRY(IDispatch)
- COM_INTERFACE_ENTRY(IElementBehavior)
- COM_INTERFACE_ENTRY(IElementBehaviorFactory)
- END_COM_MAP()
-
- CComPtr<IElementBehaviorSite> element_behavior_site_;
CComPtr<IHTMLWindow2> html_window_2_;
scoped_refptr<ModuleEnvironment> module_environment_;
@@ -66,36 +78,27 @@
scoped_ptr<JsRootedCallback> on_drag_leave_;
scoped_ptr<JsRootedCallback> on_drop_;
- DropTarget() {}
-
- // IDispatch interface.
- STDMETHOD(Invoke)(DISPID dispidMember, REFIID riid,
- LCID lcid, WORD wFlags, DISPPARAMS *pdispparams, VARIANT *pvarResult,
- EXCEPINFO *pexcepinfo, UINT *puArgErr);
-
- // IElementBehavior interface.
- STDMETHOD(Detach)(void);
- STDMETHOD(Init)(IElementBehaviorSite *pElementBehaviorSite);
- STDMETHOD(Notify)(long lEvent, VARIANT *pVar);
-
- // IElementBehaviorFactory interface.
- STDMETHOD(FindBehavior)(BSTR name, BSTR url,
- IElementBehaviorSite *behavior_site,
- IElementBehavior **behavior_out);
+ virtual ~DropTarget();
+ static DropTarget *CreateDropTarget(JsDomElement &dom_element);
HRESULT GetHtmlDataTransfer(CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer);
HRESULT CancelEventBubble(CComPtr<IHTMLEventObj> &html_event_obj,
CComPtr<IHTMLDataTransfer> &html_data_transfer);
- HRESULT HandleOnDragEnter();
- HRESULT HandleOnDragOver();
- HRESULT HandleOnDragLeave();
- HRESULT HandleOnDragDrop();
+ STDMETHOD(HandleOnDragEnter)();
+ STDMETHOD(HandleOnDragOver)();
+ STDMETHOD(HandleOnDragLeave)();
+ STDMETHOD(HandleOnDragDrop)();
virtual void HandleEvent(JsEventType event_type);
private:
+ DropTarget();
+
+ CComPtr<IDispatch> event_source_for_disp_event_unadvise_;
+
+ static _ATL_FUNC_INFO atl_func_info_;
DISALLOW_EVIL_CONSTRUCTORS(DropTarget);
};
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#1 -
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-10-21 13:42:37.000000000 +1100
+++ googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
2008-10-21 13:43:11.000000000 +1100
@@ -20,8 +20,10 @@
var dragLeaveCount = 0;
var dropCount = 0;
var eventCount = 0;
+
+var dropZone = document.getElementById('dropZone');
var desktop = google.gears.factory.create('beta.desktop');
-desktop.registerDropTarget(document.getElementById('dropZone'), {
+desktop.registerDropTarget(dropZone, {
'ondragenter': function() {
dragEnterCount++;
eventCount++;
@@ -56,6 +58,17 @@
document.getElementById('dropOutput').innerHTML = s;
}
});
+
+var dropZoneIdIsGood = false;
+try {
+ if (dropZone.id == 'dropZone') {
+ dropZoneIdIsGood = true;
+ }
+} catch (ex) {
+}
+if (!dropZoneIdIsGood) {
+ alert('dropZone.id is broken.');
+}
</script>
</body>
</html>
