Modernize remote scrolling coordinator code

https://bugs.webkit.org/show_bug.cgi?id=259993
rdar://113651997

Reviewed by Simon Fraser.

Remove some unnecessary headers including other headers.
Use ThreadSafeWeakPtr instead of storing a raw pointer.
Make RemoteScrollingCoordinatorTransaction::decode return std::optional.
Use return value instead of out-param in RemoteScrollingCoordinator::buildTransaction.

* Source/WebCore/page/scrolling/ScrollingStateTree.cpp:
(WebCore::ScrollingStateTree::setHasChangedProperties):
(WebCore::ScrollingStateTree::createUnparentedNode):
(WebCore::ScrollingStateTree::insertNode):
* Source/WebCore/page/scrolling/ScrollingStateTree.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
(WebKit::RemoteScrollingCoordinatorTransaction::RemoteScrollingCoordinatorTransaction):
(WebKit::RemoteScrollingCoordinatorTransaction::decode):
* Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.h:
(WebKit::RemoteScrollingCoordinatorTransaction::clearScrollLatching const):
(WebKit::RemoteScrollingCoordinatorTransaction::setStateTreeToEncode): Deleted.
(WebKit::RemoteScrollingCoordinatorTransaction::setClearScrollLatching): Deleted.
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::updateRendering):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
(WebKit::RemoteScrollingCoordinator::buildTransaction):

Canonical link: https://commits.webkit.org/266779@main

Source/WebCore/page/scrolling/ScrollingStateTree.cpp

@@ -74,8 +74,9 @@ void ScrollingStateTree::setHasChangedProperties(bool changedProperties)
     m_hasChangedProperties = changedProperties;
 
 #if ENABLE(ASYNC_SCROLLING)
-    if (gainedChangedProperties && m_scrollingCoordinator)
-        m_scrollingCoordinator->scrollingStateTreePropertiesChanged();
+    auto scrollingCoordinator = m_scrollingCoordinator.get();
+    if (gainedChangedProperties && scrollingCoordinator)
+        scrollingCoordinator->scrollingStateTreePropertiesChanged();
 #endif
 }
 
@@ -117,7 +118,10 @@ ScrollingNodeID ScrollingStateTree::createUnparentedNode(ScrollingNodeType nodeT
         // If the type has changed, we need to destroy and recreate the node with a new ID.
         if (nodeType != node->nodeType()) {
             unparentChildrenAndDestroyNode(newNodeID);
-            newNodeID = m_scrollingCoordinator->uniqueScrollingNodeID();
+            if (auto scrollingCoordinator = m_scrollingCoordinator.get())
+                newNodeID = scrollingCoordinator->uniqueScrollingNodeID();
+            else
+                ASSERT_NOT_REACHED();
         }
 #endif
     }
@@ -157,8 +161,12 @@ ScrollingNodeID ScrollingStateTree::insertNode(ScrollingNodeType nodeType, Scrol
 
 #if ENABLE(ASYNC_SCROLLING)
         // If the type has changed, we need to destroy and recreate the node with a new ID.
-        if (nodeType != node->nodeType())
-            newNodeID = m_scrollingCoordinator->uniqueScrollingNodeID();
+        if (nodeType != node->nodeType()) {
+            if (auto scrollingCoordinator = m_scrollingCoordinator.get())
+                newNodeID = scrollingCoordinator->uniqueScrollingNodeID();
+            else
+                ASSERT_NOT_REACHED();
+        }
 #endif
 
         // The node is being re-parented. To do that, we'll remove it, and then create a new node.

Source/WebCore/page/scrolling/ScrollingStateTree.h

@@ -27,7 +27,6 @@
 
 #if ENABLE(ASYNC_SCROLLING)
 
-#include "ScrollingCoordinator.h"
 #include "ScrollingStateNode.h"
 #include <wtf/RefPtr.h>
 
@@ -101,7 +100,7 @@ class ScrollingStateTree {
     bool isValid() const;
     void traverse(const ScrollingStateNode&, const Function<void(const ScrollingStateNode&)>&) const;
 
-    AsyncScrollingCoordinator* m_scrollingCoordinator;
+    ThreadSafeWeakPtr<AsyncScrollingCoordinator> m_scrollingCoordinator;
     // Contains all the nodes we know about (those in the m_rootStateNode tree, and in m_unparentedNodes subtrees).
     StateNodeMap m_stateNodeMap;
     // Owns roots of unparented subtrees.

Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp

@@ -503,6 +503,18 @@ bool ArgumentCoder<ScrollingStatePositionedNode>::decode(Decoder& decoder, Scrol
 
 namespace WebKit {
 
+RemoteScrollingCoordinatorTransaction::RemoteScrollingCoordinatorTransaction() = default;
+
+RemoteScrollingCoordinatorTransaction::RemoteScrollingCoordinatorTransaction(std::unique_ptr<WebCore::ScrollingStateTree>&& scrollingStateTree, bool clearScrollLatching)
+    : m_scrollingStateTree(WTFMove(scrollingStateTree))
+    , m_clearScrollLatching(clearScrollLatching) { }
+
+RemoteScrollingCoordinatorTransaction::RemoteScrollingCoordinatorTransaction(RemoteScrollingCoordinatorTransaction&&) = default;
+
+RemoteScrollingCoordinatorTransaction& RemoteScrollingCoordinatorTransaction::operator=(RemoteScrollingCoordinatorTransaction&&) = default;
+
+RemoteScrollingCoordinatorTransaction::~RemoteScrollingCoordinatorTransaction() = default;
+
 static void encodeNodeAndDescendants(IPC::Encoder& encoder, const ScrollingStateNode& stateNode, int& encodedNodeCount)
 {
     ++encodedNodeCount;
@@ -561,92 +573,88 @@ void RemoteScrollingCoordinatorTransaction::encode(IPC::Encoder& encoder) const
         encoder << Vector<ScrollingNodeID>();
 }
 
-bool RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder, RemoteScrollingCoordinatorTransaction& transaction)
+auto RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder) -> std::optional<RemoteScrollingCoordinatorTransaction>
 {
-    return transaction.decode(decoder);
-}
-
-bool RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder)
-{
-    if (!decoder.decode(m_clearScrollLatching))
-        return false;
+    bool clearScrollLatching;
+    if (!decoder.decode(clearScrollLatching))
+        return std::nullopt;
 
     int numNodes;
     if (!decoder.decode(numNodes))
-        return false;
+        return std::nullopt;
 
     bool hasNewRootNode;
     if (!decoder.decode(hasNewRootNode))
-        return false;
+        return std::nullopt;
 
-    m_scrollingStateTree = makeUnique<ScrollingStateTree>();
+    auto scrollingStateTree = makeUnique<ScrollingStateTree>();
 
     bool hasChangedProperties;
     if (!decoder.decode(hasChangedProperties))
-        return false;
+        return std::nullopt;
 
-    m_scrollingStateTree->setHasChangedProperties(hasChangedProperties);
+    scrollingStateTree->setHasChangedProperties(hasChangedProperties);
 
     for (int i = 0; i < numNodes; ++i) {
         ScrollingNodeType nodeType;
         if (!decoder.decode(nodeType))
-            return false;
+            return std::nullopt;
 
         ScrollingNodeID nodeID;
         if (!decoder.decode(nodeID))
-            return false;
+            return std::nullopt;
 
         ScrollingNodeID parentNodeID;
         if (!decoder.decode(parentNodeID))
-            return false;
+            return std::nullopt;
 
-        auto createdNodeID = m_scrollingStateTree->insertNode(nodeType, nodeID, parentNodeID, notFound);
+        auto createdNodeID = scrollingStateTree->insertNode(nodeType, nodeID, parentNodeID, notFound);
         if (createdNodeID != nodeID)
-            return false;
+            return std::nullopt;
 
-        auto newNode = m_scrollingStateTree->stateNodeForID(nodeID);
+        auto newNode = scrollingStateTree->stateNodeForID(nodeID);
         ASSERT(newNode);
         if (newNode->nodeType() != nodeType)
-            return false;
+            return std::nullopt;
 
         ASSERT(!parentNodeID || newNode->parent());
 
         switch (nodeType) {
         case ScrollingNodeType::MainFrame:
         case ScrollingNodeType::Subframe:
             if (!decoder.decode(downcast<ScrollingStateFrameScrollingNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::FrameHosting:
             if (!decoder.decode(downcast<ScrollingStateFrameHostingNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::Overflow:
             if (!decoder.decode(downcast<ScrollingStateOverflowScrollingNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::OverflowProxy:
             if (!decoder.decode(downcast<ScrollingStateOverflowScrollProxyNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::Fixed:
             if (!decoder.decode(downcast<ScrollingStateFixedNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::Sticky:
             if (!decoder.decode(downcast<ScrollingStateStickyNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         case ScrollingNodeType::Positioned:
             if (!decoder.decode(downcast<ScrollingStatePositionedNode>(*newNode)))
-                return false;
+                return std::nullopt;
             break;
         }
     }
 
-    m_scrollingStateTree->setHasNewRootStateNode(hasNewRootNode);
+    scrollingStateTree->setHasNewRootStateNode(hasNewRootNode);
 
-    return true;
+    return { RemoteScrollingCoordinatorTransaction { WTFMove(scrollingStateTree), clearScrollLatching } };
 }
 
 #if !defined(NDEBUG) || !LOG_DISABLED

Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.h

@@ -27,34 +27,38 @@
 
 #if ENABLE(UI_SIDE_COMPOSITING)
 
-#include <WebCore/ScrollingStateTree.h>
-
 namespace IPC {
 class Decoder;
 class Encoder;
 }
 
+namespace WebCore {
+class ScrollingStateTree;
+}
+
 namespace WebKit {
 
 class RemoteScrollingCoordinatorTransaction {
 public:
-    void setStateTreeToEncode(std::unique_ptr<WebCore::ScrollingStateTree> stateTree) { m_scrollingStateTree = WTFMove(stateTree); }
+    RemoteScrollingCoordinatorTransaction();
+    RemoteScrollingCoordinatorTransaction(std::unique_ptr<WebCore::ScrollingStateTree>&&, bool);
+    RemoteScrollingCoordinatorTransaction(RemoteScrollingCoordinatorTransaction&&);
+    RemoteScrollingCoordinatorTransaction& operator=(RemoteScrollingCoordinatorTransaction&&);
+    ~RemoteScrollingCoordinatorTransaction();
+
     std::unique_ptr<WebCore::ScrollingStateTree>& scrollingStateTree() { return m_scrollingStateTree; }
 
     void encode(IPC::Encoder&) const;
-    static WARN_UNUSED_RETURN bool decode(IPC::Decoder&, RemoteScrollingCoordinatorTransaction&);
+    static std::optional<RemoteScrollingCoordinatorTransaction> decode(IPC::Decoder&);
 
     bool clearScrollLatching() const { return m_clearScrollLatching; }
-    void setClearScrollLatching(bool clearLatching) { m_clearScrollLatching = clearLatching; }
 
 #if !defined(NDEBUG) || !LOG_DISABLED
     String description() const;
     void dump() const;
 #endif
 
 private:
-    WARN_UNUSED_RETURN bool decode(IPC::Decoder&);
-
     std::unique_ptr<WebCore::ScrollingStateTree> m_scrollingStateTree;
 
     // Data encoded here should be "imperative" (valid just for one transaction). Stateful things should live on scrolling tree nodes.

Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm

@@ -377,7 +377,7 @@
         RemoteScrollingCoordinatorTransaction scrollingTransaction;
 #if ENABLE(ASYNC_SCROLLING)
         if (m_webPage.scrollingCoordinator())
-            downcast<RemoteScrollingCoordinator>(*m_webPage.scrollingCoordinator()).buildTransaction(scrollingTransaction);
+            scrollingTransaction = downcast<RemoteScrollingCoordinator>(*m_webPage.scrollingCoordinator()).buildTransaction();
 #endif
         transactions.uncheckedAppend({ WTFMove(layerTransaction), WTFMove(scrollingTransaction) });
     }

Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.h

@@ -29,9 +29,6 @@
 
 #include "MessageReceiver.h"
 #include <WebCore/AsyncScrollingCoordinator.h>
-#include <WebCore/ScrollTypes.h>
-#include <WebCore/ScrollingConstraints.h>
-#include <WebCore/Timer.h>
 
 namespace IPC {
 class Decoder;
@@ -51,7 +48,7 @@ class RemoteScrollingCoordinator final : public WebCore::AsyncScrollingCoordinat
         return adoptRef(*new RemoteScrollingCoordinator(page));
     }
 
-    void buildTransaction(RemoteScrollingCoordinatorTransaction&);
+    RemoteScrollingCoordinatorTransaction buildTransaction();
 
     void scrollingStateInUIProcessChanged(const RemoteScrollingUIState&);
 

Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm

@@ -46,6 +46,7 @@
 #import <WebCore/RenderView.h>
 #import <WebCore/ScrollbarsController.h>
 #import <WebCore/ScrollingStateFrameScrollingNode.h>
+#import <WebCore/ScrollingStateTree.h>
 #import <WebCore/ScrollingTreeFixedNodeCocoa.h>
 #import <WebCore/ScrollingTreeStickyNodeCocoa.h>
 #import <WebCore/WheelEventTestMonitor.h>
@@ -102,12 +103,14 @@
     // FIXME: send to the UI process.
 }
 
-void RemoteScrollingCoordinator::buildTransaction(RemoteScrollingCoordinatorTransaction& transaction)
+RemoteScrollingCoordinatorTransaction RemoteScrollingCoordinator::buildTransaction()
 {
     willCommitTree();
 
-    transaction.setClearScrollLatching(std::exchange(m_clearScrollLatchingInNextTransaction, false));
-    transaction.setStateTreeToEncode(scrollingStateTree()->commit(LayerRepresentation::PlatformLayerIDRepresentation));
+    return {
+        scrollingStateTree()->commit(LayerRepresentation::PlatformLayerIDRepresentation),
+        std::exchange(m_clearScrollLatchingInNextTransaction, false)
+    };
 }
 
 // Notification from the UI process that we scrolled.