Bug 91502 – Avoid repainting in RenderLayer::scrollTo where possible.

• Status: NEW
• Component: Layout and Rendering

Currently, we schedule a full repaint in scrollTo. This is unnecessary if every layer that moves is composited, and every child object is a layer.

Created attachment 152764 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 152764 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1669
> +    const bool movedNonCompositedLayer = updateLayerPositionsAfterScroll();

const bool makes no sense as a return value.

> Source/WebCore/rendering/RenderLayer.cpp:1701
> +    for (RenderObject* child = renderer()->firstChild(); child; child = child->nextSibling())
> +        hasNonLayerChildObject |= !child->hasLayer();

I don't understand why you're checking for non-layer children.

> Source/WebCore/rendering/RenderLayer.cpp:1705
> +    const bool needsFullRepaint = movedNonCompositedLayer || hasNonLayerChildObject;

We don't tent to use const for local variables.

Created attachment 152799 [details]
Patch

(In reply to comment #2)
> (From update of attachment 152764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152764&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1669
> > +    const bool movedNonCompositedLayer = updateLayerPositionsAfterScroll();
> 
> const bool makes no sense as a return value.
It's just a bool now.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1701
> > +    for (RenderObject* child = renderer()->firstChild(); child; child = child->nextSibling())
> > +        hasNonLayerChildObject |= !child->hasLayer();
> 
> I don't understand why you're checking for non-layer children.
I hope to skip repainting if the only things that move are composited layers.
The return value from updateLayerPositionsAfterScroll() just tells us if any
of the layers we moved weren't composited. We also need to know if we could have
moved something that isn't a layer at all. If a non-layer changes position due
to the scroll, compositing won't save us; we really do need to repaint it in the
new location (and repaint the old location). We could potentially be clever and
only paint the affected regions, but I decided to stick with the old logic and
repaint everything if any repainting might be required.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1705
> > +    const bool needsFullRepaint = movedNonCompositedLayer || hasNonLayerChildObject;
> 
> We don't tent to use const for local variables.
Removed the const.

[James Robinson]

Comment on attachment 152799 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests. Correctness should be covered by existing scroll tests.

To make a change like this we need to have really excellent test coverage.  If existing tests really do cover this, can you list the tests that do cover it here?  For instance, if you leave out or disable some of the checks you are adding which tests fail?  Which tests do we have to make sure we don't introduce unnecessary new repaints?

[Eric Seidel (no email)]

Comment on attachment 152799 [details]
Patch

Our repaint code is incredibly fragil and poorly tested.  Please add a test.