[webkit-reviews] review denied: [Bug 97903] Make RenderLayerCompositor::requiresCompositingForScrollableFrame scrollbars agnostic : [Attachment 166312] patch 2 - actual

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 97903: Make RenderLayerCompositor::requiresCompositingForScrollableFrame
scrollbars agnostic
https://bugs.webkit.org/show_bug.cgi?id=97903

Attachment 166312: patch 2 - actual
https://bugs.webkit.org/attachment.cgi?id=166312&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166312&action=review


> Source/WebCore/page/FrameView.cpp:2794
> +bool FrameView::isScrollableChildFrameView()

I don't think this should have the "child frame" part. It's up to the caller to
determine whether it's a child frame. Just do the scrolling-related tests.

> Source/WebCore/page/FrameView.cpp:2834
> +    if (!parentFrameView())
> +	   return;
> +
> +    if (isScrollableChildFrameView()) {
> +	   parentFrameView()->addScrollableArea(this);
>	   return;
>      }
>  
> -    parentFrameView()->addScrollableArea(this);
> +    parentFrameView()->removeScrollableArea(this);

This fetches parentFrameView 3 times, and isScrollableChildFrameView() does so
once. You should use a local.