Bug 199348 - Use separate variables for moving and stationary scrolling relationships in RemoteLayerTreeNode
Summary: Use separate variables for moving and stationary scrolling relationships in R...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-30 00:17 PDT by Antti Koivisto
Modified: 2019-06-30 22:53 PDT (History)
10 users (show)

See Also:


Attachments
patch (11.18 KB, patch)
2019-06-30 00:31 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (17.76 KB, patch)
2019-06-30 12:12 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (17.66 KB, patch)
2019-06-30 21:42 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (17.74 KB, patch)
2019-06-30 22:09 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2019-06-30 00:17:27 PDT
A layer can have only one acting scroll parent. Not using a vector for that case makes the code clearer.
Comment 1 Antti Koivisto 2019-06-30 00:31:16 PDT
Created attachment 373190 [details]
patch
Comment 2 Antti Koivisto 2019-06-30 12:12:48 PDT
Created attachment 373195 [details]
patch
Comment 3 Darin Adler 2019-06-30 13:04:05 PDT
Comment on attachment 373195 [details]
patch

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

While Iā€™m not a real expert on this code, this looks good to me.

> Source/WebCore/page/scrolling/ScrollingTree.h:187
> +    HashSet<RefPtr<ScrollingTreeOverflowScrollProxyNode>> m_activeOverflowScrollProxyNodes;
> +    HashSet<RefPtr<ScrollingTreePositionedNode>> m_activePositionedNodes;

A number of places elsewhere use HashSet<Ref<>> rather than HashSet<RefPtr<>>. Is there a reason to prefer RefPtr here?

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:134
> +                return (WKChildScrollView *)actingParent->uiView();

Not new, but: Would be nice to have a type checking cast like downcast<> here for clarity/assertion. Also unclear to me how solid the guarantee behind this typecast is.
Comment 4 Antti Koivisto 2019-06-30 21:42:07 PDT
Created attachment 373210 [details]
patch
Comment 5 Antti Koivisto 2019-06-30 22:09:05 PDT
Created attachment 373211 [details]
patch
Comment 6 Antti Koivisto 2019-06-30 22:09:38 PDT
> A number of places elsewhere use HashSet<Ref<>> rather than
> HashSet<RefPtr<>>. Is there a reason to prefer RefPtr here?

No reason, I just didn't remember it works.
Comment 7 WebKit Commit Bot 2019-06-30 22:52:37 PDT
Comment on attachment 373211 [details]
patch

Clearing flags on attachment: 373211

Committed r246962: <https://trac.webkit.org/changeset/246962>
Comment 8 WebKit Commit Bot 2019-06-30 22:52:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-06-30 22:53:20 PDT
<rdar://problem/52441382>