Plugins that start out as very small and then resize to something visible should be stopped, and re-enabled with user input. However, once the user has enabled it, we don't stop it again.
<rdar://problem/12696259>
*** Bug 114539 has been marked as a duplicate of this bug. ***
Created attachment 197919 [details] Patch
Comment on attachment 197919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197919&action=review > Source/WebCore/html/HTMLPlugInImageElement.h:155 > + bool m_avoidedSnapshotDueToSize; I think this variable is a little misleading. In the case of a really large plugin, it also avoids snapshot due to its size. I suppose doing a rename here to m_avoidedSnapshotDueToTinySize would be a quick fix. But I wonder if the code might be more comprehensible if we went with an enum instead of these two bools, representing the reason for the initial evaluation of auto-starting. That gives more flexibility in tweaking behavior depending on the auto-start reason. Not strictly necessary, but I think doing that would avoid having to later triangulate a combo of variables to determine what’s happening.
(In reply to comment #5) > (From update of attachment 197919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197919&action=review > > > Source/WebCore/html/HTMLPlugInImageElement.h:155 > > + bool m_avoidedSnapshotDueToSize; > > I think this variable is a little misleading. In the case of a really large plugin, it also avoids snapshot due to its size. I suppose doing a rename here to m_avoidedSnapshotDueToTinySize would be a quick fix. > > But I wonder if the code might be more comprehensible if we went with an enum instead of these two bools, representing the reason for the initial evaluation of auto-starting. That gives more flexibility in tweaking behavior depending on the auto-start reason. Not strictly necessary, but I think doing that would avoid having to later triangulate a combo of variables to determine what’s happening. Great suggestion! I've implemented an enum. New patch coming very soon.
Created attachment 197957 [details] Patch
Comment on attachment 197957 [details] Patch Attachment 197957 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/122226
Comment on attachment 197957 [details] Patch Attachment 197957 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/102276
Comment on attachment 197957 [details] Patch Attachment 197957 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/129235
Created attachment 197958 [details] Patch
Comment on attachment 197958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197958&action=review Caution: much nitpicking ahead. (And overall, “plugIns", capital I!!) > Source/WebCore/html/HTMLPlugInImageElement.cpp:103 > + m_reasonForSnapshotting = NoSnapshotRestarted; Set to NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:468 > Nit. Could we have the outer if here do an early return instead? if (!frame->loader()->subframeLoader()->containsPlugins()) continue; It reduces indentation and braces. > Source/WebCore/html/HTMLPlugInImageElement.cpp:469 > RefPtr<NodeList> plugins = frame->document()->getElementsByTagName(embedTag.localName()); Nit: plugIns. > Source/WebCore/html/HTMLPlugInImageElement.cpp:470 > if (plugins) Nit: Combine the two here? if (RefPtr<NodeList> plugIns = ...) > Source/WebCore/html/HTMLPlugInImageElement.cpp:474 > if (plugins) Nit: Ditto. > Source/WebCore/html/HTMLPlugInImageElement.cpp:479 > + for (size_t i = 0, length = similarPlugins.size(); i < length; i++) { Nit: prefix increment > Source/WebCore/html/HTMLPlugInImageElement.cpp:486 > + pluginToRestart->m_blessedPlugin = true; Instead of this variable, set the SnapshotDecision to NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:543 > + if (!m_needsCheckForSizeChange || m_reasonForSnapshotting != NoSnapshotTinySize || m_blessedPlugin) I think you can reduce this to just m_reasonForSnapshotting != MaySnapshotWhenResized > Source/WebCore/html/HTMLPlugInImageElement.cpp:556 > + m_reasonForSnapshotting = SnapshotResized; AlwaysSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:570 > + m_reasonForSnapshotting = NoSnapshotDisabled; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:576 > + m_reasonForSnapshotting = NoSnapshotBlessed; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:582 > + m_reasonForSnapshotting = NoSnapshotRestarted; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:589 > + m_reasonForSnapshotting = NoSnapshotRestarted; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:597 > + m_reasonForSnapshotting = NoSnapshotPluginDocument; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:604 > + m_blessedPlugin = true; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:611 > + m_blessedPlugin = true; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:621 > + m_blessedPlugin = true; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:628 > setDisplayState(WaitingForSnapshot); AlwaysSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:635 > + m_blessedPlugin = true; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:654 > + m_blessedPlugin = true; NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:661 > + m_reasonForSnapshotting = NoSnapshotTinySize; MaySnapshotWhenResized > Source/WebCore/html/HTMLPlugInImageElement.cpp:668 > setDisplayState(WaitingForSnapshot); AlwaysSnapshot > Source/WebCore/html/HTMLPlugInImageElement.cpp:673 > + m_reasonForSnapshotting = SnapshotDefault; AlwaysSnapshot > Source/WebCore/html/HTMLPlugInImageElement.h:89 > + void setNeedsCheckForSizeChange() { m_needsCheckForSizeChange = true; } Not sure we need these two anymore? > Source/WebCore/html/HTMLPlugInImageElement.h:92 > + enum SnapshotReason { I think we can make this clearer. If you read these enums as written, you’d think NoSnapshotDisabled means that all snapshots are enabled, where in the code it denotes that plugins autostart because the feature is disabled. “SnapshotReason” could be improved because it implies that we’re snapshotting, when in fact it contains values to snapshot and not snapshot. How about “SnapshotDecision”? Suggestions: > Source/WebCore/html/HTMLPlugInImageElement.h:93 > + NoSnapshotValue, NotYetDetermined > Source/WebCore/html/HTMLPlugInImageElement.h:94 > + NoSnapshotDisabled, NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.h:95 > + NoSnapshotPluginDocument, Merge into NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.h:96 > + NoSnapshotTinySize, MaySnapshotWhenResized > Source/WebCore/html/HTMLPlugInImageElement.h:101 > + NoSnapshotTopLevel, Merge these 5 into NeverSnapshot > Source/WebCore/html/HTMLPlugInImageElement.h:106 > + }; Add “AlwaysSnapshot" > Source/WebCore/html/HTMLPlugInImageElement.h:171 > + bool m_blessedPlugin; Unneeded? > Source/WebCore/html/HTMLPlugInImageElement.h:172 > + bool m_needsCheckForSizeChange; Unneeded? > Source/WebCore/page/FrameView.cpp:1425 > + if (!pluginElement->needsCheckForSizeChange()) check for !MaySnapshotWhenResized instead > Source/WebCore/page/FrameView.cpp:2599 > + pluginElement->checkSnapshotStatus(); All on one line to avoid temporary. > Source/WebCore/rendering/RenderEmbeddedObject.cpp:312 > + if (plugInImageElement->reasonForSnapshotting() == HTMLPlugInImageElement::NoSnapshotTinySize && document()->view()) { Check MaySnapshotWhenResized
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:468 > > > > Nit. Could we have the outer if here do an early return instead? if (!frame->loader()->subframeLoader()->containsPlugins()) continue; > It reduces indentation and braces. Done > > > Source/WebCore/html/HTMLPlugInImageElement.cpp:469 > > RefPtr<NodeList> plugins = frame->document()->getElementsByTagName(embedTag.localName()); > > Nit: plugIns. Done > > > Source/WebCore/html/HTMLPlugInImageElement.cpp:470 > > if (plugins) > > Nit: Combine the two here? if (RefPtr<NodeList> plugIns = ...) I have a personal bias against that style :) However, the variable is used after the conditional, so it's ok to be outside. > > Source/WebCore/html/HTMLPlugInImageElement.cpp:479 > > + for (size_t i = 0, length = similarPlugins.size(); i < length; i++) { > > Nit: prefix increment Done > > Source/WebCore/html/HTMLPlugInImageElement.cpp:486 > > + pluginToRestart->m_blessedPlugin = true; > > Instead of this variable, set the SnapshotDecision to NeverSnapshot Done > > > Source/WebCore/html/HTMLPlugInImageElement.cpp:543 > > + if (!m_needsCheckForSizeChange || m_reasonForSnapshotting != NoSnapshotTinySize || m_blessedPlugin) > > I think you can reduce this to just m_reasonForSnapshotting != MaySnapshotWhenResized Done > > Source/WebCore/html/HTMLPlugInImageElement.h:89 > > + void setNeedsCheckForSizeChange() { m_needsCheckForSizeChange = true; } > > Not sure we need these two anymore? We do need this because we can have the case were a widget needs an update and we're MaySnapshotWhenResized. i.e. we could have been put on the post-layout list for a reason *other* than a resize. > > Source/WebCore/html/HTMLPlugInImageElement.h:92 > > + enum SnapshotReason { > > I think we can make this clearer. If you read these enums as written, you’d > think NoSnapshotDisabled means that all snapshots are enabled, where in the > code it denotes that plugins autostart because the feature is disabled. > > “SnapshotReason” could be improved because it implies that we’re snapshotting, > when in fact it contains values to snapshot and not snapshot. How about > “SnapshotDecision”? Agreed and fixed. > > Source/WebCore/html/HTMLPlugInImageElement.h:171 > > + bool m_blessedPlugin; > > Unneeded? Yep, removed. > > Source/WebCore/html/HTMLPlugInImageElement.h:172 > > + bool m_needsCheckForSizeChange; > > Unneeded? Unfortunately still needed. See above. > > Source/WebCore/page/FrameView.cpp:1425 > > + if (!pluginElement->needsCheckForSizeChange()) > > check for !MaySnapshotWhenResized instead See above. > > Source/WebCore/page/FrameView.cpp:2599 > > + pluginElement->checkSnapshotStatus(); > > All on one line to avoid temporary. It's used after the early return too. > > Source/WebCore/rendering/RenderEmbeddedObject.cpp:312 > > + if (plugInImageElement->reasonForSnapshotting() == HTMLPlugInImageElement::NoSnapshotTinySize && document()->view()) { > > Check MaySnapshotWhenResized Done.
Created attachment 198195 [details] Patch
Comment on attachment 198195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198195&action=review > Source/WebCore/html/HTMLPlugInImageElement.cpp:559 > + static_cast<PluginViewBase*>(widget)->beginSnapshottingRunningPlugin(); Is there no toPluginViewBase()? > Source/WebCore/html/HTMLPlugInImageElement.cpp:673 > + if (!widget->isPluginViewBase() || !static_cast<const PluginViewBase*>(widget)->shouldAlwaysAutoStart()) Here too. > Source/WebCore/rendering/RenderEmbeddedObject.cpp:306 > + if (!wasMissingWidget && newSize.width() >= oldSize.width() && newSize.height() >= oldSize.height()) { That's a lot of ifs. Is there anything better we can do here depth-wise?
Comment on attachment 198195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198195&action=review > Source/WebCore/rendering/RenderEmbeddedObject.cpp:308 > + if (element && element->isPluginElement()) { How about adding "&& toHTMLPlugInElement(element)->isPlugInImageElement()" here? then... > Source/WebCore/rendering/RenderEmbeddedObject.cpp:309 > + HTMLPlugInElement* plugInElement = toHTMLPlugInElement(element); ... HTMLPlugInImageElement can be defined here... > Source/WebCore/rendering/RenderEmbeddedObject.cpp:312 > + if (plugInImageElement->snapshotDecision() == HTMLPlugInImageElement::MaySnapshotWhenResized && document()->view()) { ... and you can push the displayState() check here, getting rid of one of the ifs.
Comment on attachment 198195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198195&action=review >> Source/WebCore/html/HTMLPlugInImageElement.cpp:559 >> + static_cast<PluginViewBase*>(widget)->beginSnapshottingRunningPlugin(); > > Is there no toPluginViewBase()? Fixed! >> Source/WebCore/html/HTMLPlugInImageElement.cpp:673 >> + if (!widget->isPluginViewBase() || !static_cast<const PluginViewBase*>(widget)->shouldAlwaysAutoStart()) > > Here too. Also fixed. >> Source/WebCore/rendering/RenderEmbeddedObject.cpp:306 >> + if (!wasMissingWidget && newSize.width() >= oldSize.width() && newSize.height() >= oldSize.height()) { > > That's a lot of ifs. Is there anything better we can do here depth-wise? Not sure I can do much about it. I can't return early because the code afterwards needs to run.
(In reply to comment #16) > (From update of attachment 198195 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198195&action=review > > > Source/WebCore/rendering/RenderEmbeddedObject.cpp:308 > > + if (element && element->isPluginElement()) { > > How about adding "&& toHTMLPlugInElement(element)->isPlugInImageElement()" here? then... > > > Source/WebCore/rendering/RenderEmbeddedObject.cpp:309 > > + HTMLPlugInElement* plugInElement = toHTMLPlugInElement(element); > > ... HTMLPlugInImageElement can be defined here... > > > Source/WebCore/rendering/RenderEmbeddedObject.cpp:312 > > + if (plugInImageElement->snapshotDecision() == HTMLPlugInImageElement::MaySnapshotWhenResized && document()->view()) { > > ... and you can push the displayState() check here, getting rid of one of the ifs. Good suggestions. Done.
https://trac.webkit.org/r148482