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

[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 167592: Patch
https://bugs.webkit.org/attachment.cgi?id=167592&action=review

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


> Source/WebCore/loader/HistoryController.cpp:131
> +	   Page* page = m_frame->page();
> +	   if (page && page->mainFrame() == m_frame) {
> +	       if (ScrollingCoordinator* scrollingCoordinator =
page->scrollingCoordinator())
> +		   scrollingCoordinator->frameViewRootLayerDidChange(view);
> +	   }

This is a bit mysterious here, and it's a shame to call into
ScrollingCoordinator from yet another file. What work does it need to do?

> Source/WebCore/page/FrameView.h:137
> +    uint64_t scrollLayerID() const;

Does this have to be public on FrameView? Can't FrameView pass its own ID into
the SC?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:153
> +

Blanky.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:473
> +    ASSERT(node->isScrollingStateScrollingNode());
> +    return toScrollingStateScrollingNode(node);

toScrollingStateScrollingNode will ASSERT for you; no need to ASSERT here too.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:92
> +    size_t size = m_children->size();
> +    for (size_t i = 0; i < size; ++i)
> +	   m_children->at(i)->removeChild(node);

Looks like you're iterating the array while mutating it here.

> Source/WebCore/rendering/RenderLayerBacking.cpp:972
> +static uint64_t generateScrollLayerID()
> +{
> +    static uint64_t uniqueScrollLayerID = 1;
> +    return uniqueScrollLayerID++;
> +}

I think the ID generation should be on ScrollingCoordinator. Calling could look
like:

m_id = scrollingCoordinator->attach(scrollingCoordinator->uniqueNodeID(),
someConstraintsData)

where attach returns the ID passed in, for convenience.

> Source/WebCore/rendering/RenderLayerBacking.cpp:978
> +void RenderLayerBacking::attachToScrollingCoordinator()
> +{
> +    if (!m_scrollLayerID)
> +	   m_scrollLayerID = generateScrollLayerID();
> +}

But this isn't attaching, it's just making an ID.