Changeset 131238 in webkit

Timestamp:

Oct 12, 2012, 4:51:46 PM (13 years ago)

Author:

Beth Dakin

Message:

​https://bugs.webkit.org/show_bug.cgi?id=99211
When ScrollingStateNodes are destroyed, they should be removed
ScrollingCoordinator's HashMap

Reviewed by Sam Weinig.

This patch adds a new member variable to ScrollingStateTree. It's a
Vector of ScrollingNodeIDs. It will contain the IDs of nodes that
have been removed from the tree since the last time the tree was
committed.

• page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::ScrollingStateTree):

When we do commit, copy the Vector over into the cloned tree, and
then clear our own Vector.
(WebCore::ScrollingStateTree::commit):

Call didRemoveNode().
(WebCore::ScrollingStateTree::removeNode):

Append the removed node's id to the vector.
(WebCore::ScrollingStateTree::didRemoveNode):
(WebCore):

• page/scrolling/ScrollingStateTree.h:

(ScrollingStateTree):

Call didRemoveNode().

• page/scrolling/ScrollingStateNode.cpp:

(WebCore::ScrollingStateNode::removeChild):

Fix the FIXME!

• page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::detachFromStateTree):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ScrollingStateNode.cpp (modified) (1 diff)
• page/scrolling/ScrollingStateTree.cpp (modified) (3 diffs)
• page/scrolling/ScrollingStateTree.h (modified) (2 diffs)
• page/scrolling/mac/ScrollingCoordinatorMac.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-10-12  Beth Dakin  <bdakin@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=99211
+        When ScrollingStateNodes are destroyed, they should be removed 
+        ScrollingCoordinator's HashMap
+
+        Reviewed by Sam Weinig.
+
+        This patch adds a new member variable to ScrollingStateTree. It's a 
+        Vector of ScrollingNodeIDs. It will contain the IDs of nodes that 
+        have been removed from the tree since the last time the tree was 
+        committed.
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::ScrollingStateTree):
+
+        When we do commit, copy the Vector over into the cloned tree, and 
+        then clear our own Vector.
+        (WebCore::ScrollingStateTree::commit):
+
+        Call didRemoveNode().
+        (WebCore::ScrollingStateTree::removeNode):
+
+        Append the removed node's id to the vector.
+        (WebCore::ScrollingStateTree::didRemoveNode):
+        (WebCore):
+        * page/scrolling/ScrollingStateTree.h:
+        (ScrollingStateTree):
+
+        Call didRemoveNode().
+        * page/scrolling/ScrollingStateNode.cpp:
+        (WebCore::ScrollingStateNode::removeChild):
+
+        Fix the FIXME!
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::detachFromStateTree):
+
 2012-10-12  Brady Eidson  <beidson@apple.com>
 

trunk/Source/WebCore/page/scrolling/ScrollingStateNode.cpp

@@ -83 +83 @@
 
     if (size_t index = m_children->find(node)) {
+        m_scrollingStateTree->didRemoveNode(node->scrollingNodeID());
         m_children->remove(index);
         return;

trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp

@@ -47 +47 @@
 PassOwnPtr<ScrollingStateTree> ScrollingStateTree::commit()
 {
-    OwnPtr<ScrollingStateTree> treeState = ScrollingStateTree::create();
-    treeState->setRootStateNode(static_pointer_cast<ScrollingStateScrollingNode>(m_rootStateNode->cloneAndResetNode()));
+    // This function clones and resets the current state tree, but leaves the tree structure intact. 
+    OwnPtr<ScrollingStateTree> treeStateClone = ScrollingStateTree::create();
+    treeStateClone->setRootStateNode(static_pointer_cast<ScrollingStateScrollingNode>(m_rootStateNode->cloneAndResetNode()));
+
+    // Copy the IDs of the nodes that have been removed since the last commit into the clone.
+    treeStateClone->m_nodesRemovedSinceLastCommit.swap(m_nodesRemovedSinceLastCommit);
 
     // Now the clone tree has changed properties, and the original tree does not.
-    treeState->setHasChangedProperties(true);
+    treeStateClone->m_hasChangedProperties = true;
     m_hasChangedProperties = false;
 
-    return treeState.release();
+    return treeStateClone.release();
 }
 
@@ -62 +66 @@
 
     if (node == m_rootStateNode) {
+        didRemoveNode(m_rootStateNode->scrollingNodeID());
         m_rootStateNode = 0;
         return;
@@ -67 +72 @@
 
     m_rootStateNode->removeChild(node);
+}
+
+void ScrollingStateTree::didRemoveNode(ScrollingNodeID nodeID)
+{
+    m_nodesRemovedSinceLastCommit.append(nodeID);
 }
 

trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h

@@ -62 +62 @@
 
     void removeNode(ScrollingStateNode*);
+    void didRemoveNode(ScrollingNodeID);
+    const Vector<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
 
     void rootLayerDidChange();
@@ -71 +73 @@
     ScrollingStateTree();
 
+    PassOwnPtr<ScrollingStateTree> clone();
+
     OwnPtr<ScrollingStateScrollingNode> m_rootStateNode;
+    Vector<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
     bool m_hasChangedProperties;
 };

trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

@@ -259 +259 @@
 
     ScrollingStateNode* node = m_stateNodeMap.take(scrollLayerID);
-
-    // FIXME: removeNode() will destroy children, and those children might still be in the HashMap.
-    // This will be a big problem once there are actually children in the tree.
     m_scrollingStateTree->removeNode(node);
+
+    // ScrollingStateTree::removeNode() will destroy children, so we have to make sure we remove those children
+    // from the HashMap.
+   const Vector<ScrollingNodeID>& removedNodes = m_scrollingStateTree->removedNodes();
+    size_t size = removedNodes.size();
+    for (size_t i = 0; i < size; ++i)
+        m_stateNodeMap.remove(removedNodes[i]);
 }