Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk
Created attachment 377855 [details] Patch
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
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.
(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).
(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.
https://trac.webkit.org/changeset/249440/webkit
<rdar://problem/54987053>