[Chromium] Expose WebPluginContainer to WebFrameClient::createPlugin (for Browser Plugin)
Created attachment 140783 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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
Created attachment 140788 [details] Patch
That's not really sufficient. What exactly is the problem you are attempting to solve and how does this patch do that?
(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.
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
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
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.
(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.
(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.
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.
Created attachment 141011 [details] Patch
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.
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.
Created attachment 141018 [details] Patch
Created attachment 141024 [details] Patch
Comment on attachment 141024 [details] Patch Clearing flags on attachment: 141024 Committed r116573: <http://trac.webkit.org/changeset/116573>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 86038
Comment on attachment 141024 [details] Patch Clearing flags on attachment: 141024 Committed r116810: <http://trac.webkit.org/changeset/116810>