Changeset 262094 in webkit

Timestamp:

May 22, 2020 9:23:48 PM (4 years ago)

Author:

Simon Fraser

Message:

Stuttery overflow scrolling in slow-scrolling regions (facebook messenger, feedly.com)
​https://bugs.webkit.org/show_bug.cgi?id=212291
<rdar://problem/61940624>

Reviewed by Tim Horton.

Now that we do scrolling tree commits on the main thread, ThreadedScrollingTree::scrollingTreeNodeDidScroll()
can be called on the main thread. In this case, don't do an RunLoop::main().dispatch() which introduces
asynchrony; just call into the ScrollingCoordinator synchronously.

Do some minor refactoring to move noteScrollingThreadSyncCompleteForNode() into updateScrollPositionAfterAsyncScroll().

Hard to test because it involves scrolling thread/main thread interactions.

• page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
(WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
(WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll):
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired):
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):

• page/scrolling/AsyncScrollingCoordinator.h:
• page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/AsyncScrollingCoordinator.cpp (modified) (5 diffs)
• page/scrolling/AsyncScrollingCoordinator.h (modified) (2 diffs)
• page/scrolling/ThreadedScrollingTree.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-05-22  Simon Fraser  <simon.fraser@apple.com>
+
+        Stuttery overflow scrolling in slow-scrolling regions (facebook messenger, feedly.com)
+        https://bugs.webkit.org/show_bug.cgi?id=212291
+        <rdar://problem/61940624>
+
+        Reviewed by Tim Horton.
+
+        Now that we do scrolling tree commits on the main thread, ThreadedScrollingTree::scrollingTreeNodeDidScroll()
+        can be called on the main thread. In this case, don't do an RunLoop::main().dispatch() which introduces
+        asynchrony; just call into the ScrollingCoordinator synchronously.
+
+        Do some minor refactoring to move noteScrollingThreadSyncCompleteForNode() into updateScrollPositionAfterAsyncScroll().
+
+        Hard to test because it involves scrolling thread/main thread interactions.
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
+        (WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
+        (WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll):
+        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired):
+        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
+        * page/scrolling/AsyncScrollingCoordinator.h:
+        * page/scrolling/ThreadedScrollingTree.cpp:
+        (WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):
+
 2020-05-22  Zalan Bujtas  <zalan@apple.com>
 

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

@@ -265 +265 @@
     bool inProgrammaticScroll = scrollableArea.currentScrollType() == ScrollType::Programmatic;
     if (inProgrammaticScroll || inBackForwardCache)
-        updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set);
+        updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
 
     ASSERT(inProgrammaticScroll == (scrollType == ScrollType::Programmatic));
@@ -300 +300 @@
         if (scrollPosition && scrolledSinceLastCommit) {
             LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::synchronizeStateFromScrollingTree - node " << nodeID << " scroll position " << scrollPosition);
-            updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition.value(), layoutViewportOrigin, ScrollType::User, ScrollingLayerPositionAction::Set);
+            updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition.value(), layoutViewportOrigin, ScrollType::User, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
         }
     });
@@ -332 +332 @@
         updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
         updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
-        
-        if (m_scheduledScrollUpdate.nodeID != nodeID)
-            noteScrollingThreadSyncCompleteForNode(m_scheduledScrollUpdate.nodeID);
-        noteScrollingThreadSyncCompleteForNode(nodeID);
         return;
     }
@@ -346 +342 @@
 {
     updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
-    noteScrollingThreadSyncCompleteForNode(m_scheduledScrollUpdate.nodeID);
 }
 
@@ -381 +376 @@
 }
 
-void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction)
+void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction, InformWheelEventMonitor informWheelEventMonitor)
 {
     ASSERT(isMainThread());
+
+    if (informWheelEventMonitor == InformWheelEventMonitor::Yes)
+        noteScrollingThreadSyncCompleteForNode(scrollingNodeID);
 
     if (!m_page)

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

@@ -54 +54 @@
     WEBCORE_EXPORT void scheduleUpdateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, const Optional<FloatPoint>& layoutViewportOrigin, ScrollingLayerPositionAction);
 
+    enum class InformWheelEventMonitor { Yes, No };
+    void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor = InformWheelEventMonitor::Yes);
+
 #if PLATFORM(COCOA)
     WEBCORE_EXPORT void handleWheelEventPhase(ScrollingNodeID, PlatformWheelEventPhase) final;
@@ -77 +80 @@
 
     RefPtr<ScrollingTree> releaseScrollingTree() { return WTFMove(m_scrollingTree); }
-
-    void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction);
 
     WEBCORE_EXPORT String scrollingStateTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const override;

trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp

@@ -149 +149 @@
     LOG_WITH_STREAM(Scrolling, stream << "ThreadedScrollingTree::scrollingTreeNodeDidScroll " << node.scrollingNodeID() << " to " << scrollPosition << " bouncing to main thread");
 
+    if (RunLoop::isMain()) {
+        m_scrollingCoordinator->updateScrollPositionAfterAsyncScroll(node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
+        return;
+    }
+
     RunLoop::main().dispatch([strongThis = makeRef(*this), nodeID = node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction] {
         if (auto* scrollingCoordinator = strongThis->m_scrollingCoordinator.get())