[webkit-reviews] review denied: [Bug 102543] ScrollingCoordinator::hasVisibleSlowRepaintFixedObject() should exclude out-of-view fixed position elements : [Attachment 178122] Patch (independent with the blocking bugs)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 102543: ScrollingCoordinator::hasVisibleSlowRepaintFixedObject() should
exclude out-of-view fixed position elements
https://bugs.webkit.org/show_bug.cgi?id=102543

Attachment 178122: Patch (independent with the blocking bugs)
https://bugs.webkit.org/attachment.cgi?id=178122&action=review

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


> Source/WebCore/rendering/RenderLayer.cpp:3130
> +	   if (!isComposited() && isStackingContext() &&
m_renderer->style()->position() == FixedPosition && m_renderer->container() ==
m_renderer->view())

(isStackingContext() && m_renderer->style()->position() == FixedPosition &&
m_renderer->container() == m_renderer->view()) should be packaged up into a
function that's called from both RLC and this code.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984
> -	   if (!viewBounds.intersects(layerBounds))
> +	   if (!viewBounds.intersects(layerBounds)) {
> +	       frameView->removeViewportConstrainedObject(renderer);
>	       return false;
> +	   }
>      }
>  
> +    frameView->addViewportConstrainedObject(renderer);

I don't like the fact that this code is messing with the FrameView's
viewportConstrainedObjects. I think you should track whether there are any
ignorable Fixed objects in RLC.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1988
> +    if (Settings* settings = m_renderView->document()->settings())
> +	   if (!settings->acceleratedCompositingForFixedPositionEnabled())
> +	       return false;

Why is this test at the end?