[webkit-reviews] review granted: [Bug 132222] Don't snapshot plugins that are overlaid with images. : [Attachment 230351] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 132222: Don't snapshot plugins that are overlaid with images.
https://bugs.webkit.org/show_bug.cgi?id=132222

Attachment 230351: patch
https://bugs.webkit.org/attachment.cgi?id=230351&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.