Bug 174130 – Move more code from ScrollingStateOverflowScrollingNodeIOS to ScrollingTreeScrollingNodeDelegate

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 314574 [details]
Patch

This patch applies on top of bugs 174134 and 174097.

[Frédéric Wang (:fredw)]

Created attachment 316539 [details]
Patch

Rebasing... This patch still applies after bug 174097.

[Frédéric Wang (:fredw)]

Created attachment 317241 [details]
Patch

Rebasing...

[Radar WebKit Bug Importer]

<rdar://problem/34215520>

[Frédéric Wang (:fredw)]

Created attachment 319856 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 319891 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 320011 [details]
Patch

[Simon Fraser (smfr)]

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)]

Created attachment 320162 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320162 [details]
Patch for landing

Clearing flags on attachment: 320162

Committed r221753: <http://trac.webkit.org/changeset/221753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

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)]

Oh right, never mind :)