Bug 1286179 - For APZ repaint requests that are triggered by main-thread updates, don't attempt to re-scroll the main thread. r=tnikkel, a=lizzard
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 18 Aug 2016 17:00:42 -0400
changeset 349940 7a83abda0b0893deb402638ed6ddbae18c491dfb
parent 349939 f5dd9aec59ef3ffc40975f4974ade1ec6820f963
child 349941 23cb9f2e57ff1f6f0f947b8657f874ade8baed84
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
bugs1286179
milestone50.0a2
Bug 1286179 - For APZ repaint requests that are triggered by main-thread updates, don't attempt to re-scroll the main thread. r=tnikkel, a=lizzard Avoiding these re-scrolls prevents APZ repaint requests from clobbering the main-thread scroll position (which may have changed in the meantime) in some cases. See https://bugzilla.mozilla.org/show_bug.cgi?id=1286179#c8 for an example of a scenario where this re-scroll is problematic. MozReview-Commit-ID: 7he2A2sygji
gfx/layers/FrameMetrics.h
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/src/AsyncPanZoomController.h
gfx/layers/apz/util/APZCCallbackHelper.cpp
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -51,16 +51,20 @@ public:
                                         // will begin at.
 
   enum ScrollOffsetUpdateType : uint8_t {
     eNone,          // The default; the scroll offset was not updated
     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).
 
     eSentinel       // For IPC use only
   };
 
   FrameMetrics()
     : mScrollId(NULL_SCROLL_ID)
     , mPresShellResolution(1)
     , mCompositionBounds(0, 0, 0, 0)
@@ -241,16 +245,21 @@ public:
 
   void UpdatePendingScrollInfo(const ScrollUpdateInfo& aInfo)
   {
     mScrollOffset = aInfo.mScrollOffset;
     mScrollGeneration = aInfo.mScrollGeneration;
     mScrollUpdateType = ePending;
   }
 
+  void SetRepaintDrivenByUserAction(bool aUserAction)
+  {
+    mScrollUpdateType = aUserAction ? eUserAction : eNone;
+  }
+
 public:
   void SetPresShellResolution(float aPresShellResolution)
   {
     mPresShellResolution = aPresShellResolution;
   }
 
   float GetPresShellResolution() const
   {
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -2815,36 +2815,37 @@ bool AsyncPanZoomController::IsPannable(
   return mX.CanScroll() || mY.CanScroll();
 }
 
 int32_t AsyncPanZoomController::GetLastTouchIdentifier() const {
   RefPtr<GestureEventListener> listener = GetGestureEventListener();
   return listener ? listener->GetLastTouchIdentifier() : -1;
 }
 
-void AsyncPanZoomController::RequestContentRepaint() {
+void AsyncPanZoomController::RequestContentRepaint(bool aUserAction) {
   // Reinvoke this method on the main thread if it's not there already. It's
   // important to do this before the call to CalculatePendingDisplayPort, so
   // that CalculatePendingDisplayPort uses the most recent available version of
   // mFrameMetrics, just before the paint request is dispatched to content.
   if (!NS_IsMainThread()) {
     // use the local variable to resolve the function overload.
-    auto func = static_cast<void (AsyncPanZoomController::*)()>
+    auto func = static_cast<void (AsyncPanZoomController::*)(bool)>
         (&AsyncPanZoomController::RequestContentRepaint);
-    NS_DispatchToMainThread(NewRunnableMethod(this, func));
+    NS_DispatchToMainThread(NewRunnableMethod<bool>(this, func, aUserAction));
     return;
   }
 
   MOZ_ASSERT(NS_IsMainThread());
 
   ReentrantMonitorAutoEnter lock(mMonitor);
   ParentLayerPoint velocity = GetVelocityVector();
   mFrameMetrics.SetDisplayPortMargins(CalculatePendingDisplayPort(mFrameMetrics, velocity));
   mFrameMetrics.SetUseDisplayPortMargins(true);
   mFrameMetrics.SetPaintRequestTime(TimeStamp::Now());
+  mFrameMetrics.SetRepaintDrivenByUserAction(aUserAction);
   RequestContentRepaint(mFrameMetrics, velocity);
 }
 
 /*static*/ CSSRect
 GetDisplayPortRect(const FrameMetrics& aFrameMetrics)
 {
   // This computation is based on what happens in CalculatePendingDisplayPort. If that
   // changes then this might need to change too
@@ -2895,16 +2896,18 @@ AsyncPanZoomController::RequestContentRe
       info << " velocity " << aVelocity;
       std::string str = info.str();
       mCheckerboardEvent->UpdateRendertraceProperty(
           CheckerboardEvent::RequestedDisplayPort, GetDisplayPortRect(aFrameMetrics),
           str);
     }
   }
 
+  MOZ_ASSERT(aFrameMetrics.GetScrollUpdateType() == FrameMetrics::eNone ||
+             aFrameMetrics.GetScrollUpdateType() == FrameMetrics::eUserAction);
   controller->RequestContentRepaint(aFrameMetrics);
   mExpectedGeckoMetrics = aFrameMetrics;
   mLastPaintRequestMetrics = aFrameMetrics;
 }
 
 bool AsyncPanZoomController::UpdateAnimation(const TimeStamp& aSampleTime,
                                              nsTArray<RefPtr<Runnable>>* aOutDeferredTasks)
 {
@@ -3410,17 +3413,18 @@ void AsyncPanZoomController::NotifyLayer
     mFrameMetrics.CopySmoothScrollInfoFrom(aLayerMetrics);
     needContentRepaint = true;
     mExpectedGeckoMetrics = aLayerMetrics;
 
     SmoothScrollTo(mFrameMetrics.GetSmoothScrollOffset());
   }
 
   if (needContentRepaint) {
-    RequestContentRepaint();
+    // This repaint request is not driven by a user action on the APZ side
+    RequestContentRepaint(false);
   }
   UpdateSharedCompositorFrameMetrics();
 }
 
 const FrameMetrics& AsyncPanZoomController::GetFrameMetrics() const {
   mMonitor.AssertCurrentThreadIn();
   return mFrameMetrics;
 }
@@ -3558,16 +3562,17 @@ void AsyncPanZoomController::ZoomToRect(
 
     // Schedule a repaint now, so the new displayport will be painted before the
     // animation finishes.
     ParentLayerPoint velocity(0, 0);
     endZoomToMetrics.SetDisplayPortMargins(
       CalculatePendingDisplayPort(endZoomToMetrics, velocity));
     endZoomToMetrics.SetUseDisplayPortMargins(true);
     endZoomToMetrics.SetPaintRequestTime(TimeStamp::Now());
+    endZoomToMetrics.SetRepaintDrivenByUserAction(true);
     if (NS_IsMainThread()) {
       RequestContentRepaint(endZoomToMetrics, velocity);
     } else {
       // use a local var to resolve the function overload
       auto func = static_cast<void (AsyncPanZoomController::*)(const FrameMetrics&, const ParentLayerPoint&)>
           (&AsyncPanZoomController::RequestContentRepaint);
       NS_DispatchToMainThread(
           NewRunnableMethod<FrameMetrics, ParentLayerPoint>(
--- a/gfx/layers/apz/src/AsyncPanZoomController.h
+++ b/gfx/layers/apz/src/AsyncPanZoomController.h
@@ -579,17 +579,17 @@ protected:
 
   /**
    * Utility function to send updated FrameMetrics to Gecko so that it can paint
    * the displayport area. Calls into GeckoContentController to do the actual
    * work. This call will use the current metrics. If this function is called
    * from a non-main thread, it will redispatch itself to the main thread, and
    * use the latest metrics during the redispatch.
    */
-  void RequestContentRepaint();
+  void RequestContentRepaint(bool aUserAction = true);
 
   /**
    * Send the provided metrics to Gecko to trigger a repaint. This function
    * may filter duplicate calls with the same metrics. This function must be
    * called on the main thread.
    */
   void RequestContentRepaint(const FrameMetrics& aFrameMetrics,
                              const ParentLayerPoint& aVelocity);
--- a/gfx/layers/apz/util/APZCCallbackHelper.cpp
+++ b/gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ -61,34 +61,41 @@ RecenterDisplayPort(mozilla::layers::Fra
 {
   ScreenMargin margins = aFrameMetrics.GetDisplayPortMargins();
   margins.right = margins.left = margins.LeftRight() / 2;
   margins.top = margins.bottom = margins.TopBottom() / 2;
   aFrameMetrics.SetDisplayPortMargins(margins);
 }
 
 static CSSPoint
-ScrollFrameTo(nsIScrollableFrame* aFrame, const CSSPoint& aPoint, bool& aSuccessOut)
+ScrollFrameTo(nsIScrollableFrame* aFrame, const FrameMetrics& aMetrics, bool& aSuccessOut)
 {
   aSuccessOut = false;
+  CSSPoint targetScrollPosition = aMetrics.GetScrollOffset();
 
   if (!aFrame) {
-    return aPoint;
+    return targetScrollPosition;
   }
 
-  CSSPoint targetScrollPosition = aPoint;
+  CSSPoint geckoScrollPosition = CSSPoint::FromAppUnits(aFrame->GetScrollPosition());
+
+  // If the repaint request was triggered due to a previous main-thread scroll
+  // offset update sent to the APZ, then we don't need to do another scroll here
+  // and we can just return.
+  if (!aMetrics.GetScrollOffsetUpdated()) {
+    return geckoScrollPosition;
+  }
 
   // If the frame is overflow:hidden on a particular axis, we don't want to allow
   // user-driven scroll on that axis. Simply set the scroll position on that axis
   // to whatever it already is. Note that this will leave the APZ's async scroll
   // position out of sync with the gecko scroll position, but APZ can deal with that
   // (by design). Note also that when we run into this case, even if both axes
   // have overflow:hidden, we want to set aSuccessOut to true, so that the displayport
   // follows the async scroll position rather than the gecko scroll position.
-  CSSPoint geckoScrollPosition = CSSPoint::FromAppUnits(aFrame->GetScrollPosition());
   if (aFrame->GetScrollbarStyles().mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
     targetScrollPosition.y = geckoScrollPosition.y;
   }
   if (aFrame->GetScrollbarStyles().mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
     targetScrollPosition.x = geckoScrollPosition.x;
   }
 
   // If the scrollable frame is currently in the middle of an async or smooth
@@ -124,17 +131,17 @@ ScrollFrame(nsIContent* aContent,
   // Scroll the window to the desired spot
   nsIScrollableFrame* sf = nsLayoutUtils::FindScrollableFrameFor(aMetrics.GetScrollId());
   if (sf) {
     sf->ResetScrollInfoIfGeneration(aMetrics.GetScrollGeneration());
     sf->SetScrollableByAPZ(!aMetrics.IsScrollInfoLayer());
   }
   bool scrollUpdated = false;
   CSSPoint apzScrollOffset = aMetrics.GetScrollOffset();
-  CSSPoint actualScrollOffset = ScrollFrameTo(sf, apzScrollOffset, scrollUpdated);
+  CSSPoint actualScrollOffset = ScrollFrameTo(sf, aMetrics, scrollUpdated);
 
   if (scrollUpdated) {
     if (aMetrics.IsScrollInfoLayer()) {
       // In cases where the APZ scroll offset is different from the content scroll
       // offset, we want to interpret the margins as relative to the APZ scroll
       // offset except when the frame is not scrollable by APZ. Therefore, if the
       // layer is a scroll info layer, we leave the margins as-is and they will
       // be interpreted as relative to the content scroll offset.