[Webkit-unassigned] [Bug 132222] Don't snapshot plugins that are overlaid with images.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132222


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #230351|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2014-04-29 09:13:59 PST ---
(From update of attachment 230351)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.