Allow control over child order when adding nodes to the scrolling tree
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Dec 2018 21:08:17 +0000 (21:08 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Dec 2018 21:08:17 +0000 (21:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176914
<rdar://problem/46542237>

Source/WebCore:

Patch by Frederic Wang <fwang@igalia.com> on 2018-12-08
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.

One test marked as flakey, since scrolling tree order is currently dependent on HashSet
traversal order.

* 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:

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

Based on an earlier patch by Simon Fraser.

* Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
(WebKit::RemoteScrollingCoordinatorTransaction::decode):

LayoutTests:

Reviewed by Simon Fraser.

* platform/mac-wk2/TestExpectations: Mark fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html
as flakey, which it will be until we attach in z-order.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
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 c7e2389..6057740 100644 (file)
@@ -1,3 +1,14 @@
+2018-12-08  Simon Fraser  <simon.fraser@apple.com>
+
+        Allow control over child order when adding nodes to the scrolling tree
+        https://bugs.webkit.org/show_bug.cgi?id=176914
+        <rdar://problem/46542237>
+
+        Reviewed by Simon Fraser.
+
+        * platform/mac-wk2/TestExpectations: Mark fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html
+        as flakey, which it will be until we attach in z-order.
+
 2018-12-07  Eric Carlson  <eric.carlson@apple.com>
 
         [MediaStream] 'devicechange' event should not fire in frames that can't access capture devices
index ff9b37f..5811214 100644 (file)
@@ -321,6 +321,8 @@ webkit.org/b/167321 tiled-drawing/scrolling/scroll-snap/scroll-snap-proximity-ma
 
 webkit.org/b/148408 tiled-drawing/scrolling/root-overflow-with-mousewheel.html [ Pass Failure Timeout ]
 
+webkit.org/b/192529 fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html [ Pass Failure ]
+
 webkit.org/b/139820 fast/frames/lots-of-objects.html [ Pass Timeout ]
 webkit.org/b/139820 fast/frames/lots-of-iframes.html [ Pass Timeout ]
 
index e440931..ff73f21 100644 (file)
@@ -1,3 +1,40 @@
+2018-12-08  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
+        <rdar://problem/46542237>
+
+        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.
+
+        One test marked as flakey, since scrolling tree order is currently dependent on HashSet
+        traversal order.
+
+        * 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-07  Eric Carlson  <eric.carlson@apple.com>
 
         [MediaStream] 'devicechange' event should not fire in frames that can't access capture devices
index 2638826..76e4268 100644 (file)
@@ -474,9 +474,10 @@ 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);
+    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::attachToStateTree " << nodeType << " node " << newNodeID << " parent " << parentID << " index " << childIndex);
+    return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID, childIndex);
 }
 
 void AsyncScrollingCoordinator::detachFromStateTree(ScrollingNodeID nodeID)
@@ -509,7 +510,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 7df762f..99bcac5 100644 (file)
@@ -1,3 +1,16 @@
+2018-12-08  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
+        <rdar://problem/46542237>
+
+        Reviewed by Simon Fraser.
+
+        Based on an earlier patch by Simon Fraser.
+
+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
+        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
+
 2018-12-07  Rob Buis  <rbuis@igalia.com>
 
         Remove unused API in NetworkProcess
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());