[webkit-reviews] review denied: [Bug 85916] [Chromium] Expose WebPluginContainer to WebFrameClient::createPlugin (for Browser Plugin) : [Attachment 140788] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Fady Samuel
<fsamuel at chromium.org>'s request for review:
Bug 85916: [Chromium] Expose WebPluginContainer to WebFrameClient::createPlugin
(for Browser Plugin)
https://bugs.webkit.org/show_bug.cgi?id=85916

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.