Bug 97365 – ScrollingTreeState needs to be a tree of nodes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

This is a step toward getting pages with fixed position to be able to be scrolled by the scrolling thread. In order to do that, fixed layers will have to be represented by nodes in the ScrollingTree…currently, the scrolling tree only ever has one node, so we have to do a little code restructuring to get it into a state where is can expect and handle multiple nodes. This is a first step along that path.

This patch specifically looks at ScrollingTreeState. Right now, the ScrollingTreeState attempts to contain all of the state information needed for the whole scrolling tree in one object. But in the future when there are multiple nodes in the scrolling tree, a single state object will not be sufficient. ScrollingState should also be represented by a tree. Technically, a HashTable would work too, but a tree will be better suited to communicating parent/child/sibling relationships for the scroll layers over to the ScrollingTree. This patch makes scrolling state into a tree. The old ScrollingTreeState class has become the ScrollingStateScrollingNode class since the majority of the class represents scroll state that is specific to ScrollableAreas and will not be applicable to fixed or sticky layers.

[Beth Dakin]

Created attachment 165204 [details]
Patch

[Anders Carlsson]

Comment on attachment 165204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165204&action=review

> Source/WebCore/page/scrolling/ScrollingStateNode.h:42
> +class ScrollingStateNode : public ThreadSafeRefCounted<ScrollingStateNode> {

I don't think this needs to be ref counted; using WTF_MAKE_NONCOPYABLE and use OwnPtr instead of RefPtr.

[Beth Dakin]

Created attachment 165245 [details]
Patch

Thanks, Anders! Here is a patch that addresses Anders' comments and also hopefully will apply to the ews bots.

[WebKit Review Bot]

Attachment 165245 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/scrolling/ScrollingTree.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:35:  The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sami Kyöstilä]

Comment on attachment 165245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165245&action=review

> Source/WebCore/page/scrolling/ScrollingStateNode.h:57
> +    void setScrollLayer(PlatformLayer*);

It seems weird to have these two variants share the same name. Do you really need both?

> Source/WebCore/page/scrolling/ScrollingStateTree.h:49
> +// ScrollingStateFixedNode and ScrollingStateStickyNode for fixed positioned objects and sticky positoned

Typo: positioned

> Source/WebCore/page/scrolling/ScrollingStateTree.h:59
> +     // Copies the current tree state and clears the changed properties mask in the original.

Indentation seems off here and after "private" below.

[Simon Fraser (smfr)]

Comment on attachment 165245 [details]
Patch

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->shouldUpdateScrollLayerPositionOnMainThread())
> +    , 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?

[Beth Dakin]

(In reply to comment #5)
> (From update of attachment 165245 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165245&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingStateNode.h:57
> > +    void setScrollLayer(PlatformLayer*);
> 
> It seems weird to have these two variants share the same name. Do you really need both?
> 

I do think having both is the easiest way. I will think abut it a bit more though.

> > Source/WebCore/page/scrolling/ScrollingStateTree.h:49
> > +// ScrollingStateFixedNode and ScrollingStateStickyNode for fixed positioned objects and sticky positoned
> 
> Typo: positioned
> 
> > Source/WebCore/page/scrolling/ScrollingStateTree.h:59
> > +     // Copies the current tree state and clears the changed properties mask in the original.
> 
> Indentation seems off here and after "private" below.

Thanks for catching these. Fixed!

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 165245 [details])
> 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->shouldUpdateScrollLayerPositionOnMainThread())
> > +    , 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?

Simon and I discussed his comments in person. The first two comments are definitely valid and they are generating some changes to the cloning code. The third comment is not actually an issue…the diff is just confusing because I moved files in subversion and then changed them, so the diff makes it look like duplicated code if you're not reading it very closely, but it is not in fact duplicated.

[Beth Dakin]

Created attachment 165511 [details]
Patch

Okay, the function is a bit complex, but I believe that this patch gets to cloning right.

[Beth Dakin]

Thanks, Simon! Committed changed with: http://trac.webkit.org/changeset/130091

[Beth Dakin]

Attempted a GTK build fix with http://trac.webkit.org/changeset/130104

[Alexey Proskuryakov]

This caused a regression, bug 98182.