Bug 102157 – Plugins that resize might need to be snapshotted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

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.

[Radar WebKit Bug Importer]

<rdar://problem/12696259>

[Tim Horton]

*** Bug 114539 has been marked as a duplicate of this bug. ***

[Dean Jackson]

Created attachment 197919 [details]
Patch

[Jon Lee]

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.

[Jon Lee]

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.

[Dean Jackson]

(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.

[Dean Jackson]

Created attachment 197957 [details]
Patch

[EFL EWS Bot]

Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/122226

[Early Warning System Bot]

Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/102276

[kov's GTK+ EWS bot]

Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/129235

[Dean Jackson]

Created attachment 197958 [details]
Patch

[Jon Lee]

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

[Dean Jackson]

> > 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.

[Dean Jackson]

Created attachment 198195 [details]
Patch

[Tim Horton]

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?

[Jon Lee]

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.

[Dean Jackson]

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.

[Dean Jackson]

(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.

[Dean Jackson]

https://trac.webkit.org/r148482