Bug 132222 – Don't snapshot plugins that are overlaid with images.

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

[Roger Fong]

There seem to many cases where sites essentially do their own form of snapshotting by overlaying an image ontop of a plugin.
When the image is clicked the plugin tries to play, but cannot since it is snapshotted and must be restarted itself.
This fix is so that in the case where an image is roughly overlaid on top of a plugin, the plugin is set to be active.

[Roger Fong]

<rdar://problem/16653536>

[Roger Fong]

Created attachment 230248 [details]
patch

[Roger Fong]

I'm not quite sure whether the autoplay should be restricted to only a plugin that meets all the other requirements of a dominant plugin.

That might make more sense but it causes sites like http://www.nytimes.com/projects/2012/snow-fall/?forceredirect=yes to break.
Scroll down a bit and you'll see a video that is definitely not the dominant plugin, but does have an image overlaid on top of the plugin.

[Jon Lee]

Comment on attachment 230248 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4383
> +            

Remove.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4386
> +            

Remove.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4394
> +            inflatedPluginRect.scale(overlappingImageBoundsScale);

Is this correct? This scales both the position and size from the corner, not out from the center.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4402
> +                    continue;

I think the check should be reversed to avoid the awkward else clause

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4407
> +            

Remove.

> LayoutTests/plugins/snapshotting/autoplay-plugin-blocked-by-image.html:13
> +<img src="../resources/apple.gif" width="600" height="600" style="position:relative; top:-600px;">

Can we add some more test cases here to handle images that are within the percentages? What happens with transforms? absolute positioned image? Or taking markup inspired by some of the websites we’ve seen issues with?

[Roger Fong]

(In reply to comment #4)
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4394
> > +            inflatedPluginRect.scale(overlappingImageBoundsScale);
> 
> Is this correct? This scales both the position and size from the corner, not out from the center.

O right, need to translate the rect too.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4402
> > +                    continue;
> 
> I think the check should be reversed to avoid the awkward else clause
> 
ok

> > LayoutTests/plugins/snapshotting/autoplay-plugin-blocked-by-image.html:13
> > +<img src="../resources/apple.gif" width="600" height="600" style="position:relative; top:-600px;">
> 
> Can we add some more test cases here to handle images that are within the percentages? What happens with transforms? absolute positioned image? Or taking markup inspired by some of the websites we’ve seen issues with?

Sure

[Roger Fong]

Created attachment 230351 [details]
patch

[Darin Adler]

Comment on attachment 230351 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4388
> -
> +            

Please don’t add this whitespace.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4391
> -
> +            

Please don’t add this whitespace.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4392
>              Element* element = hitTestResult.innerElement();

If this is guaranteed to be non-null, then it should be an Element& rather than an Element*. If it’s not guaranteed to be non-null, then we need a check for null.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4393
> +            IntRect elementRectRelativeToView = element->clientRect();

Shouldn’t we be doing this with a function that returns a LayoutRect?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4398
> +            LayoutUnit xOffset = (inflatedPluginRect.width() - plugInRectRelativeToTopDocument.width()) / 2.0;

I don’t think that / 2.0 is a good idea here. I think that means we’ll convert to floating point and back to layout units but we’d rather just do the math as layout units instead.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4406
> +            bool isImageHidingPlugin = isHTMLImageElement(element)
> +                && inflatedPluginRect.contains(elementRectRelativeToTopDocument)
> +                && elementRectRelativeToTopDocument.width() > pluginRenderBox.width() * minimumOverlappingImageToPluginDimensionScale
> +                && elementRectRelativeToTopDocument.height() > pluginRenderBox.height() * minimumOverlappingImageToPluginDimensionScale;

We always compute this, but we only use it inside an if statement. I suggest moving the computation inside the if statement.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4415
> +            

Please don’t add this whitespace.

[Jon Lee]

Comment on attachment 230351 [details]
patch

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

It would be nice to include some logging so that it is easy to see what’s happening if we turn the channel on.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4388
> +            

Please remove the extraneous whitespace sprinkled throughout this patch.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4401
> +            inflatedPluginRect.setY(inflatedPluginRect.y() - yOffset);

Isn’t this what inflate() is for?

[Jon Lee]

whoops, i didn’t realize that affected the r+.

[Roger Fong]

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4393
> > +            IntRect elementRectRelativeToView = element->clientRect();
> 
> Shouldn’t we be doing this with a function that returns a LayoutRect?
> 

Said function does not exist yet. I talked to smfr about it and he said that clientRect needs to be converted from using IntRects to LayoutRects, which seems to be a separate bug, which I may attempt to fix or give to zalan to fix :).

[Roger Fong]

Created attachment 230404 [details]
patch

[Roger Fong]

(In reply to comment #9)
> whoops, i didn’t realize that affected the r+.

Given this, 

committed http://trac.webkit.org/changeset/167960,

with fixes applied.