Changeset 142505 in webkit

Timestamp:

Feb 11, 2013 1:28:18 PM (11 years ago)

Author:

Simon Fraser

Message:

ScrollingTree node maps keep getting larger
​https://bugs.webkit.org/show_bug.cgi?id=109348

Reviewed by Sam Weinig.

When navigating between pages, nodes would get left in the ScrollingTree's
node map, and the ScrollingStateTree's node map, so these would get larger
and larger as you browse.

Simplify map maintenance by clearing the map when setting a new root node
(which happens on the first commit of a new page). Also, don't keep root nodes
around, but create them afresh for each page, which simplifies their ID
management.

This is closer to the original behavior; keeping the root nodes around was
a fix for bug 99668, but we avoid regressing that fix by bailing early
from frameViewLayoutUpdated() if there is no root state node (we'll get
called again anyway).

This now allows state nodeIDs to be purely read-only.

• page/scrolling/ScrollingStateNode.h:
• page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::ScrollingStateTree):
(WebCore::ScrollingStateTree::attachNode):
(WebCore::ScrollingStateTree::clear):
(WebCore::ScrollingStateTree::removeNode):

• page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::updateTreeFromStateNode):

• page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::frameViewLayoutUpdated):

Location:

trunk/Source/WebCore

Files:

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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-11  Simon Fraser  <simon.fraser@apple.com>
+
+        ScrollingTree node maps keep getting larger
+        https://bugs.webkit.org/show_bug.cgi?id=109348
+
+        Reviewed by Sam Weinig.
+
+        When navigating between pages, nodes would get left in the ScrollingTree's
+        node map, and the ScrollingStateTree's node map, so these would get larger
+        and larger as you browse.
+        
+        Simplify map maintenance by clearing the map when setting a new root node
+        (which happens on the first commit of a new page). Also, don't keep root nodes
+        around, but create them afresh for each page, which simplifies their ID
+        management.
+        
+        This is closer to the original behavior; keeping the root nodes around was
+        a fix for bug 99668, but we avoid regressing that fix by bailing early
+        from frameViewLayoutUpdated() if there is no root state node (we'll get
+        called again anyway).
+        
+        This now allows state nodeIDs to be purely read-only.
+
+        * page/scrolling/ScrollingStateNode.h:
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::ScrollingStateTree):
+        (WebCore::ScrollingStateTree::attachNode):
+        (WebCore::ScrollingStateTree::clear):
+        (WebCore::ScrollingStateTree::removeNode):
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::updateTreeFromStateNode):
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::frameViewLayoutUpdated):
+
 2013-02-11  Simon Fraser  <simon.fraser@apple.com>
 

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

@@ -80 +80 @@
 
     ScrollingNodeID scrollingNodeID() const { return m_nodeID; }
-    void setScrollingNodeID(ScrollingNodeID nodeID) { m_nodeID = nodeID; }
 
     ScrollingStateNode* parent() const { return m_parent; }

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

@@ -42 +42 @@
 
 ScrollingStateTree::ScrollingStateTree()
-    : m_rootStateNode(ScrollingStateScrollingNode::create(this, 0))
-    , m_hasChangedProperties(false)
+    : m_hasChangedProperties(false)
 {
 }
@@ -71 +70 @@
         clear();
 
-        rootStateNode()->setScrollingNodeID(newNodeID);
+        setRootStateNode(ScrollingStateScrollingNode::create(this, newNodeID));
         newNode = rootStateNode();
     } else {
@@ -121 +120 @@
 {
     removeNode(rootStateNode());
+    m_stateNodeMap.clear();
 }
 
@@ -141 +141 @@
 void ScrollingStateTree::removeNode(ScrollingStateNode* node)
 {
+    if (!node)
+        return;
+
+    if (node == m_rootStateNode) {
+        didRemoveNode(node->scrollingNodeID());
+        m_rootStateNode = nullptr;
+        return;
+    }
+
     ASSERT(m_rootStateNode);
     m_rootStateNode->removeChild(node);

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

@@ -160 +160 @@
         ScrollingNodeID nodeID = stateNode->scrollingNodeID();
         if (!stateNode->parent()) {
-            // This is the root node.
-            if (!m_rootNode)
-                m_rootNode = ScrollingTreeScrollingNode::create(this, nodeID);
-
+            // This is the root node. Nuke the node map.
+            m_nodeMap.clear();
+
+            m_rootNode = ScrollingTreeScrollingNode::create(this, nodeID);
             m_nodeMap.set(nodeID, m_rootNode.get());
             m_rootNode->update(stateNode);

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

@@ -108 +108 @@
     ASSERT(m_page);
 
+    // If there isn't a root node yet, don't do anything. We'll be called again after creating one.
+    if (!m_scrollingStateTree->rootStateNode())
+        return;
+
     // Compute the region of the page that we can't do fast scrolling for. This currently includes
     // all scrollable areas, such as subframes, overflow divs and list boxes. We need to do this even if the