Bug 174130 - Move more code from ScrollingStateOverflowScrollingNodeIOS to ScrollingTreeScrollingNodeDelegate
Summary: Move more code from ScrollingStateOverflowScrollingNodeIOS to ScrollingTreeSc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 174097 174134
Blocks: 149264 173833
  Show dependency tree / graph
 
Reported: 2017-07-04 07:59 PDT by Frédéric Wang (:fredw)
Modified: 2017-09-13 11:33 PDT (History)
9 users (show)

See Also:

Attachments
Patch (26.25 KB, patch)
2017-07-04 07:59 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
174134+174097+174130.patch (57.80 KB, patch)
2017-07-04 11:18 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (31.53 KB, patch)
2017-07-04 11:39 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (31.65 KB, patch)
2017-07-27 02:46 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (31.67 KB, patch)
2017-08-04 07:31 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (33.18 KB, patch)
2017-09-04 09:48 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (33.56 KB, patch)
2017-09-05 03:25 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2017-09-06 05:49 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Patch for landing (33.17 KB, patch)
2017-09-07 13:03 PDT, Frédéric Wang (:fredw) 		no flags Details | Formatted Diff | Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-07-04 07:59:52 PDT 
Created attachment 314564 [details]
Patch

This is a follow-up of bug 174097. It seems we can move most of the logic to ScrollingTreeScrollingNodeDelegate.

I'm attaching a patch doing that. ScrollingTreeOverflowScrollingNodeIOS::commitStateBeforeChildren could be simplified a bit more if ScrolledContentsLayer was on the ScrollingStateScrollingNode.
Comment 1 Frédéric Wang (:fredw) 2017-07-04 11:18:07 PDT 
Created attachment 314572 [details]
174134+174097+174130.patch
Comment 2 Frédéric Wang (:fredw) 2017-07-04 11:39:38 PDT 
Created attachment 314574 [details]
Patch

This patch applies on top of bugs 174134 and 174097.
Comment 3 Frédéric Wang (:fredw) 2017-07-27 02:46:24 PDT 
Created attachment 316539 [details]
Patch

Rebasing... This patch still applies after bug 174097.
Comment 4 Frédéric Wang (:fredw) 2017-08-04 07:31:21 PDT 
Created attachment 317241 [details]
Patch

Rebasing...
Comment 5 Radar WebKit Bug Importer 2017-09-01 14:22:54 PDT 
<rdar://problem/34215520>
Comment 6 Frédéric Wang (:fredw) 2017-09-04 09:48:31 PDT 
Created attachment 319856 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2017-09-05 03:25:37 PDT 
Created attachment 319891 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2017-09-06 05:49:15 PDT 
Created attachment 320011 [details]
Patch
Comment 9 Simon Fraser (smfr) 2017-09-07 11:38:43 PDT 
Comment on attachment 320011 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:63
> +    void commitStateBeforeChildren(const WebCore::ScrollingStateScrollingNode& scrollingStateNode);
> +    void commitStateAfterChildren(const WebCore::ScrollingStateScrollingNode& scrollingStateNode);

No need to name scrollingStateNode.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:66
> +    void setScrollLayerPosition(const WebCore::FloatPoint& scrollPosition);

No need to name scrollPosition

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:73
>      bool m_updatingFromStateNode;

{ false }

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:177
> +                m_scrollViewDelegate = adoptNS([[WKScrollingNodeScrollViewDelegate alloc] initWithScrollingTreeNodeDelegate:this]);

I wonder if this class could also be the WKScrollingNodeScrollViewDelegate, but that could be done later.
Comment 10 Frédéric Wang (:fredw) 2017-09-07 13:03:34 PDT 
Created attachment 320162 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-09-07 13:21:16 PDT 
Comment on attachment 320162 [details]
Patch for landing

Clearing flags on attachment: 320162

Committed r221753: <http://trac.webkit.org/changeset/221753>
Comment 12 WebKit Commit Bot 2017-09-07 13:21:18 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 13 Frédéric Wang (:fredw) 2017-09-13 07:44:49 PDT 
Comment on attachment 320011 [details]
Patch

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

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:177
>> +                m_scrollViewDelegate = adoptNS([[WKScrollingNodeScrollViewDelegate alloc] initWithScrollingTreeNodeDelegate:this]);
> 
> I wonder if this class could also be the WKScrollingNodeScrollViewDelegate, but that could be done later.

I'm not sure I understand this comment. WKScrollingNodeScrollViewDelegate is an objective-C object while ScrollingTreeScrollingNodeDelegateIOS is a C++, and I don't know how we can "merge" the two together?
Comment 14 Simon Fraser (smfr) 2017-09-13 11:33:52 PDT 
Oh right, never mind :)