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):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262094 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 045beb2..029b6ec 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,29 @@
+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>
Nullptr deref in WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation when parent and beforeChild are siblings
diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
index 008790c..e880ba0 100644
--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
@@ -264,7 +264,7 @@
bool inBackForwardCache = frameView->frame().document()->backForwardCacheState() != Document::NotInBackForwardCache;
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));
@@ -299,7 +299,7 @@
m_scrollingTree->traverseScrollingTree([&](ScrollingNodeID nodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin, bool scrolledSinceLastCommit) {
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);
}
});
}
@@ -331,10 +331,6 @@
m_updateNodeScrollPositionTimer.stop();
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;
}
@@ -345,7 +341,6 @@
void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired()
{
updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
- noteScrollingThreadSyncCompleteForNode(m_scheduledScrollUpdate.nodeID);
}
FrameView* AsyncScrollingCoordinator::frameViewForScrollingNode(ScrollingNodeID scrollingNodeID) const
@@ -380,10 +375,13 @@
return nullptr;
}
-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)
return;
diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
index 8a72041..09a74e5 100644
--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
@@ -53,6 +53,9 @@
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,8 +80,6 @@
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;
WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal) const override;
WEBCORE_EXPORT void willCommitTree() override;
diff --git a/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp b/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
index 6b267b0..93c8160 100644
--- a/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
+++ b/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
@@ -148,6 +148,11 @@
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())
scrollingCoordinator->scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
