Bug 132222

Summary: Don't snapshot plugins that are overlaid with images.
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, jonlee, roger_fong, thorton, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch none

Description Roger Fong 2014-04-26 17:37:10 PDT
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.
Comment 1 Roger Fong 2014-04-26 17:38:00 PDT
<rdar://problem/16653536>
Comment 2 Roger Fong 2014-04-26 18:17:40 PDT
Created attachment 230248 [details]
patch
Comment 3 Roger Fong 2014-04-26 18:21:04 PDT
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.
Comment 4 Jon Lee 2014-04-27 20:34:13 PDT
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?
Comment 5 Roger Fong 2014-04-28 10:29:15 PDT
(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
Comment 6 Roger Fong 2014-04-28 19:28:43 PDT
Created attachment 230351 [details]
patch
Comment 7 Darin Adler 2014-04-29 09:13:59 PDT
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.
Comment 8 Jon Lee 2014-04-29 09:30:21 PDT
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?
Comment 9 Jon Lee 2014-04-29 09:31:12 PDT
whoops, i didn’t realize that affected the r+.
Comment 10 Roger Fong 2014-04-29 12:25:59 PDT
> 
> > 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 :).
Comment 11 Roger Fong 2014-04-29 12:48:08 PDT
Created attachment 230404 [details]
patch
Comment 12 Roger Fong 2014-04-29 15:12:06 PDT
(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.