Bug 98277 – [chromium] Allow dragging into plugins.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sadrul Habib Chowdhury]

[chromium] Allow dragging into plugins.

[Sadrul Habib Chowdhury]

Created attachment 166912 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 166912 [details]
Patch

Attachment 166912 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14132700

[Peter Beverloo (cr-android ews)]

Comment on attachment 166912 [details]
Patch

Attachment 166912 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14138630

[Sadrul Habib Chowdhury]

Created attachment 166916 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166916 [details]
Patch

Attachment 166916 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14131735

[Peter Beverloo (cr-android ews)]

Comment on attachment 166916 [details]
Patch

Attachment 166916 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14126846

[Sadrul Habib Chowdhury]

(In reply to comment #7)
> (From update of attachment 166916 [details])
> Attachment 166916 [details] did not pass cr-android-ews (chromium-android):
> Output: http://queues.webkit.org/results/14126846

Hi! The chromium side of this CL to fix the build is at: http://codereview.chromium.org/11043022/. But I wanted to get this WebKit side reviewed first, so that I can update the chrome-CL before landing. So please take a look at this patch despite the build failures :)

[Adam Barth]

Comment on attachment 166916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166916&action=review

Seems reasonable to me, but I'm not a drag-and-drop expert.  API change LGTM.

> Source/WebKit/chromium/public/WebDragState.h:40
> +    WebDragStateUnknown  = 0,
> +    WebDragStateEnter    = 1,
> +    WebDragStateOver     = 2,
> +    WebDragStateLeave    = 3

The = sign should only be separated from the enum name by a single space.

[Robert Kroeger]

Comment on attachment 166916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166916&action=review

> Source/WebKit/chromium/public/WebDragState.h:31
> +#ifndef WebDragState_h

There is an existing DragState in webcore. the typical convention is that a WebFoo wraps a WebCore::Foo. at the risk of bike-shedding, this might need to be renamed? Couldn't you also suck in

> Source/WebKit/chromium/public/WebPlugin.h:86
> +    virtual bool handleDragEvent(WebDragState, const WebDragData&, WebDragOperationsMask, const WebPoint& position, const WebPoint& screenPosition) = 0;

we don't have drag events right? So maybe a different name?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:712
> +        dragState = WebDragStateEnter;

why do we need to track the drag state here? why not pass the eventName() to the handleDragEvent and wrap that in a Web interface?

[Sadrul Habib Chowdhury]

Created attachment 166933 [details]
Patch

[Sadrul Habib Chowdhury]

Created attachment 166934 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 166916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166916&action=review

>> Source/WebKit/chromium/public/WebDragState.h:31
>> +#ifndef WebDragState_h
> 
> There is an existing DragState in webcore. the typical convention is that a WebFoo wraps a WebCore::Foo. at the risk of bike-shedding, this might need to be renamed? Couldn't you also suck in

Renamed this to WebDragStatus.

>> Source/WebKit/chromium/public/WebDragState.h:40
>> +    WebDragStateLeave    = 3
> 
> The = sign should only be separated from the enum name by a single space.

Fixed.

>> Source/WebKit/chromium/public/WebPlugin.h:86
>> +    virtual bool handleDragEvent(WebDragState, const WebDragData&, WebDragOperationsMask, const WebPoint& position, const WebPoint& screenPosition) = 0;
> 
> we don't have drag events right? So maybe a different name?

Ranamed WebDragState to WebDragStatus, and handleDragEvent to handleDragStatusUpdate.

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:712
>> +        dragState = WebDragStateEnter;
> 
> why do we need to track the drag state here? why not pass the eventName() to the handleDragEvent and wrap that in a Web interface?

The handleDragEvent implementations do not have access to eventNames() (they are usually in chrome land). Adding a wrapper for all of eventNames() is probably an overkill. The current way is to convert these into some enum (usually something in WebInputEvent)

[Sadrul Habib Chowdhury]

Comment on attachment 166934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166934&action=review

> Source/WebKit/chromium/public/WebPlugin.h:87
> +

To allow less churn around this, would it be acceptable to have a default implementation here for now, and remove the implementation after the chrome implementations land? I was planning on landing the chrome stub-implementations first, but it'd probably be easier if this lands first with the default implementation, then the chrome CL lands, and then I remove the default implementation form here.

[Peter Beverloo (cr-android ews)]

Comment on attachment 166934 [details]
Patch

Attachment 166934 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14137689

[WebKit Review Bot]

Comment on attachment 166934 [details]
Patch

Attachment 166934 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14138673

[Sadrul Habib Chowdhury]

Created attachment 166946 [details]
Patch

[Sadrul Habib Chowdhury]

(In reply to comment #14)
> (From update of attachment 166934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166934&action=review
> 
> > Source/WebKit/chromium/public/WebPlugin.h:87
> > +
> 
> To allow less churn around this, would it be acceptable to have a default implementation here for now, and remove the implementation after the chrome implementations land? I was planning on landing the chrome stub-implementations first, but it'd probably be easier if this lands first with the default implementation, then the chrome CL lands, and then I remove the default implementation form here.

I have gone ahead with the default implementation here (looks like there are other default implementations here (e.g. container(), supportsKeyboardFocus() etc.). The EWS bots are all super happy. PTAL.

[Sadrul Habib Chowdhury]

Hi Tony! Would you mind reviewing this change? (Adding you since I understand you have reviewed drag-n-drop related changes before). Thanks!

[Tony Chang]

Comment on attachment 166946 [details]
Patch

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.

[Sadrul Habib Chowdhury]

Comment on attachment 166946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166946&action=review

>> Source/WebKit/chromium/public/WebDragStatus.h:40
>> +    WebDragStatusLeave = 3
> 
> Do we have to explicitly number these?

I looked at some of the other places where enums are used in chromium/public/, and it looks like only the first entry is explicitly given a value. So I have followed the same style here, and removed the explicit numbers from the rest of the entries.

>> 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.

It looks like none of the plugin-tests that deal with events are testable manually (since the test-expectation is actually the stdout output). So I have kept this unchanged.

>> LayoutTests/platform/chromium/plugins/drag-events.html:36
>> +        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.

Done. Thanks!

>> LayoutTests/platform/chromium/plugins/drag-events.html:41
>> +        testRunner.notifyDone();
> 
> Do we need the notifyDone? You don't actually call waitUntilDone anywhere.

Whoops. Removed.

[Sadrul Habib Chowdhury]

Created attachment 167172 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 167172 [details]
Patch

Thanks for the quick review!

[Sadrul Habib Chowdhury]

Comment on attachment 166946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166946&action=review

>> 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.

Oh, I didn't address this comment. My plan is that the return value will indicate whether the plugin is willing to accept the drop, in which case, the mouse-cursor will change accordingly.

[Tony Chang]

(In reply to comment #21)
> (From update of attachment 166946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166946&action=review
> 
> >> 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.
> 
> It looks like none of the plugin-tests that deal with events are testable manually (since the test-expectation is actually the stdout output). So I have kept this unchanged.

If you manually ran the steps in Chromium, wouldn't you still see the results on stdout?  I thought NPAPI plugin processes weren't sandboxed.

[Sadrul Habib Chowdhury]

(In reply to comment #25)
> (In reply to comment #21)
> > (From update of attachment 166946 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166946&action=review
> > 
> > >> 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.
> > 
> > It looks like none of the plugin-tests that deal with events are testable manually (since the test-expectation is actually the stdout output). So I have kept this unchanged.
> 
> If you manually ran the steps in Chromium, wouldn't you still see the results on stdout?  I thought NPAPI plugin processes weren't sandboxed.

My understanding is that this test plugin (TestWebPlugin) is not an NPAPI plugin.

[Tony Chang]

(In reply to comment #26)
> (In reply to comment #25)
> > If you manually ran the steps in Chromium, wouldn't you still see the results on stdout?  I thought NPAPI plugin processes weren't sandboxed.
> 
> My understanding is that this test plugin (TestWebPlugin) is not an NPAPI plugin.

Ah, you're right. It looks like the plugin is compiled directly into DumpRenderTree.

[Sadrul Habib Chowdhury]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > If you manually ran the steps in Chromium, wouldn't you still see the results on stdout?  I thought NPAPI plugin processes weren't sandboxed.
> > 
> > My understanding is that this test plugin (TestWebPlugin) is not an NPAPI plugin.
> 
> Ah, you're right. It looks like the plugin is compiled directly into DumpRenderTree.

Cool! Thanks for the really quick reviews.

Mind helping with the r?/cq? flags too? :)

[Tony Chang]

(In reply to comment #28)

> Mind helping with the r?/cq? flags too? :)

If you use webkit-patch and you already have an r+, you can use 'webkit-patch land-safely' to upload a new patch that sets the cq flag appropriately (cq+ if you're a committer, cq? if you're not).

[WebKit Review Bot]

Comment on attachment 167172 [details]
Patch

Rejecting attachment 167172 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
top rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9

If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14168524

[Sadrul Habib Chowdhury]

(In reply to comment #29)
> (In reply to comment #28)
> 
> > Mind helping with the r?/cq? flags too? :)
> 
> If you use webkit-patch and you already have an r+, you can use 'webkit-patch land-safely' to upload a new patch that sets the cq flag appropriately (cq+ if you're a committer, cq? if you're not).

Oh, cool! That's useful. Thanks!

[Sadrul Habib Chowdhury]

Created attachment 167219 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167219 [details]
Patch for landing

Clearing flags on attachment: 167219

Committed r130504: <http://trac.webkit.org/changeset/130504>