Bug 110495 - Plug-in snapshotting code always accepts first snapshot
Summary: Plug-in snapshotting code always accepts first snapshot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-21 11:58 PST by Dean Jackson
Modified: 2013-02-21 13:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.08 KB, patch)
2013-02-21 12:08 PST, 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 Dean Jackson 2013-02-21 11:58:31 PST
An HTMLPlugInImageElement swaps to a different renderer when it receives a snapshot from its embedding API (currently only WK2). This is a regression - previously we'd take a number of snapshots waiting for an image that had useful data, and there was a common renderer for the plugin and snapshot.

This is the first part of the fix, which is to have the embedder tell the plugin when it thinks it has enough to snapshot. Only then will we swap to the other renderer.

A later fix will be to make sure that we show intermediate (if mostly empty) snapshots.
Comment 1 Dean Jackson 2013-02-21 12:08:59 PST
Created attachment 189570 [details]
Patch
Comment 2 Tim Horton 2013-02-21 13:22:16 PST
Comment on attachment 189570 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:74
> -static const unsigned maximumSnapshotRetries = 60;
> +static const unsigned maximumSnapshotRetries = 4;

I don't see an explanation for this change.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1609
> +    m_pluginElement->setDisplayState(HTMLPlugInElement::DisplayingSnapshot);

When does the tear-down happen now?
Comment 3 Dean Jackson 2013-02-21 13:28:02 PST
Comment on attachment 189570 [details]
Patch

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

>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:74
>> +static const unsigned maximumSnapshotRetries = 4;
> 
> I don't see an explanation for this change.

I will add this to the changelog:

I also reduced the number of snapshot attempts we will make before giving up. We don't want to sit around for 66 seconds displaying nothing.

>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1609
>> +    m_pluginElement->setDisplayState(HTMLPlugInElement::DisplayingSnapshot);
> 
> When does the tear-down happen now?

Oh, that happens anyway, because as soon as I set the displayState it will swap renderers. It was the existing renderer that had the Widget, and the reference to the PluginView, and so we tear things down in the destructor.
Comment 4 Dean Jackson 2013-02-21 13:57:36 PST
Committed r143650: <http://trac.webkit.org/changeset/143650>