Hello aa,
I'd like you to do a code review. Please execute
g4 diff -c 8685912
or point your web browser to
http://mondrian/8685912
to review the following code:
Change 8685912 by [EMAIL PROTECTED] on 2008/10/22 14:58:37 *pending*
Provide optional visual feedback for debugging drag-and-drop.
PRESUBMIT=passed
R=aa
[EMAIL PROTECTED]
DELTA=126 (78 added, 9 deleted, 39 changed)
OCL=8685912
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h#4
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#5 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#2 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
126 delta lines: 78 added, 9 deleted, 39 changed
Also consider running:
g4 lint -c 8685912
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 8685912 by [EMAIL PROTECTED] on 2008/10/22 14:58:37 *pending*
Provide optional visual feedback for debugging drag-and-drop.
OCL=8685912
Affected files ...
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h#4
edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#5 edit
... //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#2 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
-
/home/nigeltao/srcgears3/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-22 11:31:55.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.cc
2008-10-22 13:42:01.000000000 +1100
@@ -48,10 +48,10 @@
#if BROWSER_FF || (BROWSER_IE && !defined(OS_WINCE))
static bool InitializeCallback(const std::string16 name,
- JsObject *js_callbacks,
+ JsObject *options,
scoped_ptr<JsRootedCallback> *scoped_callback,
std::string16 *error_out) {
- JsParamType property_type = js_callbacks->GetPropertyType(name);
+ JsParamType property_type = options->GetPropertyType(name);
if (property_type != JSPARAM_UNDEFINED &&
property_type != JSPARAM_FUNCTION) {
*error_out = STRING16(L"options.");
@@ -60,7 +60,7 @@
return false;
}
JsRootedCallback *callback;
- if (js_callbacks->GetPropertyAsFunction(name, &callback)) {
+ if (options->GetPropertyAsFunction(name, &callback)) {
scoped_callback->reset(callback);
}
return true;
@@ -68,21 +68,25 @@
static bool InitializeDropTarget(ModuleImplBaseClass *sibling_module,
- JsObject *js_callbacks,
+ JsObject *options,
DropTarget *drop_target,
std::string16 *error_out) {
sibling_module->GetModuleEnvironment(&drop_target->module_environment_);
+#ifdef DEBUG
+ drop_target->is_debugging_ =
+ JSPARAM_UNDEFINED != options->GetPropertyType(STRING16(L"debug"));
+#endif
drop_target->unload_monitor_.reset(
new JsEventMonitor(sibling_module->GetJsRunner(),
JSEVENT_UNLOAD,
drop_target));
- return InitializeCallback(STRING16(L"ondragenter"), js_callbacks,
+ return InitializeCallback(STRING16(L"ondragenter"), options,
&drop_target->on_drag_enter_, error_out) &&
- InitializeCallback(STRING16(L"ondragover"), js_callbacks,
+ InitializeCallback(STRING16(L"ondragover"), options,
&drop_target->on_drag_over_, error_out) &&
- InitializeCallback(STRING16(L"ondragleave"), js_callbacks,
+ InitializeCallback(STRING16(L"ondragleave"), options,
&drop_target->on_drag_leave_, error_out) &&
- InitializeCallback(STRING16(L"ondrop"), js_callbacks,
+ InitializeCallback(STRING16(L"ondrop"), options,
&drop_target->on_drop_, error_out);
}
#endif
@@ -91,24 +95,16 @@
DropTarget *DragAndDropRegistry::RegisterDropTarget(
ModuleImplBaseClass *sibling_module,
JsDomElement &dom_element,
- JsObject *js_callbacks,
+ JsObject *options,
std::string16 *error_out) {
#if BROWSER_FF
- nsCOMPtr<nsIDOMEventTarget> event_target =
- do_QueryInterface(dom_element.dom_html_element());
- if (!event_target) {
- *error_out = STRING16(L"Argument must be a DOMEventTarget.");
- return NULL;
- }
-
DropTarget *drop_target = new DropTarget;
- if (!InitializeDropTarget(
- sibling_module, js_callbacks, drop_target, error_out)) {
+ if (!InitializeDropTarget(sibling_module, options, drop_target, error_out)) {
delete drop_target;
return NULL;
}
- drop_target->AddSelfAsEventListeners(event_target);
+ drop_target->SetDomElement(dom_element);
return drop_target;
#elif BROWSER_IE && !defined(OS_WINCE)
@@ -122,8 +118,7 @@
}
CComPtr<CComObject<DropTarget> >
reference_adder_drop_target(drop_target);
- if (!InitializeDropTarget(
- sibling_module, js_callbacks, drop_target, error_out)) {
+ if (!InitializeDropTarget(sibling_module, options, drop_target, error_out)) {
return NULL;
}
====
//depot/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h#4
-
/home/nigeltao/srcgears3/googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h
2008-10-22 11:31:55.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drag_and_drop_registry.h
2008-10-22 13:17:54.000000000 +1100
@@ -42,7 +42,7 @@
public:
static DropTarget *RegisterDropTarget(ModuleImplBaseClass *sibling_module,
JsDomElement &dom_element,
- JsObject *js_callbacks,
+ JsObject *options,
std::string16 *error_out);
private:
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc#5 -
/home/nigeltao/srcgears3/googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-10-21 15:23:41.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.cc
2008-10-22 16:45:44.000000000 +1100
@@ -54,13 +54,42 @@
NS_IMPL_ISUPPORTS1(DropTarget, nsIDOMEventListener)
-DropTarget::DropTarget() {
+DropTarget::DropTarget()
+ : is_debugging_(false) {
LEAK_COUNTER_INCREMENT(DropTarget);
}
DropTarget::~DropTarget() {
LEAK_COUNTER_DECREMENT(DropTarget);
+}
+
+
+void DropTarget::ProvideDebugVisualFeedback(bool is_drag_enter) {
+#ifdef DEBUG
+ if (!is_debugging_) {
+ return;
+ }
+
+ nsCOMPtr<nsIDOMElement> element(do_QueryInterface(html_element_));
+ if (element) {
+ static const nsString kShow(STRING16(L"border:green 3px solid;"));
+ static const nsString kHide(STRING16(L"border:white 3px solid;"));
+ static const nsString kStyle(STRING16(L"style"));
+ nsString current_style;
+ element->GetAttribute(kStyle, current_style);
+ current_style.Append(is_drag_enter ? kShow : kHide);
+ element->SetAttribute(kStyle, current_style);
+ }
+#endif
+}
+
+
+void DropTarget::SetDomElement(JsDomElement &dom_element) {
+ html_element_ = dom_element.dom_html_element();
+ nsCOMPtr<nsIDOMEventTarget> event_target(do_QueryInterface(html_element_));
+ AddSelfAsEventListeners(event_target);
+ ProvideDebugVisualFeedback(false);
}
@@ -68,33 +97,37 @@
event_target_ = event_target;
AddRef(); // Balanced by a Release() call during RemoveSelfAsEventListeners.
- if (on_drag_enter_.get()) {
- event_target_->AddEventListener(kDragEnterAsString, this, false);
- }
- if (on_drag_over_.get()) {
- event_target_->AddEventListener(kDragOverAsString, this, false);
- }
- if (on_drag_leave_.get()) {
- event_target_->AddEventListener(kDragExitAsString, this, false);
- }
- if (on_drop_.get()) {
- event_target_->AddEventListener(kDragDropAsString, this, false);
+ if (event_target_) {
+ if (on_drag_enter_.get()) {
+ event_target_->AddEventListener(kDragEnterAsString, this, false);
+ }
+ if (on_drag_over_.get()) {
+ event_target_->AddEventListener(kDragOverAsString, this, false);
+ }
+ if (on_drag_leave_.get()) {
+ event_target_->AddEventListener(kDragExitAsString, this, false);
+ }
+ if (on_drop_.get()) {
+ event_target_->AddEventListener(kDragDropAsString, this, false);
+ }
}
}
void DropTarget::RemoveSelfAsEventListeners() {
- if (on_drag_enter_.get()) {
- event_target_->RemoveEventListener(kDragEnterAsString, this, false);
- }
- if (on_drag_over_.get()) {
- event_target_->RemoveEventListener(kDragOverAsString, this, false);
- }
- if (on_drag_leave_.get()) {
- event_target_->RemoveEventListener(kDragExitAsString, this, false);
- }
- if (on_drop_.get()) {
- event_target_->RemoveEventListener(kDragDropAsString, this, false);
+ if (event_target_) {
+ if (on_drag_enter_.get()) {
+ event_target_->RemoveEventListener(kDragEnterAsString, this, false);
+ }
+ if (on_drag_over_.get()) {
+ event_target_->RemoveEventListener(kDragOverAsString, this, false);
+ }
+ if (on_drag_leave_.get()) {
+ event_target_->RemoveEventListener(kDragExitAsString, this, false);
+ }
+ if (on_drop_.get()) {
+ event_target_->RemoveEventListener(kDragDropAsString, this, false);
+ }
}
Release(); // Balanced by an AddRef() call during AddSelfAsEventListeners.
@@ -115,6 +148,7 @@
event->GetType(event_type);
if (on_drop_.get() && event_type.Equals(kDragDropAsString)) {
+ ProvideDebugVisualFeedback(false);
std::string16 error;
scoped_ptr<JsArray> file_objects(
module_environment_->js_runner_->NewArray());
@@ -140,10 +174,12 @@
} else {
JsRootedCallback *callback = NULL;
if (on_drag_enter_.get() && event_type.Equals(kDragEnterAsString)) {
+ ProvideDebugVisualFeedback(true);
callback = on_drag_enter_.get();
} else if (on_drag_over_.get() && event_type.Equals(kDragOverAsString)) {
callback = on_drag_over_.get();
} else if (on_drag_leave_.get() && event_type.Equals(kDragExitAsString)) {
+ ProvideDebugVisualFeedback(false);
callback = on_drag_leave_.get();
}
if (callback) {
@@ -206,7 +242,7 @@
// to a file path. On Windows and Mac, we get a nsIFile out of the
// transferable and query it directly for its file path.
#if defined(LINUX) && !defined(OS_MACOSX)
- nsCOMPtr<nsISupportsString> data_as_xpcom_string = do_QueryInterface(data);
+ nsCOMPtr<nsISupportsString> data_as_xpcom_string(do_QueryInterface(data));
nsString data_as_string;
data_as_xpcom_string->GetData(data_as_string);
@@ -217,7 +253,7 @@
nr = io_service->NewURI(data_as_cstring, NULL, NULL, getter_AddRefs(uri));
if (NS_FAILED(nr)) { return false; }
- nsCOMPtr<nsIFileURL> file_url = do_QueryInterface(uri);
+ nsCOMPtr<nsIFileURL> file_url(do_QueryInterface(uri));
if (!file_url) { return false; }
nsCOMPtr<nsIFile> file;
nr = file_url->GetFile(getter_AddRefs(file));
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ff.h#2 -
/home/nigeltao/srcgears3/googleclient/gears/opensource/gears/desktop/drop_target_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-10-20 12:30:55.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ff.h
2008-10-22 13:48:21.000000000 +1100
@@ -32,6 +32,7 @@
#include <gecko_internal/nsIDragService.h>
#include <gecko_sdk/include/nsIDOMEventListener.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"
@@ -54,6 +55,7 @@
DropTarget();
virtual ~DropTarget();
+ void SetDomElement(JsDomElement &dom_element);
void AddSelfAsEventListeners(nsIDOMEventTarget *event_target);
void RemoveSelfAsEventListeners();
@@ -66,13 +68,20 @@
JsArray *files_out,
std::string16 *error_out);
+#ifdef DEBUG
+ bool is_debugging_;
+#endif
+
private:
static const nsString kDragEnterAsString;
static const nsString kDragOverAsString;
static const nsString kDragExitAsString;
static const nsString kDragDropAsString;
+ nsCOMPtr<nsIDOMHTMLElement> html_element_;
nsCOMPtr<nsIDOMEventTarget> event_target_;
+
+ void ProvideDebugVisualFeedback(bool is_drag_enter);
DISALLOW_EVIL_CONSTRUCTORS(DropTarget);
};
==== //depot/googleclient/gears/opensource/gears/desktop/drop_target_ie.cc#3 -
/home/nigeltao/srcgears3/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 12:30:55.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.cc
2008-10-22 16:45:14.000000000 +1100
@@ -39,6 +39,21 @@
#include "gears/desktop/file_dialog.h"
+void DropTarget::ProvideDebugVisualFeedback(bool is_drag_enter) {
+#ifdef DEBUG
+ if (!is_debugging_) {
+ return;
+ }
+
+ // TODO(nigeltao): Implement, once ChangeList 8653382 "Make IE's DropTarget
+ // implementation use IDispEventSimpleImpl instead of IDispatchImpl" is
+ // submitted. It will probably look something like
+ // html_style_->put_border(CComBSTR(
+ // is_drag_enter ? L"green 3px solid" : L"white 3px solid"));
+#endif
+}
+
+
HRESULT DropTarget::Detach(void) {
return S_OK;
}
@@ -147,6 +162,7 @@
HRESULT DropTarget::HandleOnDragEnter()
{
+ ProvideDebugVisualFeedback(true);
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
@@ -205,6 +221,7 @@
HRESULT DropTarget::HandleOnDragLeave()
{
+ ProvideDebugVisualFeedback(false);
CComPtr<IHTMLEventObj> html_event_obj;
CComPtr<IHTMLDataTransfer> html_data_transfer;
HRESULT hr = GetHtmlDataTransfer(html_event_obj, html_data_transfer);
@@ -223,6 +240,7 @@
HRESULT DropTarget::HandleOnDragDrop()
{
+ ProvideDebugVisualFeedback(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#2 -
/home/nigeltao/srcgears3/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 12:30:55.000000000 +1100
+++ googleclient/gears/opensource/gears/desktop/drop_target_ie.h
2008-10-22 16:43:12.000000000 +1100
@@ -66,7 +66,7 @@
scoped_ptr<JsRootedCallback> on_drag_leave_;
scoped_ptr<JsRootedCallback> on_drop_;
- DropTarget() {}
+ DropTarget() : is_debugging_(false) {}
// IDispatch interface.
STDMETHOD(Invoke)(DISPID dispidMember, REFIID riid,
@@ -95,7 +95,17 @@
virtual void HandleEvent(JsEventType event_type);
+#ifdef DEBUG
+ bool is_debugging_;
+#endif
+
private:
+ void ProvideDebugVisualFeedback(bool is_drag_enter);
+
+#ifdef DEBUG
+ CComPtr<IHTMLStyle> html_style_;
+#endif
+
DISALLOW_EVIL_CONSTRUCTORS(DropTarget);
};
====
//depot/googleclient/gears/opensource/gears/test/manual/drag_and_drop.html#1 -
/home/nigeltao/srcgears3/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 15:23:41.000000000 +1100
+++ googleclient/gears/opensource/gears/test/manual/drag_and_drop.html
2008-10-22 16:50:41.000000000 +1100
@@ -22,6 +22,7 @@
var eventCount = 0;
var desktop = google.gears.factory.create('beta.desktop');
desktop.registerDropTarget(document.getElementById('dropZone'), {
+ 'debug': true,
'ondragenter': function() {
dragEnterCount++;
eventCount++;
