[webkit-reviews] review granted: [Bug 98277] [chromium] Allow dragging into plugins. : [Attachment 166946] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted sadrul at chromium.org's request for
review:
Bug 98277: [chromium] Allow dragging into plugins.
https://bugs.webkit.org/show_bug.cgi?id=98277

Attachment 166946: Patch
https://bugs.webkit.org/attachment.cgi?id=166946&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166946&action=review


Looks fine overall.

> Source/WebKit/chromium/public/WebDragStatus.h:40
> +    WebDragStatusUnknown = 0,
> +    WebDragStatusEnter = 1,
> +    WebDragStatusOver = 2,
> +    WebDragStatusLeave = 3

Do we have to explicitly number these?

> Source/WebKit/chromium/public/WebPlugin.h:86
> +    virtual bool handleDragStatusUpdate(WebDragStatus, const WebDragData&,
WebDragOperationsMask, const WebPoint& position, const WebPoint&
screenPosition) { return false; }

What is the purpose of the return value?  In WebPluginContainerImpl.cpp, we
ignore the return value.

> LayoutTests/platform/chromium/plugins/drag-events.html:18
> +	   document.write("This test does not work in manual mode.");

Normally we write instructions on how to run the test manually.  E.g., drag the
select to the plugin; it should output blah.  This allows someone to run the
test in Chromium as well.

> LayoutTests/platform/chromium/plugins/drag-events.html:36
> +	   eventSender.mouseDown();
> +	   eventSender.mouseMoveTo(positionX, positionY);

This might not work on Chromium Mac since you have to wait 250ms after a
mouseDown before it's considered a drag.  You can use
eventSender.leapForward(250) to get this working on Mac.

> LayoutTests/platform/chromium/plugins/drag-events.html:41
> +	   testRunner.notifyDone();

Do we need the notifyDone? You don't actually call waitUntilDone anywhere.