Bug 174130

Summary:

Move more code from ScrollingStateOverflowScrollingNodeIOS to ScrollingTreeScrollingNodeDelegate

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

Frames

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cmarcelo, commit-queue, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

174097, 174134

Bug Blocks:

149264, 173833

Attachments:

Description	Flags
Patch	none
174134+174097+174130.patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Frédéric Wang (:fredw)

Reported 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.

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 attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2017-07-04 11:18:07 PDT

Created attachment 314572 [details] 174134+174097+174130.patch

Frédéric Wang (:fredw)

Comment 2 2017-07-04 11:39:38 PDT

Created attachment 314574 [details] Patch This patch applies on top of bugs 174134 and 174097.

Frédéric Wang (:fredw)

Comment 3 2017-07-27 02:46:24 PDT

Created attachment 316539 [details] Patch Rebasing... This patch still applies after bug 174097.

Frédéric Wang (:fredw)

Comment 4 2017-08-04 07:31:21 PDT

Created attachment 317241 [details] Patch Rebasing...

Radar WebKit Bug Importer

Comment 5 2017-09-01 14:22:54 PDT

<rdar://problem/34215520>

Frédéric Wang (:fredw)

Comment 6 2017-09-04 09:48:31 PDT

Created attachment 319856 [details] Patch

Frédéric Wang (:fredw)

Comment 7 2017-09-05 03:25:37 PDT

Created attachment 319891 [details] Patch

Frédéric Wang (:fredw)

Comment 8 2017-09-06 05:49:15 PDT

Created attachment 320011 [details] Patch

Simon Fraser (smfr)

Comment 9 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.

Frédéric Wang (:fredw)

Comment 10 2017-09-07 13:03:34 PDT

Created attachment 320162 [details] Patch for landing

WebKit Commit Bot

Comment 11 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>

WebKit Commit Bot

Comment 12 2017-09-07 13:21:18 PDT

All reviewed patches have been landed. Closing bug.

Frédéric Wang (:fredw)

Comment 13 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?

Simon Fraser (smfr)

Comment 14 2017-09-13 11:33:52 PDT

Oh right, never mind :)

Note You need to log in before you can comment on or make changes to this bug.