Changeset 176899 in webkit

Timestamp:

Dec 5, 2014 5:25:21 PM (9 years ago)

Author:

Simon Fraser

Message:

Programmatic scrolling and content changes are not always synchronized
​https://bugs.webkit.org/show_bug.cgi?id=139245
rdar://problem/18833612

Reviewed by Anders Carlsson.

Manual test that tries to sync layout with programmatic scrolling.

• ManualTests/programmatic-scroll-flicker.html: Added.

Source/WebCore:

For programmatic scrolls, AsyncScrollingCoordinator::requestScrollPositionUpdate()
calls updateScrollPositionAfterAsyncScroll(), then dispatches the requested
scroll position to the scrolling thread.

Once the scrolling thread commits, it calls back to the main thread via
scheduleUpdateScrollPositionAfterAsyncScroll(), which schedules a second
call to updateScrollPositionAfterAsyncScroll() on a timer. That's a problem,
because some other scroll may have happened in the meantime; when the timer
fires, it can sometimes restore a stale scroll position.

Fix by bailing early from scheduleUpdateScrollPositionAfterAsyncScroll()
for programmatic scrolls, since we know that requestScrollPositionUpdate()
already did the updateScrollPositionAfterAsyncScroll().

Test:

ManualTests/programmatic-scroll-flicker.html

• page/FrameView.cpp:

(WebCore::FrameView::reset): nullptr.
(WebCore::FrameView::setScrollPosition): Ditto.
(WebCore::FrameView::setWasScrolledByUser): Ditto.

• page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate): Use a local variable for
isProgrammaticScroll just to make sure we use the same value for the duration of this function.
(WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Do nothing
if this is a programmatic scroll.

Location:

trunk

Files:

• ChangeLog (modified) (1 diff)
• ManualTests/programmatic-scroll-flicker.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (3 diffs)
• Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (modified) (3 diffs)

trunk/ChangeLog

@@ +1 @@
+2014-12-05  Simon Fraser  <simon.fraser@apple.com>
+
+        Programmatic scrolling and content changes are not always synchronized
+        https://bugs.webkit.org/show_bug.cgi?id=139245
+        rdar://problem/18833612
+
+        Reviewed by Anders Carlsson.
+        
+        Manual test that tries to sync layout with programmatic scrolling.
+
+        * ManualTests/programmatic-scroll-flicker.html: Added.
+
 2014-12-04  Alberto Garcia  <berto@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-12-05  Simon Fraser  <simon.fraser@apple.com>
+
+        Programmatic scrolling and content changes are not always synchronized
+        https://bugs.webkit.org/show_bug.cgi?id=139245
+        rdar://problem/18833612
+
+        Reviewed by Anders Carlsson.
+        
+        For programmatic scrolls, AsyncScrollingCoordinator::requestScrollPositionUpdate()
+        calls updateScrollPositionAfterAsyncScroll(), then dispatches the requested
+        scroll position to the scrolling thread.
+        
+        Once the scrolling thread commits, it calls back to the main thread via
+        scheduleUpdateScrollPositionAfterAsyncScroll(), which schedules a second
+        call to updateScrollPositionAfterAsyncScroll() on a timer. That's a problem,
+        because some other scroll may have happened in the meantime; when the timer
+        fires, it can sometimes restore a stale scroll position.
+        
+        Fix by bailing early from scheduleUpdateScrollPositionAfterAsyncScroll()
+        for programmatic scrolls, since we know that requestScrollPositionUpdate()
+        already did the updateScrollPositionAfterAsyncScroll().
+
+        Test:
+            ManualTests/programmatic-scroll-flicker.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::reset): nullptr.
+        (WebCore::FrameView::setScrollPosition): Ditto.
+        (WebCore::FrameView::setWasScrolledByUser): Ditto.
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate): Use a local variable for
+        isProgrammaticScroll just to make sure we use the same value for the duration of this function.
+        (WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Do nothing
+        if this is a programmatic scroll.
+
 2014-12-05  Timothy Horton  <timothy_horton@apple.com>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -290 +290 @@
     m_isVisuallyNonEmpty = false;
     m_firstVisuallyNonEmptyLayoutCallbackPending = true;
-    m_maintainScrollPositionAnchor = 0;
+    m_maintainScrollPositionAnchor = nullptr;
     m_throttledTimers.clear();
 }
@@ -2024 +2024 @@
 {
     TemporaryChange<bool> changeInProgrammaticScroll(m_inProgrammaticScroll, true);
-    m_maintainScrollPositionAnchor = 0;
+    m_maintainScrollPositionAnchor = nullptr;
     ScrollView::setScrollPosition(scrollPoint);
 }
@@ -3714 +3714 @@
     if (m_inProgrammaticScroll)
         return;
-    m_maintainScrollPositionAnchor = 0;
+    m_maintainScrollPositionAnchor = nullptr;
     if (m_wasScrolledByUser == wasScrolledByUser)
         return;

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

@@ -177 +177 @@
         return false;
 
-    if (frameView->inProgrammaticScroll() || frameView->frame().document()->inPageCache())
-        updateScrollPositionAfterAsyncScroll(frameView->scrollLayerID(), scrollPosition, frameView->inProgrammaticScroll(), SetScrollingLayerPosition);
+    bool isProgrammaticScroll = frameView->inProgrammaticScroll();
+    if (isProgrammaticScroll || frameView->frame().document()->inPageCache())
+        updateScrollPositionAfterAsyncScroll(frameView->scrollLayerID(), scrollPosition, isProgrammaticScroll, SetScrollingLayerPosition);
 
     // If this frame view's document is being put into the page cache, we don't want to update our
@@ -189 +190 @@
         return false;
 
-    stateNode->setRequestedScrollPosition(scrollPosition, frameView->inProgrammaticScroll());
+    stateNode->setRequestedScrollPosition(scrollPosition, isProgrammaticScroll);
     return true;
 }
@@ -197 +198 @@
     ScheduledScrollUpdate scrollUpdate(nodeID, scrollPosition, programmaticScroll, scrollingLayerPositionAction);
     
+    // For programmatic scrolls, requestScrollPositionUpdate() has already called updateScrollPositionAfterAsyncScroll().
+    if (programmaticScroll)
+        return;
+
     if (m_updateNodeScrollPositionTimer.isActive()) {
         if (m_scheduledScrollUpdate.matchesUpdateType(scrollUpdate)) {