Bug 99254 – Make ScrollingTree an actual tree of nodes, and have it reflect the ScrollingStateTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

It's time to make the ScrollingTree capable of creating more than one node.

[Beth Dakin]

Created attachment 168563 [details]
Patch

[WebKit Review Bot]

Attachment 168563 [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:167:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 13 files


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

[Beth Dakin]

Created attachment 168564 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 168564 [details]
Patch

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

I'm starting to get nervous that none of this is covered by LayoutTests, and we've already had a couple of crashes in this code. I strongly suggest figuring out how to test this.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:147
> +void ScrollingTree::updateNodesFromStateNode(ScrollingStateNode* stateNode)

I'd call this updateTreeFromStateNode to make it clearer that it consults stateNode and its children.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:151
> +    HashMap<ScrollingNodeID, ScrollingTreeNode*>::const_iterator it = m_nodeMap.find(stateNode->scrollingNodeID());

You should make a typedef for HashMap<ScrollingNodeID, ScrollingTreeNode*>

> Source/WebCore/page/scrolling/ScrollingTree.cpp:175
> +            HashMap<ScrollingNodeID, ScrollingTreeNode*>::const_iterator it = m_nodeMap.find(stateNode->parent()->scrollingNodeID());
> +            if (it != m_nodeMap.end()) {
> +                ScrollingTreeNode* parent = it->value;
> +                newNode->setParent(parent);
> +                parent->appendChild(newNode.release());
> +            }

If we don't find the parent, what then? Is that an error? Should it assert?

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 168564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168564&action=review
> 
> I'm starting to get nervous that none of this is covered by LayoutTests, and we've already had a couple of crashes in this code. I strongly suggest figuring out how to test this.
> 

Yes, I do agree. I know Tim has a goal to get threaded scrolling turned on on a bot, and I have been holding out for that…but I agree, with the crashes, that is turning into a dubious strategy. 

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:147
> > +void ScrollingTree::updateNodesFromStateNode(ScrollingStateNode* stateNode)
> 
> I'd call this updateTreeFromStateNode to make it clearer that it consults stateNode and its children.
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:151
> > +    HashMap<ScrollingNodeID, ScrollingTreeNode*>::const_iterator it = m_nodeMap.find(stateNode->scrollingNodeID());
> 
> You should make a typedef for HashMap<ScrollingNodeID, ScrollingTreeNode*>
> 

Okay!

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:175
> > +            HashMap<ScrollingNodeID, ScrollingTreeNode*>::const_iterator it = m_nodeMap.find(stateNode->parent()->scrollingNodeID());
> > +            if (it != m_nodeMap.end()) {
> > +                ScrollingTreeNode* parent = it->value;
> > +                newNode->setParent(parent);
> > +                parent->appendChild(newNode.release());
> > +            }
> 
> If we don't find the parent, what then? Is that an error? Should it assert?

Yes, I think an assert is appropriate. I will add one.

[Beth Dakin]

Committed change: http://trac.webkit.org/changeset/131490