Summary: | [WK2] Activate plugins when user clicks on snapshot | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | Jon Lee <jonlee> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | andersca, beidson, dglazkov, eric, gustavo, gyuyoung.kim, japhet, peter+ews, philn, rakuco, webkit-bug-importer, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | OS X 10.8 | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 98318 | ||||||||||||||||||
Attachments: |
|
Description
Jon Lee
2012-10-03 16:44:51 PDT
Created attachment 167847 [details]
Patch
Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14228797 Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14238457 Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14223887 Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14218877 Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14223883 Comment on attachment 167847 [details] Patch Attachment 167847 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14228805 Created attachment 167869 [details]
Patch
Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14245269 Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14217984 Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14249144 Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14214951 Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14223932 Comment on attachment 167869 [details] Patch Attachment 167869 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14244401 Created attachment 167891 [details]
Patch
Comment on attachment 167891 [details] Patch Attachment 167891 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14231006 Created attachment 167901 [details]
Patch
Comment on attachment 167901 [details] Patch Attachment 167901 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14244439 Created attachment 167911 [details]
Patch
Holy mother, that passed EWS!!!! 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* 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? (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. (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. > > 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.
Created attachment 168068 [details]
Patch
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. (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. Created attachment 168075 [details]
Patch
Committed r130977: <http://trac.webkit.org/changeset/130977> |