Changeset 239010 in webkit


Ignore:
Timestamp:
Dec 8, 2018 1:08:17 PM (5 years ago)
Author:
Simon Fraser
Message:

Allow control over child order when adding nodes to the scrolling tree
​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.

Location:
trunk
Files:
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r239006 r239010  
     12018-12-08  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Allow control over child order when adding nodes to the scrolling tree
     4        https://bugs.webkit.org/show_bug.cgi?id=176914
     5        <rdar://problem/46542237>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * platform/mac-wk2/TestExpectations: Mark fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html
     10        as flakey, which it will be until we attach in z-order.
     11
    1122018-12-07  Eric Carlson  <eric.carlson@apple.com>
    213

  • trunk/LayoutTests/platform/mac-wk2/TestExpectations

    r238935 r239010  
    322322webkit.org/b/148408 tiled-drawing/scrolling/root-overflow-with-mousewheel.html [ Pass Failure Timeout ]
    323323
     324webkit.org/b/192529 fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html [ Pass Failure ]
     325
    324326webkit.org/b/139820 fast/frames/lots-of-objects.html [ Pass Timeout ]
    325327webkit.org/b/139820 fast/frames/lots-of-iframes.html [ Pass Timeout ]

  • trunk/Source/WebCore/ChangeLog

    r239006 r239010  
     12018-12-08  Frederic Wang  <fwang@igalia.com>
     2
     3        Allow control over child order when adding nodes to the scrolling tree
     4        https://bugs.webkit.org/show_bug.cgi?id=176914
     5        <rdar://problem/46542237>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Based on an earlier patch by Simon Fraser.
     10
     11        Previously ScrollingCoordinator just allowed nodes to be "attached" with a given parent,
     12        but with no control over sibling order. To allow for correct hit-testing overflow and
     13        frame scrolling nodes, we have to build the scrolling tree in z-order.
     14
     15        This patch adds a 'childIndex' parameter to attachNode() which gives control over
     16        sibling order. For now, RenderLayerCompositor always uses the default 'notFound' value
     17        for childIndex so the current behavior (appending new nodes at the end of child list) is
     18        preserved.
     19
     20        One test marked as flakey, since scrolling tree order is currently dependent on HashSet
     21        traversal order.
     22
     23        * page/scrolling/AsyncScrollingCoordinator.cpp:
     24        (WebCore::AsyncScrollingCoordinator::attachToStateTree):
     25        (WebCore::AsyncScrollingCoordinator::ensureRootStateNodeForFrameView):
     26        * page/scrolling/AsyncScrollingCoordinator.h:
     27        * page/scrolling/ScrollingCoordinator.h:
     28        (WebCore::ScrollingCoordinator::attachToStateTree):
     29        * page/scrolling/ScrollingStateNode.cpp:
     30        (WebCore::ScrollingStateNode::insertChild):
     31        (WebCore::ScrollingStateNode::indexOfChild const):
     32        * page/scrolling/ScrollingStateNode.h:
     33        * page/scrolling/ScrollingStateTree.cpp:
     34        (WebCore::ScrollingStateTree::nodeTypeAndParentMatch const):
     35        (WebCore::ScrollingStateTree::attachNode):
     36        * page/scrolling/ScrollingStateTree.h:
     37
    1382018-12-07  Eric Carlson  <eric.carlson@apple.com>
    239

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp

    r238958 r239010  
    475475}
    476476
    477 ScrollingNodeID AsyncScrollingCoordinator::attachToStateTree(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID)
    478 {
    479     return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID);
     477ScrollingNodeID AsyncScrollingCoordinator::attachToStateTree(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
     478{
     479    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::attachToStateTree " << nodeType << " node " << newNodeID << " parent " << parentID << " index " << childIndex);
     480    return m_scrollingStateTree->attachNode(nodeType, newNodeID, parentID, childIndex);
    480481}
    481482
    … …  
    510511    // RenderLayerCompositor::updateBacking where the node has already been created.
    511512    ASSERT(frameView.frame().isMainFrame());
    512     attachToStateTree(MainFrameScrollingNode, frameView.scrollLayerID(), 0);
     513    attachToStateTree(MainFrameScrollingNode, frameView.scrollLayerID(), 0, 0);
    513514}
    514515

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h

    r238958 r239010  
    9898    WEBCORE_EXPORT bool requestScrollPositionUpdate(FrameView&, const IntPoint&) override;
    9999
    100     WEBCORE_EXPORT ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID) override;
     100    WEBCORE_EXPORT ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex) override;
    101101    WEBCORE_EXPORT void detachFromStateTree(ScrollingNodeID) override;
    102102    WEBCORE_EXPORT void clearStateTree() override;

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h

    r238958 r239010  
    165165    virtual bool requestScrollPositionUpdate(FrameView&, const IntPoint&) { return false; }
    166166    virtual bool handleWheelEvent(FrameView&, const PlatformWheelEvent&) { return true; }
    167     virtual ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/) { return newNodeID; }
     167    virtual ScrollingNodeID attachToStateTree(ScrollingNodeType, ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/ = notFound) { return newNodeID; }
     168
    168169    virtual void detachFromStateTree(ScrollingNodeID) { }
    169170    virtual void clearStateTree() { }

  • trunk/Source/WebCore/page/scrolling/ScrollingStateNode.cpp

    r238958 r239010  
    9898}
    9999
     100void ScrollingStateNode::insertChild(Ref<ScrollingStateNode>&& childNode, size_t index)
     101{
     102    childNode->setParent(this);
     103
     104    if (!m_children) {
     105        ASSERT(!index);
     106        m_children = std::make_unique<Vector<RefPtr<ScrollingStateNode>>>();
     107    }
     108
     109    m_children->insert(index, WTFMove(childNode));
     110}
     111
     112size_t ScrollingStateNode::indexOfChild(ScrollingStateNode& childNode) const
     113{
     114    if (!m_children)
     115        return notFound;
     116
     117    return m_children->find(&childNode);
     118}
     119
    100120void ScrollingStateNode::reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction action)
    101121{

  • trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h

    r238958 r239010  
    237237
    238238    void appendChild(Ref<ScrollingStateNode>&&);
     239    void insertChild(Ref<ScrollingStateNode>&&, size_t index);
     240
     241    size_t indexOfChild(ScrollingStateNode&) const;
    239242
    240243    String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const;

  • trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp

    r238958 r239010  
    8383}
    8484
    85 bool ScrollingStateTree::nodeTypeAndParentMatch(ScrollingStateNode& node, ScrollingNodeType nodeType, ScrollingNodeID parentID) const
     85bool ScrollingStateTree::nodeTypeAndParentMatch(ScrollingStateNode& node, ScrollingNodeType nodeType, ScrollingStateNode* parentNode) const
    8686{
    8787    if (node.nodeType() != nodeType)
    8888        return false;
    8989
    90     auto* parent = stateNodeForID(parentID);
    91     if (!parent)
    92         return true;
    93 
    94     return node.parent() == parent;
    95 }
    96 
    97 ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID)
     90    return node.parent() == parentNode;
     91}
     92
     93ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID, size_t childIndex)
    9894{
    9995    ASSERT(newNodeID);
    10096
    10197    if (auto* node = stateNodeForID(newNodeID)) {
    102         if (nodeTypeAndParentMatch(*node, nodeType, parentID))
     98        auto* parent = stateNodeForID(parentID);
     99        if (nodeTypeAndParentMatch(*node, nodeType, parent)) {
     100            if (!parentID)
     101                return newNodeID;
     102
     103            size_t currentIndex = parent->indexOfChild(*node);
     104            if (currentIndex == childIndex)
     105                return newNodeID;
     106
     107            ASSERT(currentIndex != notFound);
     108            Ref<ScrollingStateNode> protectedNode(*node);
     109            parent->children()->remove(currentIndex);
     110
     111            if (childIndex == notFound)
     112                parent->appendChild(WTFMove(protectedNode));
     113            else
     114                parent->insertChild(WTFMove(protectedNode), childIndex);
     115
    103116            return newNodeID;
     117        }
    104118
    105119#if ENABLE(ASYNC_SCROLLING)
    … …  
    115129    ScrollingStateNode* newNode = nullptr;
    116130    if (!parentID) {
     131        ASSERT(!childIndex || childIndex == notFound);
    117132        // If we're resetting the root node, we should clear the HashMap and destroy the current children.
    118133        clear();
    … …  
    123138    } else {
    124139        auto* parent = stateNodeForID(parentID);
    125         if (!parent)
     140        if (!parent) {
     141            ASSERT_NOT_REACHED();
    126142            return 0;
     143        }
    127144
    128145        if (nodeType == SubframeScrollingNode && parentID) {
    129146            if (auto orphanedNode = m_orphanedSubframeNodes.take(newNodeID)) {
    130147                newNode = orphanedNode.get();
    131                 parent->appendChild(orphanedNode.releaseNonNull());
     148                if (childIndex == notFound)
     149                    parent->appendChild(orphanedNode.releaseNonNull());
     150                else
     151                    parent->insertChild(orphanedNode.releaseNonNull(), childIndex);
    132152            }
    133153        }
    … …  
    136156            auto stateNode = createNode(nodeType, newNodeID);
    137157            newNode = stateNode.ptr();
    138             parent->appendChild(WTFMove(stateNode));
    139         }
    140     }
    141 
    142     m_stateNodeMap.set(newNodeID, newNode);
     158            if (childIndex == notFound)
     159                parent->appendChild(WTFMove(stateNode));
     160            else
     161                parent->insertChild(WTFMove(stateNode), childIndex);
     162        }
     163    }
     164
     165    addNode(*newNode);
    143166    m_nodesRemovedSinceLastCommit.remove(newNodeID);
    144167    return newNodeID;

  • trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h

    r238958 r239010  
    5252    WEBCORE_EXPORT ScrollingStateNode* stateNodeForID(ScrollingNodeID) const;
    5353
    54     WEBCORE_EXPORT ScrollingNodeID attachNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID);
     54    WEBCORE_EXPORT ScrollingNodeID attachNode(ScrollingNodeType, ScrollingNodeID, ScrollingNodeID parentID, size_t childIndex);
    5555    void detachNode(ScrollingNodeID);
    5656    void clear();
    … …  
    8282    Ref<ScrollingStateNode> createNode(ScrollingNodeType, ScrollingNodeID);
    8383
    84     bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingNodeID parentID) const;
     84    bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingStateNode* parentNode) const;
    8585
    8686    enum class SubframeNodeRemoval { Delete, Orphan };

  • trunk/Source/WebKit/ChangeLog

    r239007 r239010  
     12018-12-08  Frederic Wang  <fwang@igalia.com>
     2
     3        Allow control over child order when adding nodes to the scrolling tree
     4        https://bugs.webkit.org/show_bug.cgi?id=176914
     5        <rdar://problem/46542237>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Based on an earlier patch by Simon Fraser.
     10
     11        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
     12        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
     13
    1142018-12-07  Rob Buis  <rbuis@igalia.com>
    215

  • trunk/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp

    r238958 r239010  
    414414            return false;
    415415
    416         m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID);
     416        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, notFound); // Append new node.
    417417        ScrollingStateNode* newNode = m_scrollingStateTree->stateNodeForID(nodeID);
    418418        ASSERT(newNode);
Note: See TracChangeset for help on using the changeset viewer.