[webkit-reviews] review denied: [Bug 98328] [WK2] Activate plugins when user clicks on snapshot : [Attachment 167911] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Jon Lee <jonlee at apple.com>'s
request for review:
Bug 98328: [WK2] Activate plugins when user clicks on snapshot
https://bugs.webkit.org/show_bug.cgi?id=98328

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167911&action=review


This looks good, but I have a handful of comments.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:139
> +	   if (Frame* frame = document()->frame())
> +	       frame->loader()->client()->recreatePlugin(plugInImageElement());

> +	   m_snapshotResource->setCachedImage(0);

It seems weird that we're only able to start the plug-in if there's a frame,
but we clear the snapshot image whether or not we recreated the plugin.

If we weren't able to recreate the plug-in, shouldn't we hang on to the
snapshot image?

Also, I'm not sure clearing the snapshot image is ever correct, as we might
need it for the page cache.  I can think about this two ways:
1 - When the user leaves the page we should snapshot the most recent thing they
saw before destroying the plug-in.
Pro: We don't need to hang on to the snapshot image the whole time
Pro: When they return to the page, they're looking at something familiar
Con: If they clicked the "more accurate" snapshot, the plugin would still have
to be restarting from scratch

2 - When the user leaves the page, we kill the plug-in and put the original
snapshot back in place.
Pro: The experience after returning to the page might make more sense with
regards to the experience they have once they click
Con: We have to hang on to the snapshot the whole time.

I'm leaning towards #2, but it's certainly up for discussion.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:197
> +    // The plugin might have been shut down after the snapshot was taken for
click-to-start plugins.
> +    if (!m_pluginView->m_plugin)
> +	   return;

I think we need to stop a plug-in's streams when we kill it.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:217
> +    // The plugin might have been shut down after the snapshot was taken for
click-to-start plugins.
> +    if (!m_pluginView->m_plugin)
> +	   return;

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
> +void WebFrameLoaderClient::recreatePlugin(WebCore::HTMLPlugInElement*
pluginElement)
> +{
> +    PluginView* widget =
static_cast<PluginView*>(pluginElement->pluginWidget());
> +    ASSERT(m_frame->page());
> +    RefPtr<Plugin> plugin = m_frame->page()->createPlugin(m_frame,
pluginElement, widget->initialParameters());
> +    widget->recreateAndInitialize(plugin.release());
> +}

I'm not in love with the HTMLPluginElement being the argument to this function.
 It seems like it is more possible to get "RE-creating" it wrong, as an
HTMLPluginElement might never have created its widget.

I'd rather it take a RenderWidget*