[webkit-reviews] review granted: [Bug 174097] Introduce ScrollingTreeScrollingNodeDelegateIOS to share code between overflow and frame scrolling : [Attachment 319855] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 174097: Introduce ScrollingTreeScrollingNodeDelegateIOS to share code
between overflow and frame scrolling
https://bugs.webkit.org/show_bug.cgi?id=174097

Attachment 319855: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 319855
  --> https://bugs.webkit.org/attachment.cgi?id=319855
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/ScrollingTreeOverflowScrollingNodeI
OS.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/ScrollingTreeScrollingNodeDelegateI
OS.h:30
> +#include <WebCore/IntRect.h>

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