Bug 102157 - Plugins that resize might need to be snapshotted
Summary: Plugins that resize might need to be snapshotted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
: 114539 (view as bug list)
Depends on:
Blocks: 98318
  Show dependency treegraph
 
Reported: 2012-11-13 17:29 PST by Jon Lee
Modified: 2013-04-15 18:06 PDT (History)
14 users (show)

See Also:


Attachments
Patch (21.91 KB, patch)
2013-04-12 21:13 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (24.44 KB, patch)
2013-04-13 14:47 PDT, Dean Jackson
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (24.46 KB, patch)
2013-04-13 15:06 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (28.62 KB, patch)
2013-04-15 15:56 PDT, Dean Jackson
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-11-13 17:29:25 PST
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.
Comment 1 Radar WebKit Bug Importer 2012-11-13 17:29:39 PST
<rdar://problem/12696259>
Comment 2 Tim Horton 2013-04-12 15:27:18 PDT
*** Bug 114539 has been marked as a duplicate of this bug. ***
Comment 3 Dean Jackson 2013-04-12 21:13:57 PDT
Created attachment 197919 [details]
Patch
Comment 4 Jon Lee 2013-04-12 21:40:15 PDT
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.
Comment 5 Jon Lee 2013-04-12 21:40:32 PDT
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.
Comment 6 Dean Jackson 2013-04-13 14:12:53 PDT
(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.
Comment 7 Dean Jackson 2013-04-13 14:47:38 PDT
Created attachment 197957 [details]
Patch
Comment 8 EFL EWS Bot 2013-04-13 14:52:47 PDT
Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/122226
Comment 9 Early Warning System Bot 2013-04-13 14:53:17 PDT
Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/102276
Comment 10 kov's GTK+ EWS bot 2013-04-13 14:58:44 PDT
Comment on attachment 197957 [details]
Patch

Attachment 197957 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/129235
Comment 11 Dean Jackson 2013-04-13 15:06:40 PDT
Created attachment 197958 [details]
Patch
Comment 12 Jon Lee 2013-04-13 18:17:37 PDT
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
Comment 13 Dean Jackson 2013-04-15 15:49:37 PDT
> > 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.
Comment 14 Dean Jackson 2013-04-15 15:56:46 PDT
Created attachment 198195 [details]
Patch
Comment 15 Tim Horton 2013-04-15 16:39:35 PDT
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 16 Jon Lee 2013-04-15 17:18:30 PDT
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 17 Dean Jackson 2013-04-15 17:18:52 PDT
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.
Comment 18 Dean Jackson 2013-04-15 17:54:29 PDT
(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.
Comment 19 Dean Jackson 2013-04-15 18:06:25 PDT
https://trac.webkit.org/r148482