Summary: | Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
Component: | Compositing | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dbates, koivisto, simon.fraser, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2019-09-02 10:03:20 PDT
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. |