Bug 1292781 - Send scroll-position-restore updates to APZ, but don't allow them to clobber user scrolls. r=tnikkel, a=lizzard
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 24 Aug 2016 09:15:29 -0400
changeset 349941 23cb9f2e57ff1f6f0f947b8657f874ade8baed84
parent 349940 7a83abda0b0893deb402638ed6ddbae18c491dfb
child 349942 c0cb39e5174697d693a1e3e006ff429ef7b6bca8
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel, lizzard
bugs1292781
milestone50.0a2
Bug 1292781 - Send scroll-position-restore updates to APZ, but don't allow them to clobber user scrolls. r=tnikkel, a=lizzard Previously we weren't sending scroll position updates with origin nsGkAtoms::restore over to the APZ at all, on the assumption that they should never clobber an APZ scroll offset. However, there are scenarios where that is not true. In particular, during a frame reconstruction, a layers update may be sent to the compositor between the time a scrollframe has RestoreState() called on it, and the time the scrollframe has ScrollToRestoredPosition() called on it. The layers update that happens during this interval (correctly) sends a scroll position of (0,0), and forces the APZ to scroll to that position. This is necessary to prevent APZ from remaining at an invalid scroll offset while the frame is still being rebuilt. However, once ScrollToRestoredPosition() is called and the old scroll offset is restored, that restored scroll position needs to get sent to the APZ in order to have it properly restore to the original scroll position. In order to do this, the main thread must flag the metrics with a scroll offset update. Since the user may have scrolled concurrently in the compositor from the (0,0) position, we also need to check for that case in the APZ code and avoid restoring the scroll position. This is equivalent to the corresponding main-thread code in ScrollToRestoredPosition(). MozReview-Commit-ID: LxRapVSrsJ3
gfx/layers/FrameMetrics.h
gfx/layers/apz/src/AsyncPanZoomController.cpp
layout/base/nsLayoutUtils.cpp
layout/generic/nsGfxScrollFrame.cpp
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -55,16 +55,20 @@ public:
     eMainThread,    // The scroll offset was updated by the main thread.
     ePending,       // The scroll offset was updated on the main thread, but not
                     // painted, so the layer texture data is still at the old
                     // offset.
     eUserAction,    // In an APZ repaint request, this means the APZ generated
                     // the scroll position based on user action (the alternative
                     // is eNone which means it's just request a repaint because
                     // it got a scroll update from the main thread).
+    eRestore,       // The scroll offset was updated by the main thread, but as
+                    // a restore from history or after a frame reconstruction.
+                    // In this case, APZ can ignore the offset change if the
+                    // user has done an APZ scroll already.
 
     eSentinel       // For IPC use only
   };
 
   FrameMetrics()
     : mScrollId(NULL_SCROLL_ID)
     , mPresShellResolution(1)
     , mCompositionBounds(0, 0, 0, 0)
@@ -357,16 +361,22 @@ public:
   }
 
   void SetScrollOffsetUpdated(uint32_t aScrollGeneration)
   {
     mScrollUpdateType = eMainThread;
     mScrollGeneration = aScrollGeneration;
   }
 
+  void SetScrollOffsetRestored(uint32_t aScrollGeneration)
+  {
+    mScrollUpdateType = eRestore;
+    mScrollGeneration = aScrollGeneration;
+  }
+
   void SetSmoothScrollOffsetUpdated(int32_t aScrollGeneration)
   {
     mDoSmoothScroll = true;
     mScrollGeneration = aScrollGeneration;
   }
 
   ScrollOffsetUpdateType GetScrollUpdateType() const
   {
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -3223,16 +3223,30 @@ void AsyncPanZoomController::NotifyLayer
 
   const FrameMetrics& aLayerMetrics = aScrollMetadata.GetMetrics();
 
   if ((aScrollMetadata == mLastContentPaintMetadata) && !isDefault) {
     // No new information here, skip it.
     APZC_LOG("%p NotifyLayersUpdated short-circuit\n", this);
     return;
   }
+
+  // If the mFrameMetrics scroll offset is different from the last scroll offset
+  // that the main-thread sent us, then we know that the user has been doing
+  // something that triggers a scroll. This check is the APZ equivalent of the
+  // check on the main-thread at
+  // https://hg.mozilla.org/mozilla-central/file/97a52326b06a/layout/generic/nsGfxScrollFrame.cpp#l4050
+  // There is code below (the use site of userScrolled) that prevents a restored-
+  // scroll-position update from overwriting a user scroll, again equivalent to
+  // how the main thread code does the same thing.
+  CSSPoint lastScrollOffset = mLastContentPaintMetadata.GetMetrics().GetScrollOffset();
+  bool userScrolled =
+    !FuzzyEqualsAdditive(mFrameMetrics.GetScrollOffset().x, lastScrollOffset.x) ||
+    !FuzzyEqualsAdditive(mFrameMetrics.GetScrollOffset().y, lastScrollOffset.y);
+
   if (aLayerMetrics.GetScrollUpdateType() != FrameMetrics::ScrollOffsetUpdateType::ePending) {
     mLastContentPaintMetadata = aScrollMetadata;
   }
 
   mScrollMetadata.SetScrollParentId(aScrollMetadata.GetScrollParentId());
   APZC_LOG_FM(aLayerMetrics, "%p got a NotifyLayersUpdated with aIsFirstPaint=%d, aThisLayerTreeUpdated=%d",
     this, aIsFirstPaint, aThisLayerTreeUpdated);
 
@@ -3288,16 +3302,22 @@ void AsyncPanZoomController::NotifyLayer
 
   // If the layers update was not triggered by our own repaint request, then
   // we want to take the new scroll offset. Check the scroll generation as well
   // to filter duplicate calls to NotifyLayersUpdated with the same scroll offset
   // update message.
   bool scrollOffsetUpdated = aLayerMetrics.GetScrollOffsetUpdated()
         && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration());
 
+  if (scrollOffsetUpdated && userScrolled &&
+      aLayerMetrics.GetScrollUpdateType() == FrameMetrics::ScrollOffsetUpdateType::eRestore) {
+    APZC_LOG("%p dropping scroll update of type eRestore because of user scroll\n", this);
+    scrollOffsetUpdated = false;
+  }
+
   bool smoothScrollRequested = aLayerMetrics.GetDoSmoothScroll()
        && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration());
 
   // TODO if we're in a drag and scrollOffsetUpdated is set then we want to
   // ignore it
 
   if ((aIsFirstPaint && aThisLayerTreeUpdated) || isDefault) {
     // Initialize our internal state to something sane when the content
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -8861,20 +8861,25 @@ nsLayoutUtils::ComputeScrollMetadata(nsI
     nsPoint scrollPosition = scrollableFrame->GetScrollPosition();
     metrics.SetScrollOffset(CSSPoint::FromAppUnits(scrollPosition));
 
     nsPoint smoothScrollPosition = scrollableFrame->LastScrollDestination();
     metrics.SetSmoothScrollOffset(CSSPoint::FromAppUnits(smoothScrollPosition));
 
     // If the frame was scrolled since the last layers update, and by something
     // that is higher priority than APZ, we want to tell the APZ to update
-    // its scroll offset.
-    if (CanScrollOriginClobberApz(scrollableFrame->LastScrollOrigin())) {
+    // its scroll offset. We want to distinguish the case where the scroll offset
+    // was "restored" because in that case the restored scroll position should
+    // not overwrite a user-driven scroll.
+    if (scrollableFrame->LastScrollOrigin() == nsGkAtoms::restore) {
+      metrics.SetScrollOffsetRestored(scrollableFrame->CurrentScrollGeneration());
+    } else if (CanScrollOriginClobberApz(scrollableFrame->LastScrollOrigin())) {
       metrics.SetScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration());
     }
+
     nsIAtom* lastSmoothScrollOrigin = scrollableFrame->LastSmoothScrollOrigin();
     if (lastSmoothScrollOrigin) {
       metrics.SetSmoothScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration());
     }
 
     nsSize lineScrollAmount = scrollableFrame->GetLineScrollAmount();
     LayoutDeviceIntSize lineScrollAmountInDevPixels =
       LayoutDeviceIntSize::FromAppUnitsRounded(lineScrollAmount, presContext->AppUnitsPerDevPixel());
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2119,17 +2119,17 @@ ScrollFrameHelper::ScrollToWithOrigin(ns
   if (aSnap == nsIScrollableFrame::ENABLE_SNAP) {
     GetSnapPointForDestination(nsIScrollableFrame::DEVICE_PIXELS,
                                mDestination,
                                aScrollPosition);
   }
 
   nsRect scrollRange = GetScrollRangeForClamping();
   mDestination = scrollRange.ClampPoint(aScrollPosition);
-  if (mDestination != aScrollPosition && aOrigin == nsGkAtoms::restore) {
+  if (mDestination != aScrollPosition && aOrigin == nsGkAtoms::restore && !PageIsStillLoading()) {
     // If we're doing a restore but the scroll position is clamped, promote
     // the origin from one that APZ can clobber to one that it can't clobber.
     aOrigin = nsGkAtoms::other;
   }
 
   nsRect range = aRange ? *aRange : nsRect(aScrollPosition, nsSize(0, 0));
 
   if (aMode != nsIScrollableFrame::SMOOTH_MSD) {