It's time to make the ScrollingTree capable of creating more than one node.
Created attachment 168563 [details] Patch
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.
Created attachment 168564 [details] Patch
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?
(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.
Committed change: http://trac.webkit.org/changeset/131490