Attachment 360248 Details for Bug 193754 – Patch

[patch] Patch

bug-193754-20190126130807.patch (text/plain), 32.24 KB, created by Simon Fraser (smfr) on 2019-01-26 13:08:08 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-26 13:08:08 PST

Size: 32.24 KB

patch

obsolete

>Subversion Revision: 240548
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f8099584ab8f69a4bf454e85977e532fb6556cfc..9a697b645b0dd7a5bed8e22cfb8a465fc9f628b3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,92 @@
>+2019-01-26  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Allow scrolling tree nodes to exist in a detached state
>+        https://bugs.webkit.org/show_bug.cgi?id=193754
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        One of the (questionable?) design decisions of the scrolling tree is that the tree implementation
>+        is hidden behind the ScrollingCoordinator interface. That interface only allowed nodes to exist
>+        in a connected state; attachToStateTree() required a non-zero parent for any node that was not
>+        the root.
>+
>+        This makes it impossible to coordinate the hookup of the scrolling tree across frame boundaries;
>+        the scrolling tree has to have been fully constructed in ancestor frames before subframe nodes
>+        can be attached. This is a significant difference from compositing, where a subframe can create
>+        GraphicsLayers which don't have to be parented right away, and actually get parented later via
>+        a compositing update in the parent frame.
>+
>+        We want to be able to hook up the scrolling tree via the same code paths as GraphicsLayer
>+        connection (anything else is too confusing). So we need to be able to instantiate scrolling
>+        tree nodes in a disconnected state, and attach them later.
>+
>+        To achieve this, add the notion of "unparented" nodes to ScrollingCoordinator and the ScrollingStateTree.
>+        Allow clients to create unparented nodes, which can be attached later. ScrollingCoordinator stores
>+        the roots of unparented subtrees in an owning HashMap. Nodes in unparented trees are still referenced
>+        by m_stateNodeMap, so it's possible to find them and set state on them.
>+
>+        Clean up the ScrollingCoordinator interface to remove "state tree" terminology; the state vs. scrolling tree
>+        is really an implementation detail.
>+
>+        This also removes the special-casing of ScrollingNodeType::Subframe nodes which ScrollingStateTree stored
>+        in m_orphanedSubframeNodes; now the unparenting is controlled by the client.
>+
>+        Currently no code creates unparented nodes so there is no behavior change.
>+
>+        * dom/Document.cpp:
>+        (WebCore::Document::setPageCacheState):
>+        * page/scrolling/AsyncScrollingCoordinator.cpp:
>+        (WebCore::AsyncScrollingCoordinator::createNode):
>+        (WebCore::AsyncScrollingCoordinator::insertNode):
>+        (WebCore::AsyncScrollingCoordinator::unparentNode):
>+        (WebCore::AsyncScrollingCoordinator::unparentChildrenAndDestroyNode):
>+        (WebCore::AsyncScrollingCoordinator::detachAndDestroySubtree):
>+        (WebCore::AsyncScrollingCoordinator::clearAllNodes):
>+        (WebCore::AsyncScrollingCoordinator::parentOfNode const):
>+        (WebCore::AsyncScrollingCoordinator::ensureRootStateNodeForFrameView):
>+        (WebCore::AsyncScrollingCoordinator::attachToStateTree): Deleted.
>+        (WebCore::AsyncScrollingCoordinator::detachFromStateTree): Deleted.
>+        (WebCore::AsyncScrollingCoordinator::clearStateTree): Deleted.
>+        * page/scrolling/AsyncScrollingCoordinator.h:
>+        * page/scrolling/ScrollingCoordinator.h:
>+        (WebCore::ScrollingCoordinator::handleWheelEvent):
>+        (WebCore::ScrollingCoordinator::createNode):
>+        (WebCore::ScrollingCoordinator::insertNode):
>+        (WebCore::ScrollingCoordinator::unparentNode):
>+        (WebCore::ScrollingCoordinator::unparentChildrenAndDestroyNode):
>+        (WebCore::ScrollingCoordinator::detachAndDestroySubtree):
>+        (WebCore::ScrollingCoordinator::clearAllNodes):
>+        (WebCore::ScrollingCoordinator::parentOfNode const):
>+        (WebCore::ScrollingCoordinator::childrenOfNode const):
>+        (WebCore::ScrollingCoordinator::attachToStateTree): Deleted.
>+        (WebCore::ScrollingCoordinator::detachFromStateTree): Deleted.
>+        (WebCore::ScrollingCoordinator::clearStateTree): Deleted.
>+        * page/scrolling/ScrollingStateNode.cpp:
>+        (WebCore::ScrollingStateNode::removeFromParent):
>+        (WebCore::ScrollingStateNode::removeChild):
>+        * page/scrolling/ScrollingStateNode.h:
>+        * page/scrolling/ScrollingStateTree.cpp:
>+        (WebCore::ScrollingStateTree::ScrollingStateTree):
>+        (WebCore::ScrollingStateTree::createUnparentedNode):
>+        (WebCore::ScrollingStateTree::insertNode):
>+        (WebCore::ScrollingStateTree::unparentNode):
>+        (WebCore::ScrollingStateTree::unparentChildrenAndDestroyNode):
>+        (WebCore::ScrollingStateTree::detachAndDestroySubtree):
>+        (WebCore::ScrollingStateTree::clear):
>+        (WebCore::ScrollingStateTree::commit):
>+        (WebCore::ScrollingStateTree::removeNodeAndAllDescendants):
>+        (WebCore::ScrollingStateTree::recursiveNodeWillBeRemoved):
>+        (showScrollingStateTree):
>+        (WebCore::ScrollingStateTree::attachNode): Deleted.
>+        (WebCore::ScrollingStateTree::detachNode): Deleted.
>+        * page/scrolling/ScrollingStateTree.h:
>+        (WebCore::ScrollingStateTree::nodeCount const):
>+        * rendering/RenderLayerBacking.cpp:
>+        (WebCore::RenderLayerBacking::detachFromScrollingCoordinator):
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::reattachSubframeScrollLayers):
>+        (WebCore::RenderLayerCompositor::attachScrollingNode):
>+
> 2019-01-26  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] The initial values for top/bottom in contentHeightForFormattingContextRoot should not be 0.
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6aae160ba07f1b6ecb14a86ac29d0a3111717493..6ba5aa90ebae2611f4b37419dfaf018af4f60907 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-01-26  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Allow scrolling tree nodes to exist in a detached state
>+        https://bugs.webkit.org/show_bug.cgi?id=193754
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
>+        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
>+
> 2019-01-25  Tim Horton  <timothy_horton@apple.com>
> 
>         REGRESSION (r238818): Snapshot is removed too late after swiping back on Twitter
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index ad33faf9d125d7679d7142c285a4993f435ce06b..f86b36f90680d4278fcd45f147fb378cba8857d5 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -5200,7 +5200,7 @@ void Document::setPageCacheState(PageCacheState state)
>             if (page && m_frame->isMainFrame()) {
>                 v->resetScrollbarsAndClearContentsSize();
>                 if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
>-                    scrollingCoordinator->clearStateTree();
>+                    scrollingCoordinator->clearAllNodes();
>             }
>         }
> 
>diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>index 0493ec1160c3102c3b1ae4d715f24e1300dbdca8..8f12a31da23926b06b1bd8c164e2966be8b63ae8 100644
>--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>@@ -486,22 +486,47 @@ void AsyncScrollingCoordinator::scrollableAreaScrollbarLayerDidChange(Scrollable
>     }
> }
> 
>-ScrollingNodeID AsyncScrollingCoordinator::attachToStateTree(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
>+ScrollingNodeID AsyncScrollingCoordinator::createNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID)
> {
>-    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::attachToStateTree " << nodeType << " node " << newNodeID << " parent " << parentID << " index " << childIndex);
>-    return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID, childIndex);
>+    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::createNode " << nodeType << " node " << newNodeID);
>+    return m_scrollingStateTree->createUnparentedNode(nodeType, newNodeID);
> }
> 
>-void AsyncScrollingCoordinator::detachFromStateTree(ScrollingNodeID nodeID)
>+ScrollingNodeID AsyncScrollingCoordinator::insertNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
> {
>-    m_scrollingStateTree->detachNode(nodeID);
>+    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::insertNode " << nodeType << " node " << newNodeID << " parent " << parentID << " index " << childIndex);
>+    return m_scrollingStateTree->insertNode(nodeType, newNodeID, parentID, childIndex);
> }
> 
>-void AsyncScrollingCoordinator::clearStateTree()
>+void AsyncScrollingCoordinator::unparentNode(ScrollingNodeID nodeID)
>+{
>+    m_scrollingStateTree->unparentNode(nodeID);
>+}
>+
>+void AsyncScrollingCoordinator::unparentChildrenAndDestroyNode(ScrollingNodeID nodeID)
>+{
>+    m_scrollingStateTree->unparentChildrenAndDestroyNode(nodeID);
>+}
>+
>+void AsyncScrollingCoordinator::detachAndDestroySubtree(ScrollingNodeID nodeID)
>+{
>+    m_scrollingStateTree->detachAndDestroySubtree(nodeID);
>+}
>+
>+void AsyncScrollingCoordinator::clearAllNodes()
> {
>     m_scrollingStateTree->clear();
> }
> 
>+ScrollingNodeID AsyncScrollingCoordinator::parentOfNode(ScrollingNodeID nodeID) const
>+{
>+    auto* scrollingNode = m_scrollingStateTree->stateNodeForID(nodeID);
>+    if (!scrollingNode)
>+        return 0;
>+
>+    return scrollingNode->parentNodeID();
>+}
>+
> Vector<ScrollingNodeID> AsyncScrollingCoordinator::childrenOfNode(ScrollingNodeID nodeID) const
> {
>     auto* scrollingNode = m_scrollingStateTree->stateNodeForID(nodeID);
>@@ -540,7 +565,7 @@ void AsyncScrollingCoordinator::ensureRootStateNodeForFrameView(FrameView& frame
>     // For non-main frames, it is only possible to arrive in this function from
>     // RenderLayerCompositor::updateBacking where the node has already been created.
>     ASSERT(frameView.frame().isMainFrame());
>-    attachToStateTree(ScrollingNodeType::MainFrame, frameView.scrollLayerID(), 0, 0);
>+    insertNode(ScrollingNodeType::MainFrame, frameView.scrollLayerID(), 0, 0);
> }
> 
> void AsyncScrollingCoordinator::setNodeLayers(ScrollingNodeID nodeID, GraphicsLayer* layer, GraphicsLayer* scrolledContentsLayer, GraphicsLayer* counterScrollingLayer, GraphicsLayer* insetClipLayer)
>diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>index 57177edf691b1a1b9c53ce2b599806af728ff045..cc23da8f0be8926a1443ae15394042cf27abed34 100644
>--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>@@ -97,10 +97,14 @@ private:
> 
>     WEBCORE_EXPORT bool requestScrollPositionUpdate(FrameView&, const IntPoint&) override;
> 
>-    WEBCORE_EXPORT ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex) override;
>-    WEBCORE_EXPORT void detachFromStateTree(ScrollingNodeID) override;
>-    WEBCORE_EXPORT void clearStateTree() override;
>-
>+    WEBCORE_EXPORT ScrollingNodeID createNode(ScrollingNodeType, ScrollingNodeID newNodeID) override;
>+    WEBCORE_EXPORT ScrollingNodeID insertNode(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex) override;
>+    WEBCORE_EXPORT void unparentNode(ScrollingNodeID) override;
>+    WEBCORE_EXPORT void unparentChildrenAndDestroyNode(ScrollingNodeID) override;
>+    WEBCORE_EXPORT void detachAndDestroySubtree(ScrollingNodeID) override;
>+    WEBCORE_EXPORT void clearAllNodes() override;
>+
>+    WEBCORE_EXPORT ScrollingNodeID parentOfNode(ScrollingNodeID) const override;
>     WEBCORE_EXPORT Vector<ScrollingNodeID> childrenOfNode(ScrollingNodeID) const override;
> 
>     WEBCORE_EXPORT void setNodeLayers(ScrollingNodeID, GraphicsLayer* /*layer*/, GraphicsLayer* /*scrolledContentsLayer*/ = nullptr, GraphicsLayer* /*counterScrollingLayer*/ = nullptr, GraphicsLayer* /*insetClipLayer*/ = nullptr) override;
>diff --git a/Source/WebCore/page/scrolling/ScrollingCoordinator.h b/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>index ac976e3bca226ab5b750eb600b07758c60961b1e..6547daab9a903d0b4ee89a28e30664694b26fa35 100644
>--- a/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>+++ b/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>@@ -168,11 +168,22 @@ public:
>     virtual void commitTreeStateIfNeeded() { }
>     virtual bool requestScrollPositionUpdate(FrameView&, const IntPoint&) { return false; }
>     virtual bool handleWheelEvent(FrameView&, const PlatformWheelEvent&) { return true; }
>-    virtual ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/ = notFound) { return newNodeID; }
>-    virtual Vector<ScrollingNodeID> childrenOfNode(ScrollingNodeID) const { return { }; }
> 
>-    virtual void detachFromStateTree(ScrollingNodeID) { }
>-    virtual void clearStateTree() { }
>+    // Create an unparented node.
>+    virtual ScrollingNodeID createNode(ScrollingNodeType, ScrollingNodeID newNodeID) { return newNodeID; }
>+    // Parent a node in the scrolling tree. This may return a new nodeID if the node type changed. parentID = 0 sets the root node.
>+    virtual ScrollingNodeID insertNode(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/ = notFound) { return newNodeID; }
>+    // Node will be unparented, but not destroyed. It's the client's responsibility to either re-parent or destroy this node.
>+    virtual void unparentNode(ScrollingNodeID) { }
>+    // Node will be destroyed, and its children left unparented.
>+    virtual void unparentChildrenAndDestroyNode(ScrollingNodeID) { }
>+    // Node will be unparented, and it and its children destroyed.
>+    virtual void detachAndDestroySubtree(ScrollingNodeID) { }
>+    // Destroy the tree, including both parented and unparented nodes.
>+    virtual void clearAllNodes() { }
>+
>+    virtual ScrollingNodeID parentOfNode(ScrollingNodeID) const { return 0; }
>+    virtual Vector<ScrollingNodeID> childrenOfNode(ScrollingNodeID) const { return { }; }
> 
>     virtual void setNodeLayers(ScrollingNodeID, GraphicsLayer* /*layer*/, GraphicsLayer* /*scrolledContentsLayer*/ = nullptr, GraphicsLayer* /*counterScrollingLayer*/ = nullptr, GraphicsLayer* /*insetClipLayer*/ = nullptr) { }
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>index e10b21a333b8dd50649def53a26b5ba7445470eb..0fa8299511cb8911cabd5029c7911ab00dfadf44 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>@@ -111,6 +111,21 @@ void ScrollingStateNode::insertChild(Ref<ScrollingStateNode>&& childNode, size_t
>     setPropertyChanged(ChildNodes);
> }
> 
>+void ScrollingStateNode::removeFromParent()
>+{
>+    if (!m_parent)
>+        return;
>+
>+    m_parent->removeChild(*this);
>+}
>+
>+void ScrollingStateNode::removeChild(ScrollingStateNode& childNode)
>+{
>+    auto childIndex = indexOfChild(childNode);
>+    if (childIndex != notFound)
>+        removeChildAtIndex(childIndex);
>+}
>+
> void ScrollingStateNode::removeChildAtIndex(size_t index)
> {
>     ASSERT(m_children && index < m_children->size());
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.h b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>index c28872f1fc2e060b707b1509a9b45de2bd4d6a6f..a0747ec65e645bf54fa4170b8c37c2ab32c19eea 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>@@ -240,7 +240,10 @@ public:
>     void appendChild(Ref<ScrollingStateNode>&&);
>     void insertChild(Ref<ScrollingStateNode>&&, size_t index);
> 
>+    // Note that node ownership is via the parent, so these functions can trigger node deletion.
>+    void removeFromParent();
>     void removeChildAtIndex(size_t index);
>+    void removeChild(ScrollingStateNode&);
> 
>     size_t indexOfChild(ScrollingStateNode&) const;
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>index b1a26b9ce90e383ad21c316f004a97fdab17fb17..b6076c00a8de10f8417ba26b2736f8c58dca3792 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>@@ -29,24 +29,19 @@
> #if ENABLE(ASYNC_SCROLLING) || USE(COORDINATED_GRAPHICS)
> 
> #include "AsyncScrollingCoordinator.h"
>+#include "Logging.h"
> #include "ScrollingStateFixedNode.h"
> #include "ScrollingStateFrameHostingNode.h"
> #include "ScrollingStateFrameScrollingNode.h"
> #include "ScrollingStateOverflowScrollingNode.h"
> #include "ScrollingStateStickyNode.h"
> #include <wtf/text/CString.h>
>-
>-#ifndef NDEBUG
>-#include <stdio.h>
>-#endif
>+#include <wtf/text/TextStream.h>
> 
> namespace WebCore {
> 
> ScrollingStateTree::ScrollingStateTree(AsyncScrollingCoordinator* scrollingCoordinator)
>     : m_scrollingCoordinator(scrollingCoordinator)
>-    , m_hasChangedProperties(false)
>-    , m_hasNewRootStateNode(false)
>-    , m_preferredLayerRepresentation(LayerRepresentation::GraphicsLayerRepresentation)
> {
> }
> 
>@@ -93,8 +88,35 @@ bool ScrollingStateTree::nodeTypeAndParentMatch(ScrollingStateNode& node, Scroll
>     return node.parent() == parentNode;
> }
> 
>-ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
>+ScrollingNodeID ScrollingStateTree::createUnparentedNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID)
> {
>+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " createUnparentedNode " << newNodeID);
>+
>+    if (auto* node = stateNodeForID(newNodeID)) {
>+        if (node->nodeType() == nodeType) {
>+            // If the node exists and is parented, unparent it. It may already be in m_unparentedNodes.
>+            unparentNode(newNodeID);
>+            return newNodeID;
>+        }
>+
>+#if ENABLE(ASYNC_SCROLLING)
>+        // If the type has changed, we need to destroy and recreate the node with a new ID.
>+        if (nodeType != node->nodeType()) {
>+            unparentChildrenAndDestroyNode(newNodeID);
>+            newNodeID = m_scrollingCoordinator->uniqueScrollingNodeID();
>+        }
>+#endif
>+    }
>+
>+    auto stateNode = createNode(nodeType, newNodeID);
>+    addNode(stateNode.get());
>+    m_unparentedNodes.add(newNodeID, WTFMove(stateNode));
>+    return newNodeID;
>+}
>+
>+ScrollingNodeID ScrollingStateTree::insertNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
>+{
>+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " insertNode " << newNodeID << " in parent " << parentID << " at " << childIndex);
>     ASSERT(newNodeID);
> 
>     if (auto* node = stateNodeForID(newNodeID)) {
>@@ -126,7 +148,7 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
> #endif
> 
>         // The node is being re-parented. To do that, we'll remove it, and then create a new node.
>-        removeNodeAndAllDescendants(node, SubframeNodeRemoval::Orphan);
>+        unparentNode(newNodeID);
>     }
> 
>     ScrollingStateNode* newNode = nullptr;
>@@ -145,13 +167,13 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
>             return 0;
>         }
> 
>-        if (nodeType == ScrollingNodeType::Subframe && parentID) {
>-            if (auto orphanedNode = m_orphanedSubframeNodes.take(newNodeID)) {
>-                newNode = orphanedNode.get();
>+        if (parentID) {
>+            if (auto unparentedNode = m_unparentedNodes.take(newNodeID)) {
>+                newNode = unparentedNode.get();
>                 if (childIndex == notFound)
>-                    parent->appendChild(orphanedNode.releaseNonNull());
>+                    parent->appendChild(unparentedNode.releaseNonNull());
>                 else
>-                    parent->insertChild(orphanedNode.releaseNonNull(), childIndex);
>+                    parent->insertChild(unparentedNode.releaseNonNull(), childIndex);
>             }
>         }
> 
>@@ -170,17 +192,57 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
>     return newNodeID;
> }
> 
>-void ScrollingStateTree::detachNode(ScrollingNodeID nodeID)
>+void ScrollingStateTree::unparentNode(ScrollingNodeID nodeID)
> {
>     if (!nodeID)
>         return;
> 
>-    // The node may not be found if clearStateTree() was recently called.
>+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " unparentNode " << nodeID);
>+
>+    // The node may not be found if clear() was recently called.
>+    RefPtr<ScrollingStateNode> protectedNode = m_stateNodeMap.get(nodeID);
>+    if (!protectedNode)
>+        return;
>+
>+    protectedNode->removeFromParent();
>+    m_unparentedNodes.add(nodeID, WTFMove(protectedNode));
>+}
>+
>+void ScrollingStateTree::unparentChildrenAndDestroyNode(ScrollingNodeID nodeID)
>+{
>+    if (!nodeID)
>+        return;
>+
>+    // The node may not be found if clear() was recently called.
>+    RefPtr<ScrollingStateNode> protectedNode = m_stateNodeMap.take(nodeID);
>+    if (!protectedNode)
>+        return;
>+
>+    if (auto* children = protectedNode->children()) {
>+        for (auto child : *children) {
>+            child->removeFromParent();
>+            m_unparentedNodes.add(child->scrollingNodeID(), WTFMove(child));
>+        }
>+        children->clear();
>+    }
>+    
>+    protectedNode->removeFromParent();
>+    willRemoveNode(protectedNode.get());
>+}
>+
>+void ScrollingStateTree::detachAndDestroySubtree(ScrollingNodeID nodeID)
>+{
>+    if (!nodeID)
>+        return;
>+
>+    // The node may not be found if clear() was recently called.
>     auto* node = m_stateNodeMap.take(nodeID);
>     if (!node)
>         return;
> 
>-    removeNodeAndAllDescendants(node, SubframeNodeRemoval::Orphan);
>+    // If the node was unparented, remove it from m_unparentedNodes (keeping it alive until this function returns).
>+    auto unparentedNode = m_unparentedNodes.take(nodeID);
>+    removeNodeAndAllDescendants(node);
> }
> 
> void ScrollingStateTree::clear()
>@@ -189,17 +251,14 @@ void ScrollingStateTree::clear()
>         removeNodeAndAllDescendants(rootStateNode());
> 
>     m_stateNodeMap.clear();
>-    m_orphanedSubframeNodes.clear();
>+    m_unparentedNodes.clear();
> }
> 
> std::unique_ptr<ScrollingStateTree> ScrollingStateTree::commit(LayerRepresentation::Type preferredLayerRepresentation)
> {
>-    if (!m_orphanedSubframeNodes.isEmpty()) {
>-        // If we still have orphaned subtrees, remove them from m_stateNodeMap since they will be deleted 
>-        // when clearing m_orphanedSubframeNodes.
>-        for (auto& orphanNode : m_orphanedSubframeNodes.values())
>-            recursiveNodeWillBeRemoved(orphanNode.get(), SubframeNodeRemoval::Delete);
>-        m_orphanedSubframeNodes.clear();
>+    if (!m_unparentedNodes.isEmpty()) {
>+        // We expect temporarily to have unparented nodes when committing when connecting across iframe boundaries, but unparented nodes should not stick around for a long time.
>+        LOG(Scrolling, "Committing with %u unparented nodes", m_unparentedNodes.size());
>     }
> 
>     // This function clones and resets the current state tree, but leaves the tree structure intact.
>@@ -232,18 +291,18 @@ void ScrollingStateTree::addNode(ScrollingStateNode& node)
>     m_stateNodeMap.add(node.scrollingNodeID(), &node);
> }
> 
>-void ScrollingStateTree::removeNodeAndAllDescendants(ScrollingStateNode* node, SubframeNodeRemoval subframeNodeRemoval)
>+void ScrollingStateTree::removeNodeAndAllDescendants(ScrollingStateNode* node)
> {
>     auto* parent = node->parent();
> 
>-    recursiveNodeWillBeRemoved(node, subframeNodeRemoval);
>+    recursiveNodeWillBeRemoved(node);
> 
>     if (node == m_rootStateNode)
>         m_rootStateNode = nullptr;
>     else if (parent) {
>         ASSERT(parent->children());
>         ASSERT(parent->children()->find(node) != notFound);
>-        if (auto children = parent->children()) {
>+        if (auto* children = parent->children()) {
>             size_t index = children->find(node);
>             if (index != notFound)
>                 children->remove(index);
>@@ -251,19 +310,14 @@ void ScrollingStateTree::removeNodeAndAllDescendants(ScrollingStateNode* node, S
>     }
> }
> 
>-void ScrollingStateTree::recursiveNodeWillBeRemoved(ScrollingStateNode* currNode, SubframeNodeRemoval subframeNodeRemoval)
>+void ScrollingStateTree::recursiveNodeWillBeRemoved(ScrollingStateNode* currNode)
> {
>     currNode->setParent(nullptr);
>-    if (subframeNodeRemoval == SubframeNodeRemoval::Orphan && currNode != m_rootStateNode && currNode->isFrameScrollingNode()) {
>-        m_orphanedSubframeNodes.add(currNode->scrollingNodeID(), currNode);
>-        return;
>-    }
>-
>     willRemoveNode(currNode);
> 
>-    if (auto children = currNode->children()) {
>+    if (auto* children = currNode->children()) {
>         for (auto& child : *children)
>-            recursiveNodeWillBeRemoved(child.get(), subframeNodeRemoval);
>+            recursiveNodeWillBeRemoved(child.get());
>     }
> }
> 
>@@ -302,12 +356,12 @@ void showScrollingStateTree(const WebCore::ScrollingStateTree* tree)
> 
>     auto rootNode = tree->rootStateNode();
>     if (!rootNode) {
>-        fprintf(stderr, "Scrolling state tree %p with no root node\n", tree);
>+        WTFLogAlways("Scrolling state tree %p with no root node\n", tree);
>         return;
>     }
> 
>     String output = rootNode->scrollingStateTreeAsText(WebCore::ScrollingStateTreeAsTextBehaviorDebug);
>-    fprintf(stderr, "%s\n", output.utf8().data());
>+    WTFLogAlways("%s\n", output.utf8().data());
> }
> 
> void showScrollingStateTree(const WebCore::ScrollingStateNode* node)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.h b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>index 5875586edcef90ac016ba00e2306bec6c14d5b2f..a7e5f258ca2054a3a671a18b68293518670be9e8 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>@@ -51,8 +51,11 @@ public:
>     ScrollingStateFrameScrollingNode* rootStateNode() const { return m_rootStateNode.get(); }
>     WEBCORE_EXPORT ScrollingStateNode* stateNodeForID(ScrollingNodeID) const;
> 
>-    WEBCORE_EXPORT ScrollingNodeID attachNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID, size_t childIndex);
>-    void detachNode(ScrollingNodeID);
>+    ScrollingNodeID createUnparentedNode(ScrollingNodeType, ScrollingNodeID);
>+    WEBCORE_EXPORT ScrollingNodeID insertNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID, size_t childIndex);
>+    void unparentNode(ScrollingNodeID);
>+    void unparentChildrenAndDestroyNode(ScrollingNodeID);
>+    void detachAndDestroySubtree(ScrollingNodeID);
>     void clear();
>     
>     const HashSet<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
>@@ -66,8 +69,8 @@ public:
> 
>     bool hasNewRootStateNode() const { return m_hasNewRootStateNode; }
>     void setHasNewRootStateNode(bool hasNewRoot) { m_hasNewRootStateNode = hasNewRoot; }
>-    
>-    int nodeCount() const { return m_stateNodeMap.size(); }
>+
>+    unsigned nodeCount() const { return m_stateNodeMap.size(); }
> 
>     typedef HashMap<ScrollingNodeID, ScrollingStateNode*> StateNodeMap;
>     const StateNodeMap& nodeMap() const { return m_stateNodeMap; }
>@@ -83,20 +86,22 @@ private:
> 
>     bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingStateNode* parentNode) const;
> 
>-    enum class SubframeNodeRemoval { Delete, Orphan };
>-    void removeNodeAndAllDescendants(ScrollingStateNode*, SubframeNodeRemoval = SubframeNodeRemoval::Delete);
>+    void removeNodeAndAllDescendants(ScrollingStateNode*);
> 
>-    void recursiveNodeWillBeRemoved(ScrollingStateNode* currNode, SubframeNodeRemoval);
>+    void recursiveNodeWillBeRemoved(ScrollingStateNode* currNode);
>     void willRemoveNode(ScrollingStateNode*);
> 
>     AsyncScrollingCoordinator* m_scrollingCoordinator;
>+    // Contains all the nodes we know about (those in the m_rootStateNode tree, and in m_unparentedNodes subtrees).
>     StateNodeMap m_stateNodeMap;
>+    // Owns roots of unparented subtrees.
>+    HashMap<ScrollingNodeID, RefPtr<ScrollingStateNode>> m_unparentedNodes;
>+
>     RefPtr<ScrollingStateFrameScrollingNode> m_rootStateNode;
>     HashSet<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
>-    HashMap<ScrollingNodeID, RefPtr<ScrollingStateNode>> m_orphanedSubframeNodes;
>-    bool m_hasChangedProperties;
>-    bool m_hasNewRootStateNode;
>-    LayerRepresentation::Type m_preferredLayerRepresentation;
>+    bool m_hasChangedProperties { false };
>+    bool m_hasNewRootStateNode { false };
>+    LayerRepresentation::Type m_preferredLayerRepresentation { LayerRepresentation::GraphicsLayerRepresentation };
> };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
>index dacaa3ddda006c9d4fba14c41fef57a23136d11a..818e7ba74dc31c5cf433cd5c58658e50d92a073f 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
>+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
>@@ -1778,19 +1778,19 @@ void RenderLayerBacking::detachFromScrollingCoordinator(OptionSet<ScrollCoordina
> 
>     if (roles.contains(ScrollCoordinationRole::Scrolling) && m_scrollingNodeID) {
>         LOG(Compositing, "Detaching Scrolling node %" PRIu64, m_scrollingNodeID);
>-        scrollingCoordinator->detachFromStateTree(m_scrollingNodeID);
>+        scrollingCoordinator->unparentChildrenAndDestroyNode(m_scrollingNodeID);
>         m_scrollingNodeID = 0;
>     }
> 
>     if (roles.contains(ScrollCoordinationRole::Scrolling) && m_frameHostingNodeID) {
>         LOG(Compositing, "Detaching FrameHosting node %" PRIu64, m_frameHostingNodeID);
>-        scrollingCoordinator->detachFromStateTree(m_frameHostingNodeID);
>+        scrollingCoordinator->unparentChildrenAndDestroyNode(m_frameHostingNodeID);
>         m_frameHostingNodeID = 0;
>     }
> 
>     if (roles.contains(ScrollCoordinationRole::ViewportConstrained) && m_viewportConstrainedNodeID) {
>         LOG(Compositing, "Detaching ViewportConstrained node %" PRIu64, m_viewportConstrainedNodeID);
>-        scrollingCoordinator->detachFromStateTree(m_viewportConstrainedNodeID);
>+        scrollingCoordinator->unparentChildrenAndDestroyNode(m_viewportConstrainedNodeID);
>         m_viewportConstrainedNodeID = 0;
>     }
> }
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index 2e50790faaf5d9c65aa0f9a0eeb142644605a5e6..28c82381964c0e5ee92fe5bf1ccd99be89ec691a 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -3788,7 +3788,7 @@ void RenderLayerCompositor::reattachSubframeScrollLayers()
>         if (!parentNodeID)
>             continue;
> 
>-        scrollingCoordinator->attachToStateTree(child->isMainFrame() ? ScrollingNodeType::MainFrame : ScrollingNodeType::Subframe, frameScrollingNodeID, parentNodeID);
>+        scrollingCoordinator->insertNode(child->isMainFrame() ? ScrollingNodeType::MainFrame : ScrollingNodeType::Subframe, frameScrollingNodeID, parentNodeID);
>     }
> }
> 
>@@ -3823,7 +3823,7 @@ ScrollingNodeID RenderLayerCompositor::attachScrollingNode(RenderLayer& layer, S
>     if (!nodeID)
>         nodeID = scrollingCoordinator->uniqueScrollingNodeID();
> 
>-    nodeID = scrollingCoordinator->attachToStateTree(nodeType, nodeID, parentNodeID);
>+    nodeID = scrollingCoordinator->insertNode(nodeType, nodeID, parentNodeID);
>     if (!nodeID)
>         return 0;
> 
>diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>index e95ca5ba5337d0392841a1113b86e064a19bd983..30221e478631a6359545baed5b7d7266bc5ef9df 100644
>--- a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>+++ b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>@@ -454,7 +454,7 @@ bool RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder)
>         if (!decoder.decode(parentNodeID))
>             return false;
> 
>-        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, notFound); // Append new node.
>+        m_scrollingStateTree->insertNode(nodeType, nodeID, parentNodeID, notFound);
>         ScrollingStateNode* newNode = m_scrollingStateTree->stateNodeForID(nodeID);
>         ASSERT(newNode);
>         ASSERT(!parentNodeID || newNode->parent());

Flags:

zalan: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193754: 360248