Bug 133667 – Don't snapshot offscreen plugins that would normally be considered primary plugins after they are moved in view

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Roger Fong]

We've run into an issue where some sites (namely youtube) start their plugins offscreen and then move them into view after clicking a video link.
The issue is that by the time the user clicks the video the offscreen plugin has already been snapshotted.
We should make sure to not snapshot said plugin if 
The plugin is the right size to be a primary plugin ADN
1) The maximum number of snapshot retries has been reached and no good snapshot has yet been found
OR
2) The plugin is still trying to snapshot properly and in the process is moved into view.

<rdar://problem/16743250>

[Roger Fong]

Created attachment 232752 [details]
patch

[Tim Horton]

Comment on attachment 232752 [details]
patch

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

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1687
> +    WebCore::HTMLPlugInImageElement* plugInImageElement = toHTMLPlugInImageElement(m_pluginElement.get());

I think there's no need for the WebCore:: here. Is this always safe or should you check the type?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1706
> +        snapshotFound = snapshotImage && !isAlmostSolidColor(toBitmapImage(snapshotImage.get()));
>  
>  #if PLATFORM(COCOA)
>          unsigned maximumSnapshotRetries = frame() ? frame()->settings().maximumPlugInSnapshotAttempts() : 0;
> -        if (snapshotImage && isAlmostSolidColor(toBitmapImage(snapshotImage.get())) && m_countSnapshotRetries < maximumSnapshotRetries) {
> +        if (snapshotImage && isAlmostSolidColor(toBitmapImage(snapshotImage.get())) && m_countSnapshotRetries < maximumSnapshotRetries && !plugInCameOnScreen) {

why are we checking isAlmostSolidColor twice in quick succession here? can the if() use snapshotFound?

> Source/WebKit2/WebProcess/Plugins/PluginView.h:278
> +    bool m_didPlugInStartOffScreen;

maybe pack this better, next to other bools? I dunno.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4466
> +bool WebPage::plugInIntersectsSearchRect(HTMLPlugInImageElement* plugInImageElement)

should this (and plugInIsPrimarySize) take references?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4485
> +    if (plugInRectRelativeToTopDocument.intersects(searchRect))
> +        return true;
> +    return false;

Maybe just "return plugInRectRelativeToTopDocument.intersects(searchRect);"?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4488
> +bool WebPage::plugInIsPrimarySize(WebCore::HTMLPlugInImageElement *plugInImageElement, unsigned &candidatePlugInArea)

star on the wrong side.

[Roger Fong]

Created attachment 232882 [details]
patch

With Tim's changes.
Using commit queue to land.

[WebKit Commit Bot]

Comment on attachment 232882 [details]
patch

Rejecting attachment 232882 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 232882, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Timothy Horton found in /Volumes/Data/EWS/WebKit/Source/WebKit2/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/5021336309071872

[Roger Fong]

Created attachment 232886 [details]
patch

[WebKit Commit Bot]

Comment on attachment 232886 [details]
patch

Clearing flags on attachment: 232886

Committed r169820: <http://trac.webkit.org/changeset/169820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #6)
> (From update of attachment 232886 [details])
> Clearing flags on attachment: 232886
> 
> Committed r169820: <http://trac.webkit.org/changeset/169820>

It broke the !ENABLE(PRIMARY_SNAPSHOTTED_PLUGIN_HEURISTIC) builds:

/home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp: In member function 'void WebKit::PluginView::pluginSnapshotTimerFired()':
/home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1693:40: error: 'class WebKit::WebPage' has no member named 'plugInIntersectsSearchRect'
/home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1707:96: error: 'isAlmostSolidColor' was not declared in this scope
/home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1721:20: error: 'class WebKit::WebPage' has no member named 'plugInIsPrimarySize'

[Gyuyoung Kim]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 232886 [details] [details])
> > Clearing flags on attachment: 232886
> > 
> > Committed r169820: <http://trac.webkit.org/changeset/169820>
> 
> It broke the !ENABLE(PRIMARY_SNAPSHOTTED_PLUGIN_HEURISTIC) builds:
> 
> /home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp: In member function 'void WebKit::PluginView::pluginSnapshotTimerFired()':
> /home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1693:40: error: 'class WebKit::WebPage' has no member named 'plugInIntersectsSearchRect'
> /home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1707:96: error: 'isAlmostSolidColor' was not declared in this scope
> /home/webkit/WebKit/Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1721:20: error: 'class WebKit::WebPage' has no member named 'plugInIsPrimarySize'

I file a bug to fix this error.
https://bugs.webkit.org/show_bug.cgi?id=133786