plugins: Allow a plugin to dictate whether it can process drag or not

• Component: New Bugs
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

plugins: Allow a plugin to dictate whether it can process drag or not

[Sadrul Habib Chowdhury]

Created attachment 168763 [details]
Patch

[Sadrul Habib Chowdhury]

Hi! I would love to add a test for this that works for all ports. However, it doesn't look like there is a way of testing PluginViewBase interface? So I have just updated the chromium test.

Please add additional reviewers as you see fit. Thanks!

[Tony Chang]

Comment on attachment 168763 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Need the bug URL (OOPS!).

Need the bug URL (other ChangeLog files too).

> Source/WebCore/page/DragController.cpp:557
> +    if (result.innerNonSharedNode()->isPluginElement()) {
> +        HTMLPlugInElement* plugin = static_cast<HTMLPlugInElement*>(result.innerNonSharedNode());

It seems a bit weird that DragController needs to handle plugin elements specially.  I guess DragController::operationForLoad also special cases plugins, so maybe that's not a big deal.

> Source/WebCore/page/DragController.cpp:560
> +        if (!plugin->canProcessDrag(*dragData))
> +            return false;
> +    } else if (!result.innerNonSharedNode()->rendererIsEditable())

Doesn't this change the default behavior? If canProcessDrag returns the default value, we'll never get to check the contentEditable case.  I.e., pages that currently have contentEditable on an <embed> will no longer process drags.

> LayoutTests/platform/chromium/plugins/drag-events.html:12
> -<embed id="plugin" type="application/x-webkit-test-webplugin" contentEditable></embed>
> +<embed id="plugin" type="application/x-webkit-test-webplugin"></embed>

This looks like we're replacing an old test with a new test.  Can we test both (editable + !canProcessDrag and !editable + canProcessDrag)?

[Sadrul Habib Chowdhury]

Created attachment 168783 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 168763 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        Need the bug URL (OOPS!).
> 
> Need the bug URL (other ChangeLog files too).

Whoops. Done. (sorry, still getting used to webkit-patch)

>> Source/WebCore/page/DragController.cpp:560
>> +    } else if (!result.innerNonSharedNode()->rendererIsEditable())
> 
> Doesn't this change the default behavior? If canProcessDrag returns the default value, we'll never get to check the contentEditable case.  I.e., pages that currently have contentEditable on an <embed> will no longer process drags.

Fixed.

>> LayoutTests/platform/chromium/plugins/drag-events.html:12
>> +<embed id="plugin" type="application/x-webkit-test-webplugin"></embed>
> 
> This looks like we're replacing an old test with a new test.  Can we test both (editable + !canProcessDrag and !editable + canProcessDrag)?

I have updated the test for this.

Note that chromium plugins never used to receive drag events at all until very recently (~last week), even when marked contentEditable. This goes back to that behaviour, and this is unlikely to break any existing plugins.

[Tony Chang]

Comment on attachment 168783 [details]
Patch

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

> Note that chromium plugins never used to receive drag events at all until very recently (~last week), even when marked contentEditable. This goes back to that behaviour, and this is unlikely to break any existing plugins.

I see, the same is true of Safari?  I.e., we're going back to the previous behavior of plugins not receiving drag events, even if contentEditable.

> Source/WebCore/html/HTMLPlugInElement.cpp:78
> +bool HTMLPlugInElement::canProcessDrag(const DragData& dragData) const
> +{
> +    const PluginViewBase* plugin = pluginWidget() && pluginWidget()->isPluginViewBase() ? static_cast<const PluginViewBase*>(pluginWidget()) : 0;
> +    return plugin ? plugin->canProcessDrag(dragData) : false;

Why do we pass in dragData here?  It doesn't look like we ever use it.

> Source/WebCore/page/DragController.cpp:559
> +        if (!plugin->canProcessDrag(*dragData) && !result.innerNonSharedNode()->rendererIsEditable())
> +            return false;

From your description above, it sounds like we want to remove the rendererIsEditable() check here, right?  That is, contentEditable shouldn't influence whether or not drag events are sent to the plugin.

> LayoutTests/platform/chromium/plugins/drag-events.html:27
> +    var positionX = plugin.offsetLeft + 10;
> +    var positionY = plugin.offsetTop + 10;

Can we target the middle of the plugin rather than hard code the offset of 10?

[Sadrul Habib Chowdhury]

Created attachment 168790 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 168783 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInElement.cpp:78
>> +    return plugin ? plugin->canProcessDrag(dragData) : false;
> 
> Why do we pass in dragData here?  It doesn't look like we ever use it.

I was trying to future-proof this in case a plugin might want to conditionally accept/reject drags. But there's no use for it now. I have removed it.

>> Source/WebCore/page/DragController.cpp:559
>> +            return false;
> 
> From your description above, it sounds like we want to remove the rendererIsEditable() check here, right?  That is, contentEditable shouldn't influence whether or not drag events are sent to the plugin.

contentEditable plugins did not receive drag events in chromium. However, this may not have been the case for other ports. So with this change, drag interaction for such plugins remains the same on these ports. For chromium-port, a plugin can now choose to accept drag events.

>> LayoutTests/platform/chromium/plugins/drag-events.html:27
>> +    var positionY = plugin.offsetTop + 10;
> 
> Can we target the middle of the plugin rather than hard code the offset of 10?

Done.

[Eric Seidel (no email)]

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.

[Tony Chang]

This change seems fine to me.  I'm not 100% sure about the modifications to HTMLPluginElement, but one of the API reviewers can verify whether that's OK or not.

[Sadrul Habib Chowdhury]

(In reply to comment #10)
> This change seems fine to me.  I'm not 100% sure about the modifications to HTMLPluginElement, but one of the API reviewers can verify whether that's OK or not.

Thanks for the quick reviews! I will wait for review from API reviewers.

[Maciej Stachowiak]

Is this a change we should be making in other ports too? Is it worthy of a feature announcement maybe? http://www.webkit.org/coding/adding-features.html

[Adam Barth]

Comment on attachment 168790 [details]
Patch

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

> Source/WebKit/chromium/public/WebPlugin.h:73
> +    virtual bool canProcessDrag() const { return false; }

API change LGTM

[Adam Barth]

> Is this a change we should be making in other ports too?

Are other ports interested in elaborating their plugin support?  My understanding is that other ports are only interested in supporting the existing plugin APIs.

> Is it worthy of a feature announcement maybe  http://www.webkit.org/coding/adding-features.html

I wouldn't think so.  This isn't functionality exposed to the web.

[Maciej Stachowiak]

(In reply to comment #14)
> > Is this a change we should be making in other ports too?
> 
> Are other ports interested in elaborating their plugin support?  My understanding is that other ports are only interested in supporting the existing plugin APIs.

I don't actually understand the nature or purpose of this change (the Description is not very explanatory) so it's hard to say.

I had the vague impression that it was already possible to Drag & Drop to and from plugins such as Flash or Java (e.g. demo here <http://www.hellokeita.in/xp/DragDrop/>) so I was wondering if this is a performance optimization for non-drag-supporting plugins, or if my understanding of the state of plugin DnD was wrong.

If it's actually something totally specific to Pepper (which maybe is what you're implying) then it probably would not be of interest to other ports, but that wasn't clear from the bug or the patch.

[Adam Barth]

(In reply to comment #15)
> (In reply to comment #14)
> > > Is this a change we should be making in other ports too?
> > 
> > Are other ports interested in elaborating their plugin support?  My understanding is that other ports are only interested in supporting the existing plugin APIs.
> 
> I don't actually understand the nature or purpose of this change (the Description is not very explanatory) so it's hard to say.

That's a good point.  @sadrul, would you be willing to add more details to the ChangeLog about the nature and purpose of this change?

> I had the vague impression that it was already possible to Drag & Drop to and from plugins such as Flash or Java (e.g. demo here <http://www.hellokeita.in/xp/DragDrop/>) so I was wondering if this is a performance optimization for non-drag-supporting plugins, or if my understanding of the state of plugin DnD was wrong.

I'm not super familiar with NPAPI, but I suspect that these plugins talk directly to the OS to handle drag and drop rather than going through an API that can be properly sandboxed.

> If it's actually something totally specific to Pepper (which maybe is what you're implying) then it probably would not be of interest to other ports, but that wasn't clear from the bug or the patch.

The overarching context here is that sadrul is creating a plugin, called the BrowserPlugin, that re-embeds WebKit as seamlessly as possible.  If you're familiar with Chrome Frame, you can think of it as Chrome Frame for Chrome as opposed to IE.

Part of this work involves elaborating the plugin API to fill in some missing details.  For example, a previous patch improved the focus traversal so that the plugin could participate in focus traversal more like an iframe than a single monolithic blob.

BrowserPlugin happens to be a PPAPI plugin, but it would make sense to add similar enhancements to NPAPI if you were interested in improving NPAPI.  My understanding, however, is that most WebKit contributors have little interest in elaborating NPAPI.

It's possible that we'll expose these APIs to other plugin authors in the future, but at the moment, we're just using them internally in BrowserPlugin.

[Maciej Stachowiak]

(In reply to comment #16)

> 
> > I had the vague impression that it was already possible to Drag & Drop to and from plugins such as Flash or Java (e.g. demo here <http://www.hellokeita.in/xp/DragDrop/>) so I was wondering if this is a performance optimization for non-drag-supporting plugins, or if my understanding of the state of plugin DnD was wrong.
> 
> I'm not super familiar with NPAPI, but I suspect that these plugins talk directly to the OS to handle drag and drop rather than going through an API that can be properly sandboxed.

Good point, that is certainly a possibility.

> 
> > If it's actually something totally specific to Pepper (which maybe is what you're implying) then it probably would not be of interest to other ports, but that wasn't clear from the bug or the patch.
> 
> The overarching context here is that sadrul is creating a plugin, called the BrowserPlugin, that re-embeds WebKit as seamlessly as possible.  If you're familiar with Chrome Frame, you can think of it as Chrome Frame for Chrome as opposed to IE.
> 
> Part of this work involves elaborating the plugin API to fill in some missing details.  For example, a previous patch improved the focus traversal so that the plugin could participate in focus traversal more like an iframe than a single monolithic blob.

Thanks for the context!

> 
> BrowserPlugin happens to be a PPAPI plugin, but it would make sense to add similar enhancements to NPAPI if you were interested in improving NPAPI.  My understanding, however, is that most WebKit contributors have little interest in elaborating NPAPI.
> 
> It's possible that we'll expose these APIs to other plugin authors in the future, but at the moment, we're just using them internally in BrowserPlugin.

It might be interesting for someone interested in NPAPI to propose something to plugin-futures in the future, but given what you say and the fact that plugins seem to be getting by otherwise, it does not seem urgent.

I agree with you that a better ChangeLog entry for this patch would be helpful.

[Sadrul Habib Chowdhury]

> 
> That's a good point.  @sadrul, would you be willing to add more details to the ChangeLog about the nature and purpose of this change?
> 

Sure. I have updated the changelog. Please feel free to suggest amendments (I am not very good with words :) ).

> > I had the vague impression that it was already possible to Drag & Drop to and from plugins such as Flash or Java (e.g. demo here <http://www.hellokeita.in/xp/DragDrop/>) so I was wondering if this is a performance optimization for non-drag-supporting plugins, or if my understanding of the state of plugin DnD was wrong.
> 

Looking at the source of the web-page, it looks like the plugin calls into JS functions in the page to complete the drag, and the JS functions listen for mousemove events, as opposed to drag events.

In any case, this particular change is about being able to drag things from outside the plugin into the plugin. This is currently not supported (without using native system calls, anyway). For example: in http://www.paciellogroup.com/blog/misc/adobe/flash/samples/form.html try to drag some text (from the location-bar in the browser, for example) into one of the textfields into the plugin. You will notice that the cursor indicates that you cannot drop onto the plugin during the drag, and dropping actually doesn't do anything.


> BrowserPlugin happens to be a PPAPI plugin, but it would make sense to add similar enhancements to NPAPI if you were interested in improving NPAPI.  My understanding, however, is that most WebKit contributors have little interest in elaborating NPAPI.
> 

Just a quick correction here: in the current incarnation, BrowserPlugin is neither an NPAPI or a PPAPI plugin. It's a different kind of plugin. However it implements the same WebPlugin interface as PPAPI/NPAPI, so code in WebKit isn't aware of the differences, and it doesn't need to do any special-casing for browser-plugins.

> It's possible that we'll expose these APIs to other plugin authors in the future, but at the moment, we're just using them internally in BrowserPlugin.

[Sadrul Habib Chowdhury]

Created attachment 169191 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169191 [details]
Patch

Clearing flags on attachment: 169191

Committed r131625: <http://trac.webkit.org/changeset/131625>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> Just a quick correction here: in the current incarnation, BrowserPlugin is neither an NPAPI or a PPAPI plugin. It's a different kind of plugin. However it implements the same WebPlugin interface as PPAPI/NPAPI, so code in WebKit isn't aware of the differences, and it doesn't need to do any special-casing for browser-plugins.

Oh, interesting.  I didn't know that.  Thanks for the correction.