Bug 98326 – [WK2] Have plugins render initially offscreen to capture snapshot

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

[Jon Lee]

Once a timer fires after the plugin initializes, make a snapshot of the plugin offscreen to display as a placeholder.

[Radar WebKit Bug Importer]

<rdar://problem/12426658>

[Jon Lee]

Created attachment 167616 [details]
Patch

[Brian Weinstein]

Comment on attachment 167616 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:271
> +    , m_pluginSnapshotTimer(this, &PluginView::pluginSnapshotTimerFired, 3)

Should this be a named constant?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1381
> +{

Do you want to do something like: ASSERT_UNUSED(timer, timer == m_pluginSnapshotTimer?

[Jon Lee]

Comment on attachment 167616 [details]
Patch

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

>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:271
>> +    , m_pluginSnapshotTimer(this, &PluginView::pluginSnapshotTimerFired, 3)
> 
> Should this be a named constant?

Done.

>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1381
>> +{
> 
> Do you want to do something like: ASSERT_UNUSED(timer, timer == m_pluginSnapshotTimer?

Done.

[Jon Lee]

Created attachment 167782 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 167782 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:292
> +    // Null out the plug-in element explicitly so we'll crash earlier if we try to use
> +    // the plug-in view after it's been destroyed.
> +    m_pluginElement = nullptr;

There doesn't seem to be much point in doing this in the destructor.

> Source/WebKit2/WebProcess/Plugins/PluginView.h:235
> +    // The snapshot is used in two scenarios:
> +    // 1) If plugin snapshotting is enabled, this snapshot represents the poster image.
> +    // 2) Otherwise, it represents the first image displayed when the plugin is run, to avoid
> +    // side effects should the plugin perform DOM manipulation on initialization.
>      RefPtr<ShareableBitmap> m_snapshot;
> +    RefPtr<WebCore::Image> m_snapshotImage;

This comment is ambiguous now; is it referring to m_snapshot, or m_snapshotImage. Do we still need both of these? Can they be renamed to make their purposes more obvious?

[Jon Lee]

(In reply to comment #6)
> (From update of attachment 167782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167782&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:292
> > +    // Null out the plug-in element explicitly so we'll crash earlier if we try to use
> > +    // the plug-in view after it's been destroyed.
> > +    m_pluginElement = nullptr;
> 
> There doesn't seem to be much point in doing this in the destructor.

This already existed, and seems to have been added as a sanity check, so I will leave this in.
> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.h:235
> > +    // The snapshot is used in two scenarios:
> > +    // 1) If plugin snapshotting is enabled, this snapshot represents the poster image.
> > +    // 2) Otherwise, it represents the first image displayed when the plugin is run, to avoid
> > +    // side effects should the plugin perform DOM manipulation on initialization.
> >      RefPtr<ShareableBitmap> m_snapshot;
> > +    RefPtr<WebCore::Image> m_snapshotImage;
> 
> This comment is ambiguous now; is it referring to m_snapshot, or m_snapshotImage. Do we still need both of these? Can they be renamed to make their purposes more obvious?

Yes, I believe I can get rid of the image variable and make this all happen locally. For readability, this means that instead of giving an Image* to WebCore, I will be using PassRefPtr<Image> instead.

Unfortunately, there is still a naming issue, because there is m_snapshot, and m_pluginSnapshotTimer, which at first glance would appear related. I am appending comments to explain that they are unrelated, and will rename m_snapshot to m_transientPaintingSnapshot.

New patch forthcoming.

[Jon Lee]

Created attachment 167813 [details]
Patch

Rename m_snapshot to m_transientPaintingSnapshot, refactor pluginSnapshotTimerFired to use local variables.

[Jon Lee]

Committed r130810: <http://trac.webkit.org/changeset/130810>