Bug 201403 – Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk

• Status: RESOLVED
• Resolution: FIXED
• Component: Compositing

[Simon Fraser (smfr)]

Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk

[Simon Fraser (smfr)]

Created attachment 377855 [details]
Patch

[Daniel Bates]

Comment on attachment 377855 [details]
Patch

This patch looks good. In my opinion it would benefit by using more references and less pointers because:
1. I do not see null checks so the code looks to be written assuming non-nullness
2. Because of (1) a newcomer to this code may think RenderLayer can be null and add null checks or asserts when they are not necessary

[Antti Koivisto]

Comment on attachment 377855 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.h:519
> -    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&) const;
> +    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&, const RenderLayer* compositingAncestor) const;
>  
>      // Returns the ScrollingNodeID which acts as the parent for children.
> -    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> +    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, const RenderLayer* compositingAncestor, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);

It might be better to pass in UpdateBackingTraversalState& instead of RenderLayer* to these functions. It makes clear in which context these functions are used in and potentially allows caching more things in future.

[Simon Fraser (smfr)]

(In reply to Daniel Bates from comment #2)
> Comment on attachment 377855 [details]
> Patch
> 
> This patch looks good. In my opinion it would benefit by using more
> references and less pointers because:
> 1. I do not see null checks so the code looks to be written assuming
> non-nullness

compositedAncestor can be null (e.g. for the root layer).

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 377855 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377855&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:519
> > -    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&) const;
> > +    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&, const RenderLayer* compositingAncestor) const;
> >  
> >      // Returns the ScrollingNodeID which acts as the parent for children.
> > -    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> > +    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, const RenderLayer* compositingAncestor, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> 
> It might be better to pass in UpdateBackingTraversalState& instead of
> RenderLayer* to these functions. It makes clear in which context these
> functions are used in and potentially allows caching more things in future.

For now, I don't want UpdateBackingTraversalState to leak out of the tree walking code, but if it gains most stuff for caching, I can pass it around later.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/249440/webkit

[Radar WebKit Bug Importer]

<rdar://problem/54987053>