Bug 74117 – Split RenderLayer::paintLayer into overlay scrollbars vs normal path

• Status: NEW
• Component: Layout and Rendering

[Julien Chaffraix]

This should make the code more readable but also simplify the different branch as they don't need to know about each other anymore.

[Julien Chaffraix]

Created attachment 118444 [details]
WIP: Naming to be enhanced, needs to check overlay scrollbar painting.

[Julien Chaffraix]

Created attachment 118512 [details]
Proposed change 1: Better naming, tested to not break overlay scrollbars.

[Simon Fraser (smfr)]

Comment on attachment 118512 [details]
Proposed change 1: Better naming, tested to not break overlay scrollbars.

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

> Source/WebCore/rendering/RenderLayer.cpp:2694
> +    ASSERT(!overlapTestRequests);

Why not just assert in the caller, and not pass overlapTestRequests down?

> Source/WebCore/rendering/RenderLayer.cpp:2708
> +    // Now walk the sorted list of children with negative z-indices.
> +    paintList(m_negZOrderList, rootLayer, p, paintDirtyRect, paintBehavior, paintingRoot, region, overlapTestRequests, paintFlags);
> +
> +    // Paint any child layers that have overflow.
> +    paintList(m_normalFlowList, rootLayer, p, paintDirtyRect, paintBehavior, paintingRoot, region, overlapTestRequests, paintFlags);
> +
> +    // Now walk the sorted list of children with positive z-indices.
> +    paintList(m_posZOrderList, rootLayer, p, paintDirtyRect, paintBehavior, paintingRoot, region, overlapTestRequests, paintFlags);

I don't think the comments are useful here.

> Source/WebCore/rendering/RenderLayer.cpp:2710
> +    // Paint the overlay scrollbar.

...ff there is one

> Source/WebCore/rendering/RenderLayer.h:586
>      void paintLayer(RenderLayer* rootLayer, GraphicsContext*, const LayoutRect& paintDirtyRect,
>                      PaintBehavior, RenderObject* paintingRoot, RenderRegion* = 0, OverlapTestRequestMap* = 0,
>                      PaintLayerFlags = 0);
> +    void paintOverlayScrollbarsInLayer(RenderLayer* rootLayer, GraphicsContext*,
> +                                       const LayoutRect& paintDirtyRect, PaintBehavior,
> +                                       RenderObject* paintingRoot, RenderRegion*, OverlapTestRequestMap*,
> +                                       PaintLayerFlags);
> +
> +    void paintLayerWithoutOverlayScrollbars(RenderLayer* rootLayer, GraphicsContext*,
> +                                            const LayoutRect& paintDirtyRect, PaintBehavior,
> +                                            RenderObject* paintingRoot, RenderRegion*, OverlapTestRequestMap*,
> +                                            PaintLayerFlags);
> +

I think the plethora of paintLayer calls is getting confusing.

paintLayer() is really paintLayerIncludingDescendants(), or maybe just paintLayers().
paintOverlayScrollbarsInLayer() should be something like paintLayersOverlayScrollbars()
paintLayerWithoutOverlayScrollbars() should be something like paintContentsofLayers()

[Julien Chaffraix]

Comment on attachment 118512 [details]
Proposed change 1: Better naming, tested to not break overlay scrollbars.

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

>> Source/WebCore/rendering/RenderLayer.cpp:2694
>> +    ASSERT(!overlapTestRequests);
> 
> Why not just assert in the caller, and not pass overlapTestRequests down?

No particular reason apart from having the same arguments for all the paint* methods. Looks like a good change.

>> Source/WebCore/rendering/RenderLayer.cpp:2708
>> +    paintList(m_posZOrderList, rootLayer, p, paintDirtyRect, paintBehavior, paintingRoot, region, overlapTestRequests, paintFlags);
> 
> I don't think the comments are useful here.

Fine enough.

>> Source/WebCore/rendering/RenderLayer.cpp:2710
>> +    // Paint the overlay scrollbar.
> 
> ...ff there is one

Will be updated.

>> Source/WebCore/rendering/RenderLayer.h:586
>> +
> 
> I think the plethora of paintLayer calls is getting confusing.
> 
> paintLayer() is really paintLayerIncludingDescendants(), or maybe just paintLayers().
> paintOverlayScrollbarsInLayer() should be something like paintLayersOverlayScrollbars()
> paintLayerWithoutOverlayScrollbars() should be something like paintContentsofLayers()

I am OK with the new names (the 2nd could be improved as we also paint the depth, the transparency layers and the clipping but I don't have a better name).

I think the plethora of names reflects that RenderLayer is now used for way too many things and it is time for us to think about splitting it into more manageable components.

[Julien Chaffraix]

Created attachment 118679 [details]
Proposed change 2: Followed Simon's suggestions.

[Julien Chaffraix]

Comment on attachment 118679 [details]
Proposed change 2: Followed Simon's suggestions.

The patch has rotten (paintLayer was split in the meantime), I would need to update it to ToT

[Simon Fraser (smfr)]

Sorry, that was me.