Attachment 356599 Details for Bug 192395 – Attachment 356581, rebased on top of bug 192398

[patch] Attachment 356581, rebased on top of bug 192398

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

(hide)

Description:

Filename:

MIME Type:

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

Created: 2018-12-05 06:00:46 PST

Size: 15.16 KB

patch

obsolete

>From 442fe38160029199ec46bb86cc2821444b27af61 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 2/2] Bug 192395 - Allow control over child order when
> adding nodes to the scrolling tree
>
>---
> Source/WebCore/ChangeLog                      | 30 +++++++++++++
> .../scrolling/AsyncScrollingCoordinator.cpp   |  6 +--
> .../scrolling/AsyncScrollingCoordinator.h     |  2 +-
> .../page/scrolling/ScrollingCoordinator.h     |  3 +-
> .../page/scrolling/ScrollingStateNode.cpp     | 20 +++++++++
> .../page/scrolling/ScrollingStateNode.h       |  6 ++-
> .../page/scrolling/ScrollingStateTree.cpp     | 44 ++++++++++++++-----
> .../page/scrolling/ScrollingStateTree.h       |  4 +-
> Source/WebKit/ChangeLog                       | 10 +++++
> .../RemoteScrollingCoordinatorTransaction.cpp |  2 +-
> 10 files changed, 106 insertions(+), 21 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b2cec33bc0c..940e535d4cb 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-12-04  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=192395
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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, but doesn't use it yet.
>+
>+        * 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:
>+        (WebCore::ScrollingStateNode::scrollingNodeID const):
>+        * page/scrolling/ScrollingStateTree.cpp:
>+        (WebCore::ScrollingStateTree::nodeTypeAndParentMatch const):
>+        (WebCore::ScrollingStateTree::attachNode):
>+        * page/scrolling/ScrollingStateTree.h:
>+
> 2018-12-05  Frederic Wang  <fwang@igalia.com>
> 
>         Minor refactoring of the scrolling code
>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..b727a5b2924 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>@@ -195,6 +195,7 @@ public:
>     virtual ~ScrollingStateNode();
>     
>     ScrollingNodeType nodeType() const { return m_nodeType; }
>+    ScrollingNodeID scrollingNodeID() const { return m_nodeID; }
> 
>     bool isFixedNode() const { return m_nodeType == FixedNode; }
>     bool isStickyNode() const { return m_nodeType == StickyNode; }
>@@ -227,8 +228,6 @@ public:
> 
>     ScrollingStateTree& scrollingStateTree() const { return m_scrollingStateTree; }
> 
>-    ScrollingNodeID scrollingNodeID() const { return m_nodeID; }
>-
>     ScrollingStateNode* parent() const { return m_parent; }
>     void setParent(ScrollingStateNode* parent) { m_parent = parent; }
>     ScrollingNodeID parentNodeID() const { return m_parent ? m_parent->scrollingNodeID() : 0; }
>@@ -236,6 +235,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..242eb09a7ca 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,12 +137,15 @@ 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();
>+                // FIXME: use childIndex
>                 parent->appendChild(orphanedNode.releaseNonNull());
>             }
>         }
>@@ -135,11 +153,15 @@ ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, Scrol
>         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 d8df7441d7c..960c04b0efd 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-12-04  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=192395
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
>+        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
>+
> 2018-12-05  Rob Buis  <rbuis@igalia.com>
> 
>         [Mac] HEAD requests changed to GET after 301, 302, and 303 redirections (http/tests/xmlhttprequest/head-redirection.html)
>diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>index 8d2d18c096e..9b8d1c512eb 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, i);
>         ScrollingStateNode* newNode = m_scrollingStateTree->stateNodeForID(nodeID);
>         ASSERT(newNode);
>         ASSERT(!parentNodeID || newNode->parent());
>-- 
>2.19.1
>

Actions: View | Formatted Diff | Diff

Attachments on bug 192395: 356581 | 356584 | 356589 | 356591 | 356592 | 356599