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.
<rdar://problem/16653536>
Created attachment 230248 [details] patch
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 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?
(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
Created attachment 230351 [details] patch
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 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?
whoops, i didn’t realize that affected the r+.
> > > 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 :).
Created attachment 230404 [details] patch
(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.