Bug 174097 – Introduce ScrollingTreeScrollingNodeDelegateIOS to share code between overflow and frame scrolling

• Component: Frames
• Status: RESOLVED FIXED

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

Attempt to move some code out of ScrollingTreeOverflowScrollingNodeIOS so that it can be used for frames.

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

Created attachment 314486 [details]
Part 1

First part. WKOverflowScrollViewDelegate can now be renamed/moved and more code could be extracted from ScrollingTreeOverflowScrollingNodeIOS.

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

Created attachment 314559 [details]
Patch

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

Created attachment 316538 [details]
Patch

Rebasing...

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

Created attachment 317226 [details]
Patch

Rebasing after r220261

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

Created attachment 317228 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 317228 [details]
Patch

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

I think this is on the right track.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:40
> +#if PLATFORM(IOS)
> +namespace WebKit {
> +class ScrollingTreeScrollingNodeDelegateIOS;
> +}
> +#endif

It's wrong for WebCore to know anything about WebKit, so you can't do this.

Maybe there should be a ScrollingTreeScrollingNodeDelegate base class in WebCore, which WebKit subclasses.

> Source/WebKit/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:145
> +    m_scrollingNodeDelegate.reset(new ScrollingTreeScrollingNodeDelegateIOS(*this));

We never use bare 'new'. This should use make_unique<>.

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

Created attachment 319855 [details]
Patch

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

Comment on attachment 317228 [details]
Patch

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

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:40
>> +#endif
> 
> It's wrong for WebCore to know anything about WebKit, so you can't do this.
> 
> Maybe there should be a ScrollingTreeScrollingNodeDelegate base class in WebCore, which WebKit subclasses.

OK, I've done that in the patch I just uploaded. The drawback is that now we need to make more non-public members of ScrollingTreeScrollingNode accessible to the UIProcess classes, here and in bug 174130 at least. I've done that by adding new members to ScrollingTreeScrollingNodeDelegate but maybe we want to make the corresponding members of ScrollingTreeScrollingNode public, I don't know. WDYT?

[Darin Adler]

Comment on attachment 319855 [details]
Patch

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

Looks OK.

I think I understand the goal, but this looks a bit awkward. Is this really a “delegate”? Why does the delegate need to access private members of ScrollingTreeScrollingNode?

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNodeDelegate.h:30
> +#include "IntRect.h"

Why? This file doesn’t use IntRect. It does use FloatPoint, though.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNodeDelegate.h:33
> +namespace WebCore {
> +class ScrollingTreeScrollingNode;

Missing blank line.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:64
> +        _scrollingTreeNodeDelegate = delegate;

How does the object lifetime work? What guarantees this delegate won’t be deallocated while WKOverflowScrollViewDelegate is still around and holding a pointer to it?

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:30
> +#include <WebCore/IntRect.h>

Why is this included here? This file does not use IntRect.

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

Created attachment 319887 [details]
Patch for landing

This patch addresses Darin's comments.

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

Comment on attachment 319855 [details]
Patch

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

Indeed, this class is really the "iOS scrolling logic" that will be shared between derived classes of ScrollingTreeScrollingNode for iOS: non-main frames and overflow nodes. In C++ that would be multiple inheritance but here I have to introduce this helper class to extract that "iOS scrolling logic". I'm not sure whether "delegate" is the right term and moreover protected members non-accessible to that helper class. As I said in comment 8, I wonder whether we just want instead to make more members of ScrollingTreeScrollingNode public.

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNodeDelegate.h:30
>> +#include "IntRect.h"
> 
> Why? This file doesn’t use IntRect. It does use FloatPoint, though.

Right, this should just be a "class FloatPoint". Done.

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNodeDelegate.h:33
>> +class ScrollingTreeScrollingNode;
> 
> Missing blank line.

Done.

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:64
>> +        _scrollingTreeNodeDelegate = delegate;
> 
> How does the object lifetime work? What guarantees this delegate won’t be deallocated while WKOverflowScrollViewDelegate is still around and holding a pointer to it?

The ScrollingTreeScrollingNodeDelegateIOS has the same lifetime as the node and WKOverflowScrollViewDelegate is released in the node's destructor. I've modified the node's destructor to ensure that WKOverflowScrollViewDelegate is destroyed before ScrollingTreeScrollingNodeDelegateIOS.

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:30
>> +#include <WebCore/IntRect.h>
> 
> Why is this included here? This file does not use IntRect.

Done. I've just put a "class FloatPoint". (Not sure why I used a header...)

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

Created attachment 320009 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320009 [details]
Patch for landing

Clearing flags on attachment: 320009

Committed r221669: <http://trac.webkit.org/changeset/221669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694163>