Bug 98328 – [WK2] Activate plugins when user clicks on snapshot

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Remove the placeholder and create a new plugin with the same initialization parameters.

[Radar WebKit Bug Importer]

<rdar://problem/12426681>

[Jon Lee]

Created attachment 167847 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167847 [details]
Patch

Attachment 167847 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14228797

[Early Warning System Bot]

Comment on attachment 167847 [details]
Patch

Attachment 167847 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14238457

[Early Warning System Bot]

Comment on attachment 167847 [details]
Patch

Attachment 167847 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14223887

[WebKit Review Bot]

Comment on attachment 167847 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 167847 [details]
Patch

Attachment 167847 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14223883

[Build Bot]

Comment on attachment 167847 [details]
Patch

Attachment 167847 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14228805

[Jon Lee]

Created attachment 167869 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167869 [details]
Patch

Attachment 167869 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14245269

[WebKit Review Bot]

Comment on attachment 167869 [details]
Patch

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

[Build Bot]

Comment on attachment 167869 [details]
Patch

Attachment 167869 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14249144

[Gyuyoung Kim]

Comment on attachment 167869 [details]
Patch

Attachment 167869 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14214951

[Early Warning System Bot]

Comment on attachment 167869 [details]
Patch

Attachment 167869 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14223932

[Peter Beverloo (cr-android ews)]

Comment on attachment 167869 [details]
Patch

Attachment 167869 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14244401

[Jon Lee]

Created attachment 167891 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167891 [details]
Patch

Attachment 167891 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14231006

[Jon Lee]

Created attachment 167901 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167901 [details]
Patch

Attachment 167901 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14244439

[Jon Lee]

Created attachment 167911 [details]
Patch

[Brady Eidson]

Holy mother, that passed EWS!!!!

[Brady Eidson]

Comment on attachment 167911 [details]
Patch

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*

[Jon Lee]

Comment on attachment 167911 [details]
Patch

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

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:139
>> +        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.

I think we should do something similar to #2, but figured the experience for the user would be the same as when they first entered the page-- that is, when you go back, the plugin runs again to grab a new snapshot, so there is no need to hang on to the snapshot.

I guess if we couldn't recreate the plug-in, maybe we should switch to an error message? "Plug-in error" or something?

>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:197
>> +        return;
> 
> I think we need to stop a plug-in's streams when we kill it.

Are you suggesting I also call cancelStreams here? When we get the snapshot in platformSnapshotTimerFired(), I call destroyPluginAndReset(), which calls cancelAllStreams().

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
>> +}
> 
> 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*

Do you mean Widget?

[Brady Eidson]

(In reply to comment #23)
> (From update of attachment 167911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167911&action=review
> 
> >> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:139
> >> +        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.
> 
> I think we should do something similar to #2, but figured the experience for the user would be the same as when they first entered the page-- that is, when you go back, the plugin runs again to grab a new snapshot, so there is no need to hang on to the snapshot.

That seems crazy, though, since the experience when they go back *could* be better than the first time they visited... assuming we hang on to the snapshot.

> I guess if we couldn't recreate the plug-in, maybe we should switch to an error message? "Plug-in error" or something?

Sounds reasonable.

> >> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:197
> >> +        return;
> > 
> > I think we need to stop a plug-in's streams when we kill it.
> 
> Are you suggesting I also call cancelStreams here? When we get the snapshot in platformSnapshotTimerFired(), I call destroyPluginAndReset(), which calls cancelAllStreams().

So if we *are* canceling all streams, then why do we need these new protections here?

> >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
> >> +}
> > 
> > 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*
> 
> Do you mean Widget?

I did, yes.

[Jon Lee]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 167911 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167911&action=review
> > 
> > >> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:139
> > >> +        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.
> > 
> > I think we should do something similar to #2, but figured the experience for the user would be the same as when they first entered the page-- that is, when you go back, the plugin runs again to grab a new snapshot, so there is no need to hang on to the snapshot.
> 
> That seems crazy, though, since the experience when they go back *could* be better than the first time they visited... assuming we hang on to the snapshot.

Ok. I will get rid of the cached image in the next patch. I had already filed bug 98529 to deal with fixing issues with back/forward.

> 
> > I guess if we couldn't recreate the plug-in, maybe we should switch to an error message? "Plug-in error" or something?
> 
> Sounds reasonable.

Filed 98929 to cover this.

> 
> > >> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:197
> > >> +        return;
> > > 
> > > I think we need to stop a plug-in's streams when we kill it.
> > 
> > Are you suggesting I also call cancelStreams here? When we get the snapshot in platformSnapshotTimerFired(), I call destroyPluginAndReset(), which calls cancelAllStreams().
> 
> So if we *are* canceling all streams, then why do we need these new protections here?

Before, the lifetime of the PluginProxy and PluginView were the same. Any leftover stream callbacks could be forwarded to the PluginProxy, even when the plugin in the PluginProcess was getting destroyed. With plugin snapshotting we break that relationship, and PluginProxy might be null while the PluginView is alive. Hence these extra checks are needed.

> > >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
> > >> +}
> > > 
> > > 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*
> > 
> > Do you mean Widget?
> 
> I did, yes.
Done.

[Jon Lee]

> > So if we *are* canceling all streams, then why do we need these new protections here?
> 
> Before, the lifetime of the PluginProxy and PluginView were the same. Any leftover stream callbacks could be forwarded to the PluginProxy, even when the plugin in the PluginProcess was getting destroyed. With plugin snapshotting we break that relationship, and PluginProxy might be null while the PluginView is alive. Hence these extra checks are needed.

Looks like I was wrong. By canceling the streams, any pending callbacks should immediately be cancelled, obviating the need to do these extra checks. I will remove them in the next patch.

[Jon Lee]

Created attachment 168068 [details]
Patch

[Brady Eidson]

Comment on attachment 168068 [details]
Patch

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

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:141
> +    if (event->type() == eventNames().clickEvent && mouseEvent->button() == LeftButton) {
> +        plugInImageElement()->setDisplayState(HTMLPlugInElement::Playing);
> +        if (Frame* frame = document()->frame())
> +            frame->loader()->client()->recreatePlugin(widget());
> +        repaint();
> +        event->setDefaultHandled();
> +    }

Does the call to widget() just access a Widget, or does it sometimes create one?

This was the crux of my comment about making recreatePlugin() take a Widget instead of an Element.  If we don't already have a Widget, we shouldn't be calling recreatePlugin().

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
> +    ASSERT(widget->isPluginViewBase());
> +    PluginView* pluginView = static_cast<PluginView*>(widget);
> +    ASSERT(m_frame->page());
> +    RefPtr<Plugin> plugin = m_frame->page()->createPlugin(m_frame, pluginView->pluginElement(), pluginView->initialParameters());
> +    pluginView->recreateAndInitialize(plugin.release());

Nit - I think the ASSERTs should both be at the head of the function, and separated by an empty line from the actual code.

[Jon Lee]

(In reply to comment #28)
> (From update of attachment 168068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168068&action=review
> 
> > Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:141
> > +    if (event->type() == eventNames().clickEvent && mouseEvent->button() == LeftButton) {
> > +        plugInImageElement()->setDisplayState(HTMLPlugInElement::Playing);
> > +        if (Frame* frame = document()->frame())
> > +            frame->loader()->client()->recreatePlugin(widget());
> > +        repaint();
> > +        event->setDefaultHandled();
> > +    }
> 
> Does the call to widget() just access a Widget, or does it sometimes create one?

It just returns the widget pointer.

> This was the crux of my comment about making recreatePlugin() take a Widget instead of an Element.  If we don't already have a Widget, we shouldn't be calling recreatePlugin().

I will add a check for widget().

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1362
> > +    ASSERT(widget->isPluginViewBase());
> > +    PluginView* pluginView = static_cast<PluginView*>(widget);
> > +    ASSERT(m_frame->page());
> > +    RefPtr<Plugin> plugin = m_frame->page()->createPlugin(m_frame, pluginView->pluginElement(), pluginView->initialParameters());
> > +    pluginView->recreateAndInitialize(plugin.release());
> 
> Nit - I think the ASSERTs should both be at the head of the function, and separated by an empty line from the actual code.

Done.

[Jon Lee]

Created attachment 168075 [details]
Patch

[Jon Lee]

Committed r130977: <http://trac.webkit.org/changeset/130977>