[webkit-reviews] review granted: [Bug 133667] Don't snapshot offscreen plugins that would normally be considered primary plugins after they are moved in view : [Attachment 232752] patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 133667: Don't snapshot offscreen plugins that would normally be considered
primary plugins after they are moved in view
https://bugs.webkit.org/show_bug.cgi?id=133667

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

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232752&action=review


> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1687
> +    WebCore::HTMLPlugInImageElement* plugInImageElement =
toHTMLPlugInImageElement(m_pluginElement.get());

I think there's no need for the WebCore:: here. Is this always safe or should
you check the type?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1706
> +	   snapshotFound = snapshotImage &&
!isAlmostSolidColor(toBitmapImage(snapshotImage.get()));
>  
>  #if PLATFORM(COCOA)
>	   unsigned maximumSnapshotRetries = frame() ?
frame()->settings().maximumPlugInSnapshotAttempts() : 0;
> -	   if (snapshotImage &&
isAlmostSolidColor(toBitmapImage(snapshotImage.get())) &&
m_countSnapshotRetries < maximumSnapshotRetries) {
> +	   if (snapshotImage &&
isAlmostSolidColor(toBitmapImage(snapshotImage.get())) &&
m_countSnapshotRetries < maximumSnapshotRetries && !plugInCameOnScreen) {

why are we checking isAlmostSolidColor twice in quick succession here? can the
if() use snapshotFound?

> Source/WebKit2/WebProcess/Plugins/PluginView.h:278
> +    bool m_didPlugInStartOffScreen;

maybe pack this better, next to other bools? I dunno.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4466
> +bool WebPage::plugInIntersectsSearchRect(HTMLPlugInImageElement*
plugInImageElement)

should this (and plugInIsPrimarySize) take references?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4485
> +    if (plugInRectRelativeToTopDocument.intersects(searchRect))
> +	   return true;
> +    return false;

Maybe just "return plugInRectRelativeToTopDocument.intersects(searchRect);"?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4488
> +bool WebPage::plugInIsPrimarySize(WebCore::HTMLPlugInImageElement
*plugInImageElement, unsigned &candidatePlugInArea)

star on the wrong side.