Bug 105652 – RenderLayerCompositor should let ScrollingCoordinator update main thread scrolling reasons after change of layers

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

[Xianzhu Wang]

On Chromium ScrollingCoordinator will force main thread scrolling if there are any viewport-constrained objects not composited. However, sometimes (e.g. when the page is loading) the main thread scrolling reasons are not updated immediately after RLC promotes some fixed position elements to composited layers. This causes slow scrolling before the main thread scrolling reasons are correctly updated.

crbug.com/167312 describes the slow scrolling case.

[Xianzhu Wang]

Created attachment 180578 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 180578 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:473
> +        if (scrollingCoordinator && m_fixedPositionLayerNotCompositedReasonMap != originalFixedPositionLayerNotCompositedReasonMap)

I don't really like m_fixedPositionLayerNotCompositedReasonMap, and the fact that you're comparing two HashMaps here.

[Xianzhu Wang]

(In reply to comment #2)
> (From update of attachment 180578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180578&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:473
> > +        if (scrollingCoordinator && m_fixedPositionLayerNotCompositedReasonMap != originalFixedPositionLayerNotCompositedReasonMap)
> 
> I don't really like m_fixedPositionLayerNotCompositedReasonMap, and the fact that you're comparing two HashMaps here.

(I thought I posted the following on last Friday, but just found I made it lost somehow)

How about moving the reason information map into individual RenderLayers or RenderLayerModelObjects as a new field?

[Xianzhu Wang]

Created attachment 181770 [details]
Patch

[Build Bot]

Comment on attachment 181770 [details]
Patch

Attachment 181770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15758280

[Xianzhu Wang]

Created attachment 181789 [details]
Fix mac break

[Xianzhu Wang]

Simon, do you like the latest patch?

[Xianzhu Wang]

Kindly ping reviewers...

[Simon Fraser (smfr)]

Comment on attachment 181789 [details]
Fix mac break

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

Nice cleanup, thanks!

> Source/WebCore/rendering/RenderLayerCompositor.cpp:636
> +        if (layerChanged)
> +            if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator())
> +                scrollingCoordinator->frameViewFixedObjectsDidChange(m_renderView->frameView());

We put {} around multi-line conditional blocks like this.

[Xianzhu Wang]

Created attachment 182365 [details]
for landing

[Xianzhu Wang]

Comment on attachment 181789 [details]
Fix mac break

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:636
>> +                scrollingCoordinator->frameViewFixedObjectsDidChange(m_renderView->frameView());
> 
> We put {} around multi-line conditional blocks like this.

Done.

[WebKit Review Bot]

Comment on attachment 182365 [details]
for landing

Clearing flags on attachment: 182365

Committed r139461: <http://trac.webkit.org/changeset/139461>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.