Bug 85916 – [Chromium] Expose WebPluginContainer of WebPlugin to embedder

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

[Chromium] Expose WebPluginContainer to WebFrameClient::createPlugin (for Browser Plugin)

[Fady Samuel]

Created attachment 140783 [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.

[James Robinson]

Comment on attachment 140783 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:6
> +
> +        Reviewed by NOBODY (OOPS!).

this needs some explanation

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1462
> +    RefPtr<WebPluginContainerImpl> container =
> +        WebPluginContainerImpl::create(element);

don't wrap this

> Source/WebKit/chromium/src/WebPluginContainerImpl.h:157
> +    WebPluginContainerImpl(WebCore::HTMLPlugInElement*);

explicit

[Fady Samuel]

Created attachment 140788 [details]
Patch

[James Robinson]

That's not really sufficient.  What exactly is the problem you are attempting to solve and how does this patch do that?

[Fady Samuel]

(In reply to comment #5)
> That's not really sufficient.  What exactly is the problem you are attempting to solve and how does this patch do that?

It's a bit outside the scope of this particular patch I think? I'm slowly landing the browser plugin.

On my latest changes, there is a class "BrowserPlugin" that acts as a wrapper for WebPluginContainer. On navigation, depending on the process isolation policy used, an entirely new process may be forked. WebPluginImpl is a wrapper for a channel with a particular process. Since we want to point to a new process, we create a new WebPlugin and set the plugin of WebPluginContainer to the new WebPlugin.

[WebKit Review Bot]

Comment on attachment 140788 [details]
Patch

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

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html
plugins/missing-plugin.html
fast/images/embed-does-not-propagate-dimensions-to-object-ancestor.html
fast/loader/loadInProgress.html
plugins/hidden-iframe-with-swf-plugin.html
plugins/clicking-missing-plugin-fires-delegate.html
compositing/geometry/object-clip-rects-assertion.html
plugins/invalid-mime-with-valid-extension-shows-missing-plugin.html
plugins/embed-attributes-style.html
compositing/geometry/empty-embed-rects.html
http/tests/loading/nested_bad_objects.php
plugins/iframe-shims.html
dom/html/level2/html/AppletsCollection.html

[WebKit Review Bot]

Created attachment 140823 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Fisher (:fishd, Google)]

Comment on attachment 140788 [details]
Patch

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

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1462
> +    WebPlugin* webPlugin = m_webFrame->client()->createPlugin(m_webFrame, container.get(), params);

It's a little strange for the container to exist without an associated WebPlugin.
I don't understand why you need to invert the creation order here.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1468
>      if (!webPlugin->initialize(container.get()))

at the very least, it seems like you should no longer need to pass container
to the initialize method.  it is redundant to both pass it here and in the
constructor for a WebPlugin.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (In reply to comment #5)
> > That's not really sufficient.  What exactly is the problem you are attempting to solve and how does this patch do that?
> 
> It's a bit outside the scope of this particular patch I think? I'm slowly landing the browser plugin.
> 
> On my latest changes, there is a class "BrowserPlugin" that acts as a wrapper for WebPluginContainer. On navigation, depending on the process isolation policy used, an entirely new process may be forked. WebPluginImpl is a wrapper for a channel with a particular process. Since we want to point to a new process, we create a new WebPlugin and set the plugin of WebPluginContainer to the new WebPlugin.

I read this over a couple times, and I'm still confused about why you need this change.  I think we do something similar when implementing blocked plugins.  I believe this is why WebPluginContainer has a setPlugin call.  You can change the WebPlugin instance later by calling that method.  It is not clear that you need any WebKit API changes.

[Fady Samuel]

(In reply to comment #10)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > That's not really sufficient.  What exactly is the problem you are attempting to solve and how does this patch do that?
> > 
> > It's a bit outside the scope of this particular patch I think? I'm slowly landing the browser plugin.
> > 
> > On my latest changes, there is a class "BrowserPlugin" that acts as a wrapper for WebPluginContainer. On navigation, depending on the process isolation policy used, an entirely new process may be forked. WebPluginImpl is a wrapper for a channel with a particular process. Since we want to point to a new process, we create a new WebPlugin and set the plugin of WebPluginContainer to the new WebPlugin.
> 
> I read this over a couple times, and I'm still confused about why you need this change.  I think we do something similar when implementing blocked plugins.  I believe this is why WebPluginContainer has a setPlugin call.  You can change the WebPlugin instance later by calling that method.  It is not clear that you need any WebKit API changes.


The browser plugin is first constructed in RenderViewImpl::createPlugin. It needs to know about the WebPluginContainer on construction so that it can keep the container around in a member variable so that it can point it to a new WebPlugin on cross-process navigation.

[Fady Samuel]

Comment on attachment 140788 [details]
Patch

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

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1462
>> +    WebPlugin* webPlugin = m_webFrame->client()->createPlugin(m_webFrame, container.get(), params);
> 
> It's a little strange for the container to exist without an associated WebPlugin.
> I don't understand why you need to invert the creation order here.

The browser plugin needs to know about the container object so that it can make calls to "setPlugin" outside of WebKit on cross-process navigation.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1468
>>      if (!webPlugin->initialize(container.get()))
> 
> at the very least, it seems like you should no longer need to pass container
> to the initialize method.  it is redundant to both pass it here and in the
> constructor for a WebPlugin.

Will make this change.

[Fady Samuel]

Created attachment 141011 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 141011 [details]
Patch

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

> Source/WebKit/chromium/public/WebPlugin.h:58
> +    // FIXME: Make this pure virtual once all derived classes implement this.

nit: add a new line here

nit: we actually have a policy that embedder implemented interfaces do not need
to have pure virtual methods.  that way it is easier to make API changes, so
you can just drop this comment.

[Darin Fisher (:fishd, Google)]

Fady and I discussed this offline.  We could also solve this issue by making all WebPlugin classes on the Chromium side implement a subclass of WebPlugin designed purely to track the WebPluginContainer pointer.  Normally, I don't like to add APIs to WebKit that WebKit neither calls nor implements.  Adding such an intermediary class just seems like bloat, so I'm happy to go ahead with this 'container()' accessor on WebPlugin.

[Fady Samuel]

Created attachment 141018 [details]
Patch

[Fady Samuel]

Created attachment 141024 [details]
Patch

[WebKit Review Bot]

Comment on attachment 141024 [details]
Patch

Clearing flags on attachment: 141024

Committed r116573: <http://trac.webkit.org/changeset/116573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86038

[WebKit Review Bot]

Comment on attachment 141024 [details]
Patch

Clearing flags on attachment: 141024

Committed r116810: <http://trac.webkit.org/changeset/116810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.