Bug 1536157 - Make sure we repaint after accepting a visual scroll update. r=kats
authorBotond Ballo <botond@mozilla.com>
Fri, 22 Mar 2019 05:20:31 +0000
changeset 465542 7635315e975f4caf22e2f7f325bcb3ce9a231399
parent 465541 847ee44069dc4a93b65905f0da17b7846ae2d836
child 465543 c48b22d152f23fe1f8546e5d6a239df8e3ac52b9
push id35741
push userapavel@mozilla.com
push dateFri, 22 Mar 2019 09:56:25 +0000
treeherdermozilla-central@6332e136b825 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1536157 - Make sure we repaint after accepting a visual scroll update. r=kats A possible alternative would be to have the main thread already paint a displayport at the target position of a requested visual update as part of the same transaction that requests the update. There are a couple of reasons we may not want to do that: 1) APZ could reject the requested visual update under certain conditions, e.g. if there is a higher-priority layout update. 2) It would break the property that the displayport in the main thread's scroll metadata is relative to the scroll offset in said metadata. Various places assume this and untangling that seems tricky. This does mean that if the main thread requests a visual update to "far away" (outside the existing displayport), we can get temporary checkerboarding before the content at the target position is painted. However, it's straightforward for callers to work around that, by changing the layout scroll offset _and_ scheduling a visual update if they wish to visual-scroll far away. Differential Revision: https://phabricator.services.mozilla.com/D23902
--- a/gfx/layers/RepaintRequest.h
+++ b/gfx/layers/RepaintRequest.h
@@ -27,18 +27,22 @@ namespace layers {
 struct RepaintRequest {
   friend struct IPC::ParamTraits<mozilla::layers::RepaintRequest>;
   // clang-format off
     ScrollOffsetUpdateType, uint8_t, (
-      eNone,             // The default; the scroll offset was not updated.
-      eUserAction        // The scroll offset was updated by APZ.
+        eNone,             // The default; the scroll offset was not updated.
+        eUserAction,       // The scroll offset was updated by APZ in response
+                           // to user action.
+        eVisualUpdate      // The scroll offset was updated by APZ in response
+                           // to a visual scroll update request from the
+                           // main thread.
   // clang-format on
       : mScrollId(ScrollableLayerGuid::NULL_SCROLL_ID),
         mCompositionBounds(0, 0, 0, 0),
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -4643,17 +4643,27 @@ void AsyncPanZoomController::NotifyLayer
     // |if (scrollOffsetUpdated)| branch above.
     mCompositedLayoutViewport = Metrics().GetLayoutViewport();
     mCompositedScrollOffset = Metrics().GetScrollOffset();
     mExpectedGeckoMetrics = aLayerMetrics;
     if (!mAnimation || !mAnimation->HandleScrollOffsetUpdate(Nothing())) {
+    // The main thread did not actually paint a displayport at the target
+    // visual offset, so we need to ask it to repaint. We need to set the
+    // contentRepaintType to something other than eNone, otherwise the main
+    // thread will short-circuit the repaint request.
+    // Don't do this for eRestore visual updates as a repaint coming from APZ
+    // breaks the scroll offset restoration mechanism.
     needContentRepaint = true;
+    if (aLayerMetrics.GetVisualScrollUpdateType() ==
+        FrameMetrics::eMainThread) {
+      contentRepaintType = RepaintUpdateType::eVisualUpdate;
+    }
   if (viewportUpdated) {
     // While we want to accept the main thread's layout viewport _size_,
     // its position may be out of date in light of async scrolling, to
     // adjust it if necessary to make sure it continues to enclose the
     // visual viewport.
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -1689,16 +1689,20 @@ class nsIPresShell : public nsStubDocume
     FrameMetrics::ScrollOffsetUpdateType mUpdateType;
   // Ask APZ in the next transaction to scroll to the given visual viewport
   // offset (relative to the document).
   // Use this sparingly, as it will clobber JS-driven scrolling that happens
   // in the same frame. This is mostly intended to be used in special
   // situations like "first paint" or session restore.
+  // If scrolling "far away", i.e. not just within the existing layout
+  // viewport, it's recommended to use both nsIScrollableFrame.ScrollTo*()
+  // (via window.scrollTo if calling from JS) *and* this function; otherwise,
+  // temporary checkerboarding may result.
   // Please request APZ review if adding a new call site.
   void SetPendingVisualScrollUpdate(
       const nsPoint& aVisualViewportOffset,
       FrameMetrics::ScrollOffsetUpdateType aUpdateType);
   void ClearPendingVisualScrollUpdate() {
     mPendingVisualScrollUpdate = mozilla::Nothing();
   const mozilla::Maybe<VisualScrollUpdate>& GetPendingVisualScrollUpdate()