[webkit-reviews] review denied: [Bug 98457] ScrollingStateNodes should be referenced via IDs on RenderLayerBacking : [Attachment 167215] Patch that should build

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 98457: ScrollingStateNodes should be referenced via IDs on
RenderLayerBacking
https://bugs.webkit.org/show_bug.cgi?id=98457

Attachment 167215: Patch that should build
https://bugs.webkit.org/attachment.cgi?id=167215&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167215&action=review


> Source/WebCore/dom/Document.cpp:4064
>	       v->cacheCurrentScrollPosition();
> -	       if (page() && page()->mainFrame() == m_frame)
> +	       if (page && page->mainFrame() == m_frame) {
>		   v->resetScrollbarsAndClearContentsSize();
> -	       else
> +		   if (ScrollingCoordinator* scrollingCoordinator =
page->scrollingCoordinator())
> +		       scrollingCoordinator->clearStateTree();
> +	       } else

Maybe this should be done in a separate patch? This patch is big enough
already.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:255
> +    // ensureRootStateNodeForFrameView() can fail if, for example, the
FrameView does not have a
> +    // RenderLayerBacking. If that is the case, then we should not do any
more work here.

The comment would be more useful if it said when this might happen (to allow a
developer to reproduce the scenario).

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:462
> +static RenderLayerBacking* backingForFrameView(FrameView* frameView)

I'm not sure why SC ever needs to know about RLB.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:503
> +void ScrollingCoordinator::detachFromStateTree(RenderLayerBacking* backing)

Why not just pass the ID in here?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:549
> +    if (!rootLayerID) {
> +	   rootLayerID = generateScrollLayerID();
> +	   backing->setScrollLayerID(rootLayerID);
> +    }

I think this code should live in RLB.

> Source/WebCore/rendering/RenderLayer.cpp:4462
> +	   if (isRootLayer() && compositor()->scrollLayer()) {
> +	       if (Frame* frame = renderer()->frame()) {
> +		   if (Page* page = frame->page()) {
> +		       if (ScrollingCoordinator* scrollingCoordinator =
page->scrollingCoordinator())
> +			  
scrollingCoordinator->frameViewRootLayerDidChange(frame->view());
> +		   }
> +	       }
> +	   }

Seems like an odd place to put this. I think we should try to keep all of the
calls into SC in RLC.