[webkit-reviews] review denied: [Bug 97365] ScrollingTreeState needs to be a tree of nodes : [Attachment 165245] 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 97365: ScrollingTreeState needs to be a tree of nodes
https://bugs.webkit.org/show_bug.cgi?id=97365

Attachment 165245: Patch
https://bugs.webkit.org/attachment.cgi?id=165245&action=review

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


Going in the right direction, but I'd like to see clarification of ownership
when cloning nodes, and less duplication of state-related stuff.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:45
> +    : m_scrollingStateTree(nodeState->scrollingStateTree())

When cloning the node tree, does the clone belong ot the same
ScrollingStateTree or a new one?

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:48
> +    , m_parent(nodeState->parent())
> +    , m_firstChild(nodeState->firstChild())
> +    , m_nextSibling(nodeState->nextSibling())

It's a bit confusing to have a copy ctor that also copies over the
child/sibling pointers. What's the ownership model for those?

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:68
> +    , m_changedProperties(stateNode->changedProperties())
> +    , m_viewportRect(stateNode->viewportRect())
> +    , m_contentsSize(stateNode->contentsSize())
> +    , m_wheelEventHandlerCount(stateNode->wheelEventHandlerCount())
> +    ,
m_shouldUpdateScrollLayerPositionOnMainThread(stateNode->shouldUpdateScrollLaye
rPositionOnMainThread())
> +    , m_horizontalScrollElasticity(stateNode->horizontalScrollElasticity())
> +    , m_verticalScrollElasticity(stateNode->verticalScrollElasticity())
> +    ,
m_hasEnabledHorizontalScrollbar(stateNode->hasEnabledHorizontalScrollbar())
> +    ,
m_hasEnabledVerticalScrollbar(stateNode->hasEnabledVerticalScrollbar())
> +    , m_horizontalScrollbarMode(stateNode->horizontalScrollbarMode())
> +    , m_verticalScrollbarMode(stateNode->verticalScrollbarMode())
> +    , m_requestedScrollPosition(stateNode->requestedScrollPosition())
> +    , m_scrollOrigin(stateNode->scrollOrigin())

Duplicates a lot of ScrollingTreeState setup. Maybe the node can just have a
ScrollingTreeState member var?