Allow control over child order when adding nodes to the scrolling tree
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2018 01:47:04 +0000 (01:47 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2018 01:47:04 +0000 (01:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176914

Patch by Frederic Wang <fwang@igalia.com> on 2018-12-06
Reviewed by Simon Fraser.

Based on an earlier patch by Simon Fraser.

Source/WebCore:

Previously ScrollingCoordinator just allowed nodes to be "attached" with a given parent,
but with no control over sibling order. To allow for correct hit-testing overflow and
frame scrolling nodes, we have to build the scrolling tree in z-order.

This patch adds a 'childIndex' parameter to attachNode() which gives control over
sibling order. For now, RenderLayerCompositor always uses the default 'notFound' value
for childIndex so the current behavior (appending new nodes at the end of child list) is
preserved.

No new tests, behavior unchanged and already covered by existing tests.

* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::attachToStateTree):
(WebCore::AsyncScrollingCoordinator::ensureRootStateNodeForFrameView):
* page/scrolling/AsyncScrollingCoordinator.h:
* page/scrolling/ScrollingCoordinator.h:
(WebCore::ScrollingCoordinator::attachToStateTree):
* page/scrolling/ScrollingStateNode.cpp:
(WebCore::ScrollingStateNode::insertChild):
(WebCore::ScrollingStateNode::indexOfChild const):
* page/scrolling/ScrollingStateNode.h:
* page/scrolling/ScrollingStateTree.cpp:
(WebCore::ScrollingStateTree::nodeTypeAndParentMatch const):
(WebCore::ScrollingStateTree::attachNode):
* page/scrolling/ScrollingStateTree.h:

Source/WebKit:

* Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
(WebKit::RemoteScrollingCoordinatorTransaction::decode): Make explicit that we want to append
the new node at the end of child list.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238947 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingStateNode.cpp
Source/WebCore/page/scrolling/ScrollingStateNode.h
Source/WebCore/page/scrolling/ScrollingStateTree.cpp
Source/WebCore/page/scrolling/ScrollingStateTree.h
Source/WebKit/ChangeLog
Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp

index 1d166d7..30df288 100644 (file)
@@ -1,3 +1,38 @@
+2018-12-06  Frederic Wang  <fwang@igalia.com>
+
+        Allow control over child order when adding nodes to the scrolling tree
+        https://bugs.webkit.org/show_bug.cgi?id=176914
+
+        Reviewed by Simon Fraser.
+
+        Based on an earlier patch by Simon Fraser.
+
+        Previously ScrollingCoordinator just allowed nodes to be "attached" with a given parent,
+        but with no control over sibling order. To allow for correct hit-testing overflow and
+        frame scrolling nodes, we have to build the scrolling tree in z-order.
+
+        This patch adds a 'childIndex' parameter to attachNode() which gives control over
+        sibling order. For now, RenderLayerCompositor always uses the default 'notFound' value
+        for childIndex so the current behavior (appending new nodes at the end of child list) is
+        preserved.
+
+        No new tests, behavior unchanged and already covered by existing tests.
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::attachToStateTree):
+        (WebCore::AsyncScrollingCoordinator::ensureRootStateNodeForFrameView):
+        * page/scrolling/AsyncScrollingCoordinator.h:
+        * page/scrolling/ScrollingCoordinator.h:
+        (WebCore::ScrollingCoordinator::attachToStateTree):
+        * page/scrolling/ScrollingStateNode.cpp:
+        (WebCore::ScrollingStateNode::insertChild):
+        (WebCore::ScrollingStateNode::indexOfChild const):
+        * page/scrolling/ScrollingStateNode.h:
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::nodeTypeAndParentMatch const):
+        (WebCore::ScrollingStateTree::attachNode):
+        * page/scrolling/ScrollingStateTree.h:
+
 2018-12-06  Yongjun Zhang  <yongjun_zhang@apple.com>
 
         We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
index 2638826..25513f2 100644 (file)
@@ -474,9 +474,9 @@ void AsyncScrollingCoordinator::scrollableAreaScrollbarLayerDidChange(Scrollable
         scrollableArea.horizontalScrollbarLayerDidChange();
 }
 
-ScrollingNodeID AsyncScrollingCoordinator::attachToStateTree(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID)
+ScrollingNodeID AsyncScrollingCoordinator::attachToStateTree(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
 {
-    return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID);
+    return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID, childIndex);
 }
 
 void AsyncScrollingCoordinator::detachFromStateTree(ScrollingNodeID nodeID)
@@ -509,7 +509,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(MainFrameScrollingNode, frameView.scrollLayerID(), 0);
+    attachToStateTree(MainFrameScrollingNode, frameView.scrollLayerID(), 0, 0);
 }
 
 void AsyncScrollingCoordinator::updateFrameScrollingNode(ScrollingNodeID nodeID, GraphicsLayer* layer, GraphicsLayer* scrolledContentsLayer, GraphicsLayer* counterScrollingLayer, GraphicsLayer* insetClipLayer, const ScrollingGeometry& scrollingGeometry)
index 5d678e4..03ae58a 100644 (file)
@@ -97,7 +97,7 @@ private:
 
     WEBCORE_EXPORT bool requestScrollPositionUpdate(FrameView&, const IntPoint&) override;
 
-    WEBCORE_EXPORT ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID) 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;
     
index 53a6918..aa64e95 100644 (file)
@@ -164,7 +164,8 @@ 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*/) { return newNodeID; }
+    virtual ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/ = notFound) { return newNodeID; }
+
     virtual void detachFromStateTree(ScrollingNodeID) { }
     virtual void clearStateTree() { }
 
index d9bf7c5..03b7ab6 100644 (file)
@@ -97,6 +97,26 @@ void ScrollingStateNode::appendChild(Ref<ScrollingStateNode>&& childNode)
     m_children->append(WTFMove(childNode));
 }
 
+void ScrollingStateNode::insertChild(Ref<ScrollingStateNode>&& childNode, size_t index)
+{
+    childNode->setParent(this);
+
+    if (!m_children) {
+        ASSERT(!index);
+        m_children = std::make_unique<Vector<RefPtr<ScrollingStateNode>>>();
+    }
+
+    m_children->insert(index, WTFMove(childNode));
+}
+
+size_t ScrollingStateNode::indexOfChild(ScrollingStateNode& childNode) const
+{
+    if (!m_children)
+        return notFound;
+
+    return m_children->find(&childNode);
+}
+
 void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
 {
     if (!m_children)
index f310487..3390885 100644 (file)
@@ -236,6 +236,9 @@ public:
     Vector<RefPtr<ScrollingStateNode>>* children() const { return m_children.get(); }
 
     void appendChild(Ref<ScrollingStateNode>&&);
+    void insertChild(Ref<ScrollingStateNode>&&, size_t index);
+
+    size_t indexOfChild(ScrollingStateNode&) const;
 
     String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const;
 
index 70a95f4..01cbe3e 100644 (file)
@@ -82,25 +82,39 @@ Ref<ScrollingStateNode> ScrollingStateTree::createNode(ScrollingNodeType nodeTyp
     return ScrollingStateFixedNode::create(*this, nodeID);
 }
 
-bool ScrollingStateTree::nodeTypeAndParentMatch(ScrollingStateNode& node, ScrollingNodeType nodeType, ScrollingNodeID parentID) const
+bool ScrollingStateTree::nodeTypeAndParentMatch(ScrollingStateNode& node, ScrollingNodeType nodeType, ScrollingStateNode* parentNode) const
 {
     if (node.nodeType() != nodeType)
         return false;
 
-    auto* parent = stateNodeForID(parentID);
-    if (!parent)
-        return true;
-
-    return node.parent() == parent;
+    return node.parent() == parentNode;
 }
 
-ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID)
+ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
 {
     ASSERT(newNodeID);
 
     if (auto* node = stateNodeForID(newNodeID)) {
-        if (nodeTypeAndParentMatch(*node, nodeType, parentID))
+        auto* parent = stateNodeForID(parentID);
+        if (nodeTypeAndParentMatch(*node, nodeType, parent)) {
+            if (!parentID)
+                return newNodeID;
+
+            size_t currentIndex = parent->indexOfChild(*node);
+            if (currentIndex == childIndex)
+                return newNodeID;
+
+            ASSERT(currentIndex != notFound);
+            Ref<ScrollingStateNode> protectedNode(*node);
+            parent->children()->remove(currentIndex);
+
+            if (childIndex == notFound)
+                parent->appendChild(WTFMove(protectedNode));
+            else
+                parent->insertChild(WTFMove(protectedNode), childIndex);
+
             return newNodeID;
+        }
 
 #if ENABLE(ASYNC_SCROLLING)
         // If the type has changed, we need to destroy and recreate the node with a new ID.
@@ -114,6 +128,7 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
 
     ScrollingStateNode* newNode = nullptr;
     if (!parentID) {
+        ASSERT(!childIndex || childIndex == notFound);
         // If we're resetting the root node, we should clear the HashMap and destroy the current children.
         clear();
 
@@ -122,24 +137,32 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
         m_hasNewRootStateNode = true;
     } else {
         auto* parent = stateNodeForID(parentID);
-        if (!parent)
+        if (!parent) {
+            ASSERT_NOT_REACHED();
             return 0;
+        }
 
         if (nodeType == SubframeScrollingNode && parentID) {
             if (auto orphanedNode = m_orphanedSubframeNodes.take(newNodeID)) {
                 newNode = orphanedNode.get();
-                parent->appendChild(orphanedNode.releaseNonNull());
+                if (childIndex == notFound)
+                    parent->appendChild(orphanedNode.releaseNonNull());
+                else
+                    parent->insertChild(orphanedNode.releaseNonNull(), childIndex);
             }
         }
 
         if (!newNode) {
             auto stateNode = createNode(nodeType, newNodeID);
             newNode = stateNode.ptr();
-            parent->appendChild(WTFMove(stateNode));
+            if (childIndex == notFound)
+                parent->appendChild(WTFMove(stateNode));
+            else
+                parent->insertChild(WTFMove(stateNode), childIndex);
         }
     }
 
-    m_stateNodeMap.set(newNodeID, newNode);
+    addNode(*newNode);
     m_nodesRemovedSinceLastCommit.remove(newNodeID);
     return newNodeID;
 }
index cc9e571..5875586 100644 (file)
@@ -51,7 +51,7 @@ public:
     ScrollingStateFrameScrollingNode* rootStateNode() const { return m_rootStateNode.get(); }
     WEBCORE_EXPORT ScrollingStateNode* stateNodeForID(ScrollingNodeID) const;
 
-    WEBCORE_EXPORT ScrollingNodeID attachNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID);
+    WEBCORE_EXPORT ScrollingNodeID attachNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID, size_t childIndex);
     void detachNode(ScrollingNodeID);
     void clear();
     
@@ -81,7 +81,7 @@ private:
 
     Ref<ScrollingStateNode> createNode(ScrollingNodeType, ScrollingNodeID);
 
-    bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingNodeID parentID) const;
+    bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingStateNode* parentNode) const;
 
     enum class SubframeNodeRemoval { Delete, Orphan };
     void removeNodeAndAllDescendants(ScrollingStateNode*, SubframeNodeRemoval = SubframeNodeRemoval::Delete);
index 299569e..0e4251b 100644 (file)
@@ -1,3 +1,16 @@
+2018-12-06  Frederic Wang  <fwang@igalia.com>
+
+        Allow control over child order when adding nodes to the scrolling tree
+        https://bugs.webkit.org/show_bug.cgi?id=176914
+
+        Reviewed by Simon Fraser.
+
+        Based on an earlier patch by Simon Fraser.
+
+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
+        (WebKit::RemoteScrollingCoordinatorTransaction::decode): Make explicit that we want to append
+        the new node at the end of child list.
+
 2018-12-06  Yongjun Zhang  <yongjun_zhang@apple.com>
 
         We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
index 8d2d18c..9af089d 100644 (file)
@@ -413,7 +413,7 @@ bool RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder)
         if (!decoder.decode(parentNodeID))
             return false;
 
-        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID);
+        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, notFound); // Append new node.
         ScrollingStateNode* newNode = m_scrollingStateTree->stateNodeForID(nodeID);
         ASSERT(newNode);
         ASSERT(!parentNodeID || newNode->parent());