Bug 64480 – Speed up RenderLayer::updateLayerPositions when scrolling

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Julien Chaffraix]

On a table where each cell has overflow: hidden, each cell has its own RenderLayer (has they have an overflow clip). Thus when scrolling, we spend a significant amount of time in RenderLayer::updateLayerPositions.

This time is mostly spend recalculating the repaint rectangles and the goal of this bug is to avoid doing a recalculation when it is not needed.

[Julien Chaffraix]

Created attachment 100708 [details]
Proposed fix 1: Use scroll delta to avoid some rect recomputation.

[Simon Fraser (smfr)]

See bug 57571

[Simon Fraser (smfr)]

Comment on attachment 100708 [details]
Proposed fix 1: Use scroll delta to avoid some rect recomputation.

View in context: https://bugs.webkit.org/attachment.cgi?id=100708&action=review

r- due to fixed position issue.

> Source/WebCore/rendering/RenderLayer.cpp:287
> +    if (scrollDelta && (renderer()->style()->position() == FixedPosition || m_visibleContentStatusDirty))
> +        scrollDelta = 0;

What if some ancestor is fixed position?

[Julien Chaffraix]

> > Source/WebCore/rendering/RenderLayer.cpp:287
> > +    if (scrollDelta && (renderer()->style()->position() == FixedPosition || m_visibleContentStatusDirty))
> > +        scrollDelta = 0;
> 
> What if some ancestor is fixed position?

Your question got me thinking about the ancestry / descendance in RenderLayer real impact on this change. I think we can have a better fix for that.
We cannot use the scrollDelta for fixed position layer but nothing guarantees that our child will have a fixed position so the decision to use the scrollDelta should be per layer not inherited as it is now. Let me test this and come back to you.

[Julien Chaffraix]

Created attachment 100857 [details]
Proposed fix 2: Don't inherit the scrollDelta disabled from our parent as it is not used.

[Simon Fraser (smfr)]

Comment on attachment 100857 [details]
Proposed fix 2: Don't inherit the scrollDelta disabled from our parent as it is not used.

View in context: https://bugs.webkit.org/attachment.cgi?id=100857&action=review

> Source/WebCore/rendering/RenderLayer.cpp:286
> +    bool canUseScrollDelta = scrollDelta && (renderer()->style()->position() == FixedPosition || m_visibleContentStatusDirty);

Shouldn't that be != FixedPosition?

I'm worried that you're making patches that obviously don't do the right thing, yet you don't have any new tests, or show any failing tests. This suggests that our test coverage is lacking, and that you should include tests with your change that broke with intermediate versions of the patch.

[Julien Chaffraix]

As discussed our test coverage is fairly inadequate (as it does not detect such a broken patch). Closing this bug WONTFIX as:
1) we need to increase our test coverage of layers
2) bug 57571 is a better option for fixing that