Changeset 170197 in webkit


Ignore:
Timestamp:
Jun 20, 2014, 12:14:50 PM (11 years ago)
Author:
Simon Fraser
Message:

Handle scrolling tree modifications which remove intermediate nodes
​https://bugs.webkit.org/show_bug.cgi?id=134082

Reviewed by Tim Horton.

When updating the scrolling tree from the state tree, we failed to maintain
the children arrays correctly. Fix by removing all children on scrolling nodes,
and allowing the calls on children to add them back. A temporary hash map
keeps the nodes alive.

The state tree's m_nodesRemovedSinceLastCommit was also made into a HashSet,
to make it easier to handle removal followed by re-insertion.

Source/WebCore:

  • WebCore.exp.in:
  • page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::attachNode): If a node is (possibly re-)added,
remove it from m_nodesRemovedSinceLastCommit.remove.
(WebCore::ScrollingStateTree::willRemoveNode):
(WebCore::ScrollingStateTree::setRemovedNodes):

  • page/scrolling/ScrollingStateTree.h:

(WebCore::ScrollingStateTree::removedNodes):

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::commitNewTreeState):
(WebCore::ScrollingTree::updateTreeFromStateNode): Clean up to have only one call
to updateBeforeChildren(), and remove all children from the scrolling node
before visiting state children.
(WebCore::ScrollingTree::removeDestroyedNodes): It was very wrong to assume
that all non-root nodes were parented in the root! Now we don't need to
remove from the parent anyway.

  • page/scrolling/ScrollingTree.h:
  • page/scrolling/ScrollingTreeNode.h:

(WebCore::ScrollingTreeNode::children):

Source/WebKit2:

  • Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:

(WebKit::RemoteScrollingCoordinatorTransaction::decode):
(WebKit::RemoteScrollingTreeTextStream::dump):

Location:
trunk/Source
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r170196 r170197  
     12014-06-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Handle scrolling tree modifications which remove intermediate nodes
     4        https://bugs.webkit.org/show_bug.cgi?id=134082
     5
     6        Reviewed by Tim Horton.
     7
     8        When updating the scrolling tree from the state tree, we failed to maintain
     9        the children arrays correctly. Fix by removing all children on scrolling nodes,
     10        and allowing the calls on children to add them back. A temporary hash map
     11        keeps the nodes alive.
     12       
     13        The state tree's m_nodesRemovedSinceLastCommit was also made into a HashSet,
     14        to make it easier to handle removal followed by re-insertion.
     15
     16        * WebCore.exp.in:
     17        * page/scrolling/ScrollingStateTree.cpp:
     18        (WebCore::ScrollingStateTree::attachNode): If a node is (possibly re-)added,
     19        remove it from m_nodesRemovedSinceLastCommit.remove.
     20        (WebCore::ScrollingStateTree::willRemoveNode):
     21        (WebCore::ScrollingStateTree::setRemovedNodes):
     22        * page/scrolling/ScrollingStateTree.h:
     23        (WebCore::ScrollingStateTree::removedNodes):
     24        * page/scrolling/ScrollingTree.cpp:
     25        (WebCore::ScrollingTree::commitNewTreeState):
     26        (WebCore::ScrollingTree::updateTreeFromStateNode): Clean up to have only one call
     27        to updateBeforeChildren(), and remove all children from the scrolling node
     28        before visiting state children.
     29        (WebCore::ScrollingTree::removeDestroyedNodes): It was very wrong to assume
     30        that all non-root nodes were parented in the root! Now we don't need to
     31        remove from the parent anyway.
     32        * page/scrolling/ScrollingTree.h:
     33        * page/scrolling/ScrollingTreeNode.h:
     34        (WebCore::ScrollingTreeNode::children):
     35
    1362014-06-19  Simon Fraser  <simon.fraser@apple.com>
    237

  • trunk/Source/WebCore/WebCore.exp.in

    r170179 r170197  
    28362836__ZN7WebCore18ScrollingStateTree10attachNodeENS_17ScrollingNodeTypeEyy
    28372837__ZN7WebCore18ScrollingStateTree14stateNodeForIDEy
    2838 __ZN7WebCore18ScrollingStateTree15setRemovedNodesEN3WTF6VectorIyLm0ENS1_15CrashOnOverflowEEE
     2838__ZN7WebCore18ScrollingStateTree15setRemovedNodesEN3WTF7HashSetIyNS1_7IntHashIyEENS1_10HashTraitsIyEEEE
    28392839__ZN7WebCore18ScrollingStateTree23setHasChangedPropertiesEb
    28402840__ZN7WebCore18ScrollingStateTree6commitENS_19LayerRepresentation4TypeE

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

    r170131 r170197  
    7171{
    7272    ASSERT(newNodeID);
    73 
    7473    if (ScrollingStateNode* node = stateNodeForID(newNodeID)) {
    7574        if (!parentID)
    … …  
    129128
    130129    m_stateNodeMap.set(newNodeID, newNode);
     130    m_nodesRemovedSinceLastCommit.remove(newNodeID);
    131131    return newNodeID;
    132132}
    … …  
    210210void ScrollingStateTree::willRemoveNode(ScrollingStateNode* node)
    211211{
    212     m_nodesRemovedSinceLastCommit.append(node->scrollingNodeID());
     212    m_nodesRemovedSinceLastCommit.add(node->scrollingNodeID());
    213213    m_stateNodeMap.remove(node->scrollingNodeID());
    214214    setHasChangedProperties();
    215215}
    216216
    217 void ScrollingStateTree::setRemovedNodes(Vector<ScrollingNodeID> nodes)
     217void ScrollingStateTree::setRemovedNodes(HashSet<ScrollingNodeID> nodes)
    218218{
    219219    m_nodesRemovedSinceLastCommit = std::move(nodes);

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

    r170131 r170197  
    5757    void clear();
    5858   
    59     const Vector<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
    60     void setRemovedNodes(Vector<ScrollingNodeID>);
     59    const HashSet<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
     60    void setRemovedNodes(HashSet<ScrollingNodeID>);
    6161
    6262    // Copies the current tree state and clears the changed properties mask in the original.
    … …  
    9090    StateNodeMap m_stateNodeMap;
    9191    RefPtr<ScrollingStateFrameScrollingNode> m_rootStateNode;
    92     Vector<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
     92    HashSet<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
    9393    bool m_hasChangedProperties;
    9494    bool m_hasNewRootStateNode;

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

    r170196 r170197  
    150150    TemporaryChange<bool> changeHandlingProgrammaticScroll(m_isHandlingProgrammaticScroll, scrollRequestIsProgammatic);
    151151
     152    {
     153        OrphanScrollingNodeMap orphanNodes;
     154        updateTreeFromStateNode(rootNode, orphanNodes);
     155    }
     156
    152157    removeDestroyedNodes(*scrollingStateTree);
    153     updateTreeFromStateNode(rootNode);
    154 }
    155 
    156 void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode)
     158}
     159
     160void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode, OrphanScrollingNodeMap& orphanNodes)
    157161{
    158162    if (!stateNode) {
    … …  
    162166    }
    163167   
    164     // This fuction recurses through the ScrollingStateTree and updates the corresponding ScrollingTreeNodes.
    165     // Find the ScrollingTreeNode associated with the current stateNode using the shared ID and our HashMap.
    166     ScrollingTreeNodeMap::const_iterator it = m_nodeMap.find(stateNode->scrollingNodeID());
    167 
    168     ScrollingTreeNode* node;
    169     if (it != m_nodeMap.end()) {
     168    ScrollingNodeID nodeID = stateNode->scrollingNodeID();
     169    ScrollingNodeID parentNodeID = stateNode->parentNodeID();
     170
     171    auto it = m_nodeMap.find(nodeID);
     172
     173    RefPtr<ScrollingTreeNode> node;
     174    if (it != m_nodeMap.end())
    170175        node = it->value;
    171         node->updateBeforeChildren(*stateNode);
    172     } else {
    173         // If the node isn't found, it's either new and needs to be added to the tree, or there is a new ID for our
    174         // root node.
    175         ScrollingNodeID nodeID = stateNode->scrollingNodeID();
    176         if (!stateNode->parent()) {
    177             // This is the root node. Nuke the node map.
     176    else {
     177        node = createNode(stateNode->nodeType(), nodeID);
     178        if (!parentNodeID) {
     179            // This is the root node. Clear the node map.
     180            ASSERT(stateNode->nodeType() == FrameScrollingNode);
     181            m_rootNode = node;
    178182            m_nodeMap.clear();
    179 
    180             m_rootNode = createNode(FrameScrollingNode, nodeID);
    181             m_nodeMap.set(nodeID, m_rootNode.get());
    182             m_rootNode->updateBeforeChildren(*stateNode);
    183             node = m_rootNode.get();
    184         } else {
    185             RefPtr<ScrollingTreeNode> newNode = createNode(stateNode->nodeType(), nodeID);
    186             node = newNode.get();
    187             m_nodeMap.set(nodeID, node);
    188             ScrollingTreeNodeMap::const_iterator it = m_nodeMap.find(stateNode->parent()->scrollingNodeID());
    189             ASSERT(it != m_nodeMap.end());
    190             if (it != m_nodeMap.end()) {
    191                 ScrollingTreeNode* parent = it->value;
    192                 newNode->setParent(parent);
    193                 parent->appendChild(newNode.release());
    194             }
    195             node->updateBeforeChildren(*stateNode);
     183        }
     184        m_nodeMap.set(nodeID, node.get());
     185    }
     186
     187    if (parentNodeID) {
     188        auto parentIt = m_nodeMap.find(parentNodeID);
     189        ASSERT(parentIt != m_nodeMap.end());
     190        if (parentIt != m_nodeMap.end()) {
     191            ScrollingTreeNode* parent = parentIt->value;
     192            node->setParent(parent);
     193            parent->appendChild(node);
    196194        }
     195    }
     196
     197    node->updateBeforeChildren(*stateNode);
     198   
     199    // Move all children into the orphanNodes map. Live ones will get added back as we recurse over children.
     200    if (auto nodeChildren = node->children()) {
     201        for (auto& childScrollingNode : *nodeChildren) {
     202            childScrollingNode->setParent(nullptr);
     203            orphanNodes.add(childScrollingNode->scrollingNodeID(), childScrollingNode);
     204        }
     205        nodeChildren->clear();
    197206    }
    198207
    … …  
    200209    if (auto children = stateNode->children()) {
    201210        for (auto& child : *children)
    202             updateTreeFromStateNode(child.get());
    203     }
     211            updateTreeFromStateNode(child.get(), orphanNodes);
     212    }
     213
    204214    node->updateAfterChildren(*stateNode);
    205215}
    … …  
    215225            clearLatchedNode();
    216226
    217         // Never destroy the root node. There will be a new root node in the state tree, and we will
    218         // associate it with our existing root node in updateTreeFromStateNode().
    219         if (node->parent())
    220             m_rootNode->removeChild(node);
     227        // We should have unparented this node already via updateTreeFromStateNode().
     228        ASSERT(!node->parent());
    221229    }
    222230}

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.h

    r170196 r170197  
    132132private:
    133133    void removeDestroyedNodes(const ScrollingStateTree&);
    134     void updateTreeFromStateNode(const ScrollingStateNode*);
     134   
     135    typedef HashMap<ScrollingNodeID, RefPtr<ScrollingTreeNode>> OrphanScrollingNodeMap;
     136    void updateTreeFromStateNode(const ScrollingStateNode*, OrphanScrollingNodeMap&);
    135137
    136138    virtual PassRefPtr<ScrollingTreeNode> createNode(ScrollingNodeType, ScrollingNodeID) = 0;

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeNode.h

    r170196 r170197  
    6161    void setParent(ScrollingTreeNode* parent) { m_parent = parent; }
    6262
     63    typedef Vector<RefPtr<ScrollingTreeNode>> ScrollingTreeChildrenVector;
     64    ScrollingTreeChildrenVector* children() { return m_children.get(); }
     65   
    6366    void appendChild(PassRefPtr<ScrollingTreeNode>);
    6467    void removeChild(ScrollingTreeNode*);
    … …  
    6871    ScrollingTree& scrollingTree() const { return m_scrollingTree; }
    6972
    70     typedef Vector<RefPtr<ScrollingTreeNode>> ScrollingTreeChildrenVector;
    7173    OwnPtr<ScrollingTreeChildrenVector> m_children;
    7274

  • trunk/Source/WebKit2/ChangeLog

    r170196 r170197  
     12014-06-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Handle scrolling tree modifications which remove intermediate nodes
     4        https://bugs.webkit.org/show_bug.cgi?id=134082
     5
     6        Reviewed by Tim Horton.
     7
     8        When updating the scrolling tree from the state tree, we failed to maintain
     9        the children arrays correctly. Fix by removing all children on scrolling nodes,
     10        and allowing the calls on children to add them back. A temporary hash map
     11        keeps the nodes alive.
     12       
     13        The state tree's m_nodesRemovedSinceLastCommit was also made into a HashSet,
     14        to make it easier to handle removal followed by re-insertion.
     15
     16        * Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:
     17        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
     18        (WebKit::RemoteScrollingTreeTextStream::dump):
     19
    1202014-06-19  Simon Fraser  <simon.fraser@apple.com>
    221

  • trunk/Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp

    r170118 r170197  
    425425
    426426    // Removed nodes
    427     Vector<ScrollingNodeID> removedNodes;
     427    HashSet<ScrollingNodeID> removedNodes;
    428428    if (!decoder.decode(removedNodes))
    429429        return false;
    … …  
    684684        recursiveDumpNodes(*stateTree.rootStateNode(), changedPropertiesOnly);
    685685
    686     if (!stateTree.removedNodes().isEmpty())
    687         dumpProperty<Vector<ScrollingNodeID>>(ts, "removed-nodes", stateTree.removedNodes());
     686    if (!stateTree.removedNodes().isEmpty()) {
     687        Vector<ScrollingNodeID> removedNodes;
     688        copyToVector(stateTree.removedNodes(), removedNodes);
     689        dumpProperty<Vector<ScrollingNodeID>>(ts, "removed-nodes", removedNodes);
     690    }
    688691}
    689692
Note: See TracChangeset for help on using the changeset viewer.