Attachment 356722 Details for Bug 176914 – Patch

[patch] Patch

0001-Bug-192395-Allow-control-over-child-order-when-addin.patch (text/plain), 14.84 KB, created by Frédéric Wang (:fredw) on 2018-12-06 01:21:46 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2018-12-06 01:21:46 PST

Size: 14.84 KB

patch

obsolete

>From e1533e65c742f3ed57c2127563d4aa190eb3e364 Mon Sep 17 00:00:00 2001
>From: Frederic Wang <fwang@igalia.com>
>Date: Wed, 5 Dec 2018 14:54:57 +0100
>Subject: [PATCH xserver] Bug 192395 - Allow control over child order when
> adding nodes to the scrolling tree
>
>---
> Source/WebCore/ChangeLog                      | 35 ++++++++++++++
> .../scrolling/AsyncScrollingCoordinator.cpp   |  6 +--
> .../scrolling/AsyncScrollingCoordinator.h     |  2 +-
> .../page/scrolling/ScrollingCoordinator.h     |  3 +-
> .../page/scrolling/ScrollingStateNode.cpp     | 20 ++++++++
> .../page/scrolling/ScrollingStateNode.h       |  3 ++
> .../page/scrolling/ScrollingStateTree.cpp     | 47 ++++++++++++++-----
> .../page/scrolling/ScrollingStateTree.h       |  4 +-
> Source/WebKit/ChangeLog                       | 13 +++++
> .../RemoteScrollingCoordinatorTransaction.cpp |  2 +-
> 10 files changed, 115 insertions(+), 20 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d3e4d463aae..e54680e2312 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-05  Don Olmstead  <don.olmstead@sony.com>
> 
>         [PlayStation] Enable WebCore
>diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>index 2638826b507..25513f2a74d 100644
>--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>@@ -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)
>diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>index 5d678e400e5..03ae58aa66e 100644
>--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
>@@ -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;
>     
>diff --git a/Source/WebCore/page/scrolling/ScrollingCoordinator.h b/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>index 53a6918ec2e..aa64e95bbc8 100644
>--- a/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>+++ b/Source/WebCore/page/scrolling/ScrollingCoordinator.h
>@@ -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() { }
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>index d9bf7c5e680..03b7ab61c5f 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>@@ -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)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.h b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>index f310487454f..33908850c11 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>@@ -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;
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>index 70a95f47e68..01cbe3e2857 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>@@ -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;
> }
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.h b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>index cc9e571abd8..5875586edce 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>@@ -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);
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index d250437cf9b..3547bed4aeb 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-05  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         REGRESSION(PSON): Process swapping code doesn't set DisplayID in WebContent process
>diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>index 8d2d18c096e..9af089d6f5d 100644
>--- a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>+++ b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>@@ -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());
>-- 
>2.19.1
>

Actions: View | Formatted Diff | Diff

Attachments on bug 176914: 320777 | 320782 | 320785 | 320787 | 320795 | 324232 | 324410 | 327061 | 356603 | 356615 | 356722