Bug 97365 - ScrollingTreeState needs to be a tree of nodes
Summary: ScrollingTreeState needs to be a tree of nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-21 14:55 PDT by Beth Dakin
Modified: 2012-10-02 15:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (80.37 KB, patch)
2012-09-21 15:09 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (94.21 KB, patch)
2012-09-21 22:30 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (82.55 KB, patch)
2012-09-24 21:16 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-09-21 14:55:57 PDT
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.
Comment 1 Beth Dakin 2012-09-21 15:09:53 PDT
Created attachment 165204 [details]
Patch
Comment 2 Anders Carlsson 2012-09-21 15:14:35 PDT
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.
Comment 3 Beth Dakin 2012-09-21 22:30:41 PDT
Created attachment 165245 [details]
Patch

Thanks, Anders! Here is a patch that addresses Anders' comments and also hopefully will apply to the ews bots.
Comment 4 WebKit Review Bot 2012-09-21 22:33:56 PDT
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.
Comment 5 Sami Kyöstilä 2012-09-24 11:01:03 PDT
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.
Comment 6 Simon Fraser (smfr) 2012-09-24 14:22:00 PDT
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?
Comment 7 Beth Dakin 2012-09-24 14:47:30 PDT
(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!
Comment 8 Beth Dakin 2012-09-24 14:49:12 PDT
(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.
Comment 9 Beth Dakin 2012-09-24 21:16:17 PDT
Created attachment 165511 [details]
Patch

Okay, the function is a bit complex, but I believe that this patch gets to cloning right.
Comment 10 Beth Dakin 2012-10-01 16:06:45 PDT
Thanks, Simon! Committed changed with: http://trac.webkit.org/changeset/130091
Comment 11 Beth Dakin 2012-10-01 17:26:57 PDT
Attempted a GTK build fix with http://trac.webkit.org/changeset/130104
Comment 12 Alexey Proskuryakov 2012-10-02 15:42:28 PDT
This caused a regression, bug 98182.