Bug 174097 - Introduce ScrollingTreeScrollingNodeDelegateIOS to share code between overflow and frame scrolling
Summary: Introduce ScrollingTreeScrollingNodeDelegateIOS to share code between overflo...
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:
Depends on: 176273
Blocks: 149264 173833 174130
  Show dependency treegraph
 
Reported: 2017-07-03 08:26 PDT by Frédéric Wang (:fredw)
Modified: 2017-09-06 05:42 PDT (History)
9 users (show)

See Also:


Attachments
Part 1 (23.90 KB, patch)
2017-07-03 08:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (30.59 KB, patch)
2017-07-04 03:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.20 KB, patch)
2017-07-27 02:45 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.66 KB, patch)
2017-08-04 01:36 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.39 KB, patch)
2017-08-04 02:19 PDT, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (43.00 KB, patch)
2017-09-04 09:48 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for landing (43.48 KB, patch)
2017-09-05 01:56 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for landing (42.57 KB, patch)
2017-09-06 05:01 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

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-03 08:26:12 PDT
Attempt to move some code out of ScrollingTreeOverflowScrollingNodeIOS so that it can be used for frames.
Comment 1 Frédéric Wang (:fredw) 2017-07-03 08:51:34 PDT
Created attachment 314486 [details]
Part 1

First part. WKOverflowScrollViewDelegate can now be renamed/moved and more code could be extracted from ScrollingTreeOverflowScrollingNodeIOS.
Comment 2 Frédéric Wang (:fredw) 2017-07-04 03:42:58 PDT
Created attachment 314559 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2017-07-27 02:45:23 PDT
Created attachment 316538 [details]
Patch

Rebasing...
Comment 4 Frédéric Wang (:fredw) 2017-08-04 01:36:09 PDT
Created attachment 317226 [details]
Patch

Rebasing after r220261
Comment 5 Frédéric Wang (:fredw) 2017-08-04 02:19:41 PDT
Created attachment 317228 [details]
Patch
Comment 6 Simon Fraser (smfr) 2017-09-01 13:01:40 PDT
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<>.
Comment 7 Frédéric Wang (:fredw) 2017-09-04 09:48:07 PDT
Created attachment 319855 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2017-09-04 09:53:13 PDT
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?
Comment 9 Darin Adler 2017-09-04 20:20:00 PDT
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.
Comment 10 Frédéric Wang (:fredw) 2017-09-05 01:56:05 PDT
Created attachment 319887 [details]
Patch for landing

This patch addresses Darin's comments.
Comment 11 Frédéric Wang (:fredw) 2017-09-05 02:16:28 PDT
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...)
Comment 12 Frédéric Wang (:fredw) 2017-09-06 05:01:31 PDT
Created attachment 320009 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2017-09-06 05:42:50 PDT
Comment on attachment 320009 [details]
Patch for landing

Clearing flags on attachment: 320009

Committed r221669: <http://trac.webkit.org/changeset/221669>
Comment 14 WebKit Commit Bot 2017-09-06 05:42:52 PDT
All reviewed patches have been landed.  Closing bug.