Attachment 373491 Details for Bug 199514 – patch

[patch] patch

fixed-delta-cleanup-3.patch (text/plain), 14.41 KB, created by Antti Koivisto on 2019-07-05 06:18:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2019-07-05 06:18:55 PDT

Size: 14.41 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 247150)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,53 @@
>+2019-07-05  Antti Koivisto  <antti@apple.com>
>+
>+        Clean up ScrollingTreeFixedNode::applyLayerPositions
>+        https://bugs.webkit.org/show_bug.cgi?id=199514
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Avoid is<FooNode> tests, simplify code, and move towards computing everything during tree walk
>+        rather than using parent traversal.
>+
>+        No functional changes.
>+
>+        * page/scrolling/ScrollingTreeNode.h:
>+        (WebCore::ScrollingTreeNode::fixedPositionDeltaSinceLastCommit const):
>+
>+        Make virtual.
>+
>+        * page/scrolling/ScrollingTreeScrollingNode.h:
>+        * page/scrolling/cocoa/ScrollingTreeFixedNode.h:
>+        (WebCore::ScrollingTreeFixedNode::layer):
>+        * page/scrolling/cocoa/ScrollingTreeFixedNode.mm:
>+        (WebCore::ScrollingTreeFixedNode::applyLayerPositions):
>+
>+        Call virtual fixedPositionDeltaSinceLastCommit instead of doing is<FooNode> tests.
>+        Reverse the double-negative signs.
>+
>+        * page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.h:
>+        * page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm:
>+        (WebCore::ScrollingTreeOverflowScrollProxyNode::fixedPositionDeltaSinceLastCommit const):
>+        (WebCore::ScrollingTreeOverflowScrollProxyNode::scrollDeltaSinceLastCommit const): Deleted.
>+
>+        Rename to indicate this is specifically about fixed positioning (affects signs).
>+
>+        * page/scrolling/cocoa/ScrollingTreePositionedNode.h:
>+        * page/scrolling/cocoa/ScrollingTreePositionedNode.mm:
>+        (WebCore::ScrollingTreePositionedNode::fixedPositionDeltaSinceLastCommit const):
>+
>+        No delta if layer positioning is handled by a child.
>+        This logic moves here from ScrollingTreeFixedNode::applyLayerPositions.
>+
>+        (WebCore::ScrollingTreePositionedNode::applyLayerPositions):
>+        (WebCore::ScrollingTreePositionedNode::scrollDeltaSinceLastCommit const): Deleted.
>+        * page/scrolling/cocoa/ScrollingTreeStickyNode.h:
>+        * page/scrolling/cocoa/ScrollingTreeStickyNode.mm:
>+        (WebCore::ScrollingTreeStickyNode::fixedPositionDeltaSinceLastCommit const):
>+
>+        Reverse the sign to be consistent with the rest.
>+
>+        (WebCore::ScrollingTreeStickyNode::scrollDeltaSinceLastCommit const): Deleted.
>+
> 2019-07-05  Adrian Perez de Castro  <aperez@igalia.com>
> 
>         [ATK] Do not use C linkage for functions using C++ features
>Index: Source/WebCore/page/scrolling/ScrollingTreeNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/ScrollingTreeNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/ScrollingTreeNode.h	(working copy)
>@@ -80,6 +80,7 @@ public:
>     virtual LayoutPoint parentToLocalPoint(LayoutPoint point) const { return point; }
>     virtual LayoutPoint localToContentsPoint(LayoutPoint point) const { return point; }
>     virtual ScrollingTreeScrollingNode* scrollingNodeForPoint(LayoutPoint) const;
>+    virtual FloatSize fixedPositionDeltaSinceLastCommit() const { return { }; }
> 
> protected:
>     ScrollingTreeNode(ScrollingTree&, ScrollingNodeType, ScrollingNodeID);
>Index: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h	(working copy)
>@@ -57,7 +57,7 @@ public:
>     FloatPoint currentScrollPosition() const { return m_currentScrollPosition; }
>     FloatPoint currentScrollOffset() const { return ScrollableArea::scrollOffsetFromPosition(m_currentScrollPosition, toFloatSize(m_scrollOrigin)); }
>     FloatPoint lastCommittedScrollPosition() const { return m_lastCommittedScrollPosition; }
>-    FloatSize scrollDeltaSinceLastCommit() const { return m_currentScrollPosition - m_lastCommittedScrollPosition; }
>+    FloatSize fixedPositionDeltaSinceLastCommit() const override { return m_currentScrollPosition - m_lastCommittedScrollPosition; }
> 
>     // These are imperative; they adjust the scrolling layers.
>     void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.h	(working copy)
>@@ -43,6 +43,8 @@ public:
> 
>     virtual ~ScrollingTreeFixedNode();
> 
>+    CALayer *layer() { return m_layer.get(); }
>+
> private:
>     ScrollingTreeFixedNode(ScrollingTree&, ScrollingNodeID);
> 
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeFixedNode.mm	(working copy)
>@@ -71,52 +71,21 @@ void ScrollingTreeFixedNode::commitState
> void ScrollingTreeFixedNode::applyLayerPositions()
> {
>     auto computeLayerPosition = [&] {
>-        FloatSize overflowScrollDelta;
>-        ScrollingTreeStickyNode* lastStickyNode = nullptr;
>+        FloatSize delta;
>         for (auto* ancestor = parent(); ancestor; ancestor = ancestor->parent()) {
>             if (is<ScrollingTreeFrameScrollingNode>(*ancestor)) {
>                 // Fixed nodes are positioned relative to the containing frame scrolling node.
>                 // We bail out after finding one.
>                 auto layoutViewport = downcast<ScrollingTreeFrameScrollingNode>(*ancestor).layoutViewport();
>-                return m_constraints.layerPositionForViewportRect(layoutViewport) - overflowScrollDelta;
>-            }
>-
>-            if (is<ScrollingTreeOverflowScrollingNode>(*ancestor)) {
>-                // To keep the layer still during async scrolling we adjust by how much the position has changed since layout.
>-                auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*ancestor);
>-                overflowScrollDelta -= overflowNode.scrollDeltaSinceLastCommit();
>-                continue;
>-            }
>-
>-            if (is<ScrollingTreeOverflowScrollProxyNode>(*ancestor)) {
>-                // To keep the layer still during async scrolling we adjust by how much the position has changed since layout.
>-                auto& overflowNode = downcast<ScrollingTreeOverflowScrollProxyNode>(*ancestor);
>-                overflowScrollDelta -= overflowNode.scrollDeltaSinceLastCommit();
>-                continue;
>-            }
>-
>-            if (is<ScrollingTreePositionedNode>(*ancestor)) {
>-                auto& positioningAncestor = downcast<ScrollingTreePositionedNode>(*ancestor);
>-                // See if sticky node already handled this positioning node.
>-                // FIXME: Include positioning node information to sticky/fixed node to avoid these tests.
>-                if (lastStickyNode && lastStickyNode->layer() == positioningAncestor.layer())
>-                    continue;
>-                if (positioningAncestor.layer() != m_layer)
>-                    overflowScrollDelta -= positioningAncestor.scrollDeltaSinceLastCommit();
>-                continue;
>-            }
>-
>-            if (is<ScrollingTreeStickyNode>(*ancestor)) {
>-                auto& stickyNode = downcast<ScrollingTreeStickyNode>(*ancestor);
>-                overflowScrollDelta += stickyNode.scrollDeltaSinceLastCommit();
>-                lastStickyNode = &stickyNode;
>-                continue;
>+                return m_constraints.layerPositionForViewportRect(layoutViewport) + delta;
>             }
> 
>             if (is<ScrollingTreeFixedNode>(*ancestor)) {
>-                // The ancestor fixed node has already applied the needed corrections to say put.
>-                return m_constraints.layerPositionAtLastLayout() - overflowScrollDelta;
>+                // The ancestor fixed node has already applied the needed corrections to stay put.
>+                return m_constraints.layerPositionAtLastLayout() + delta;
>             }
>+
>+            delta += ancestor->fixedPositionDeltaSinceLastCommit();
>         }
>         ASSERT_NOT_REACHED();
>         return FloatPoint();
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.h	(working copy)
>@@ -40,7 +40,7 @@ public:
> 
>     CALayer *layer() const { return m_layer.get(); }
> 
>-    FloatSize scrollDeltaSinceLastCommit() const;
>+    FloatSize fixedPositionDeltaSinceLastCommit() const override;
> 
> protected:
>     WEBCORE_EXPORT ScrollingTreeOverflowScrollProxyNode(ScrollingTree&, ScrollingNodeID);
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm	(working copy)
>@@ -67,11 +67,11 @@ void ScrollingTreeOverflowScrollProxyNod
>     }
> }
> 
>-FloatSize ScrollingTreeOverflowScrollProxyNode::scrollDeltaSinceLastCommit() const
>+FloatSize ScrollingTreeOverflowScrollProxyNode::fixedPositionDeltaSinceLastCommit() const
> {
>     if (auto* node = scrollingTree().nodeForID(m_overflowScrollingNodeID)) {
>         if (is<ScrollingTreeOverflowScrollingNode>(node))
>-            return downcast<ScrollingTreeOverflowScrollingNode>(*node).scrollDeltaSinceLastCommit();
>+            return downcast<ScrollingTreeOverflowScrollingNode>(*node).fixedPositionDeltaSinceLastCommit();
>     }
> 
>     return { };
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h	(working copy)
>@@ -45,7 +45,7 @@ public:
> 
>     const Vector<ScrollingNodeID>& relatedOverflowScrollingNodes() const { return m_relatedOverflowScrollingNodes; }
> 
>-    FloatSize scrollDeltaSinceLastCommit() const;
>+    FloatSize fixedPositionDeltaSinceLastCommit() const override;
> 
> private:
>     ScrollingTreePositionedNode(ScrollingTree&, ScrollingNodeID);
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm	(working copy)
>@@ -67,14 +67,33 @@ void ScrollingTreePositionedNode::commit
>         scrollingTree().activePositionedNodes().add(*this);
> }
> 
>-FloatSize ScrollingTreePositionedNode::scrollDeltaSinceLastCommit() const
>+FloatSize ScrollingTreePositionedNode::fixedPositionDeltaSinceLastCommit() const
> {
>+    auto shouldPositionLayer = [&] {
>+        if (!children() || children()->size() != 1)
>+            return true;
>+
>+        // In there is a child node for the same layer then this node exits for scroll propagationg
>+        // and container finding purposes only. The child node is responsible for layer positioning.
>+        // FIXME: Combine positioning node types to avoid this.
>+        auto& child = *children()->at(0);
>+        if (is<ScrollingTreeStickyNode>(child))
>+            return downcast<ScrollingTreeStickyNode>(child).layer() != m_layer;
>+        if (is<ScrollingTreeFixedNode>(child))
>+            return downcast<ScrollingTreeFixedNode>(child).layer() != m_layer;
>+
>+        return true;
>+    };
>+
>+    if (!shouldPositionLayer())
>+        return { };
>+
>     FloatSize delta;
>     for (auto nodeID : m_relatedOverflowScrollingNodes) {
>         if (auto* node = scrollingTree().nodeForID(nodeID)) {
>             if (is<ScrollingTreeOverflowScrollingNode>(node)) {
>                 auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*node);
>-                delta += overflowNode.scrollDeltaSinceLastCommit();
>+                delta += overflowNode.fixedPositionDeltaSinceLastCommit();
>             }
>         }
>     }
>@@ -85,7 +104,7 @@ FloatSize ScrollingTreePositionedNode::s
> 
> void ScrollingTreePositionedNode::applyLayerPositions()
> {
>-    auto delta = scrollDeltaSinceLastCommit();
>+    auto delta = fixedPositionDeltaSinceLastCommit();
>     auto layerPosition = m_constraints.layerPositionAtLastLayout() - delta;
> 
>     LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreePositionedNode " << scrollingNodeID() << " applyLayerPositions: overflow delta " << delta << " moving layer to " << layerPosition);
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.h
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.h	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.h	(working copy)
>@@ -41,7 +41,7 @@ public:
> 
>     virtual ~ScrollingTreeStickyNode();
> 
>-    FloatSize scrollDeltaSinceLastCommit() const;
>+    FloatSize fixedPositionDeltaSinceLastCommit() const override;
> 
>     CALayer *layer() { return m_layer.get(); }
> 
>Index: Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm
>===================================================================
>--- Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm	(revision 247090)
>+++ Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm	(working copy)
>@@ -112,10 +112,13 @@ void ScrollingTreeStickyNode::applyLayer
>     [m_layer _web_setLayerTopLeftPosition:layerPosition - m_constraints.alignmentOffset()];
> }
> 
>-FloatSize ScrollingTreeStickyNode::scrollDeltaSinceLastCommit() const
>+FloatSize ScrollingTreeStickyNode::fixedPositionDeltaSinceLastCommit() const
> {
>     auto layerPosition = computeLayerPosition();
>-    return layerPosition - m_constraints.layerPositionAtLastLayout();
>+    auto delta = layerPosition - m_constraints.layerPositionAtLastLayout();
>+
>+    // Sticky nodes compensate for scrolling, so negate the scroll delta.
>+    return -delta;
> }
> 
> void ScrollingTreeStickyNode::dumpProperties(TextStream& ts, ScrollingStateTreeAsTextBehavior behavior) const

Flags:

koivisto: review?

Actions: View | Formatted Diff | Diff

Attachments on bug 199514: 373490 | 373491