Bug 1647034 - Change how the visual viewport is updated to eliminate spurious reflows. r=tnikkel

There's two code changes in this patch:

- The update to the visual viewport that was happening just before positioning
  the fixed items gets moved to happen after determining the scrollbars for
  the root scrollframe. This moves it a little bit earlier, to basically the
  earliest point at which the visual viewport can actually be computed, since
  it depends on the presence of the root scrollframe's scrollbars.

  More importantly, this change sets the visual viewport without checking to
  see if one was already set, as the old code did. This means every reflow
  of the root scrollframe on a presShell with an MVM will now have a visual
  viewport set. Previously the visual viewport would only get set for the first
  time when the MVM got a load or first-paint event, and then would get updated
  for subsequent reflows. The net effect here is that the visual viewport is
  set earlier, and this can sometimes eliminate extra reflows from after the
  load event, because everything is already in a consistent state.

- The NotifyResizeReflow call to MVM is replaced by a NotifyReflow call that
  runs before every reflow, instead of just on resizes. Note that the
  NotifyReflow also doesn't update the visual viewport like NotifyResizeReflow
  used to do, because that is taken care of by the above-mentioned code change
  to set the visual viewport.

  This is desirable because there are things that run during reflow that attempt
  to read the display size from the MVM, and they were getting a zero size
  for reflows that happened before the first resize or load/first-paint events.
  Now they get a valid display size on every reflow, and so again this allows
  fewer overall reflows as the code converges to a stable state faster.

Together these changes ensure that every reflow has access to up-to-date
properties (display size, mobile viewport size, visual viewport size) from the
MVM. This eliminates unnecessary reflows because of out-of-order computations
based on stale values and such. Therefore the number of reflows goes down,
which is reflected by the changes to the crashtest assertion counts.

Differential Revision: https://phabricator.services.mozilla.com/D81375

layout/base/MobileViewportManager.cpp

@@ -557,17 +557,15 @@ void MobileViewportManager::RefreshVisualViewportSize() {
   UpdateVisualViewportSize(displaySize, GetZoom());
 }
 
-void MobileViewportManager::NotifyResizeReflow() {
-  // If there's a resize-reflow, the visual viewport may need to be recomputed
-  // for a new display size, so let's do that.
+void MobileViewportManager::UpdateSizesBeforeReflow() {
   if (Maybe<LayoutDeviceIntSize> newDisplaySize =
           mContext->GetContentViewerSize()) {
-    // Note that we intentionally don't short-circuit here if the display size
-    // (in LD units) is unchanged, because a resize-reflow may also be triggered
-    // by a change in the CSS/LD pixel ratio which would affect GetZoom() and
-    // therefore the computed visual viewport.
+    if (mDisplaySize == *newDisplaySize) {
+      return;
+    }
+
     mDisplaySize = *newDisplaySize;
-    MVM_LOG("%p: Display size updated to %s\n", this,
+    MVM_LOG("%p: Reflow starting, display size updated to %s\n", this,
             Stringify(mDisplaySize).c_str());
 
     if (mDisplaySize.width == 0 || mDisplaySize.height == 0) {
@@ -580,8 +578,6 @@ void MobileViewportManager::NotifyResizeReflow() {
     mMobileViewportSize = viewportInfo.GetSize();
     MVM_LOG("%p: MVSize updated to %s\n", this,
             Stringify(mMobileViewportSize).c_str());
-
-    RefreshVisualViewportSize();
   }
 }
 

layout/base/MobileViewportManager.h

@@ -74,11 +74,12 @@ class MobileViewportManager final : public nsIDOMEventListener,
   void SetRestoreResolution(float aResolution);
 
  public:
-  /* Notify the MobileViewportManager that a resize-reflow is about to happen,
-   * possibly indicating a change in the display size or some other quantity
-   * that the visual viewport depends on.
+  /* Notify the MobileViewportManager that a reflow is about to happen. This
+   * triggers the MVM to update its internal notion of display size and CSS
+   * viewport, so that code that queries those during the reflow gets an
+   * up-to-date value.
    */
-  void NotifyResizeReflow();
+  void UpdateSizesBeforeReflow();
 
   /* Notify the MobileViewportManager that a reflow was requested in the
    * presShell.*/

layout/base/PresShell.cpp

@@ -1956,9 +1956,6 @@ nsresult PresShell::ResizeReflow(nscoord aWidth, nscoord aHeight,
     mMobileViewportManager->RequestReflow(false);
     return NS_OK;
   }
-  if (mMobileViewportManager) {
-    mMobileViewportManager->NotifyResizeReflow();
-  }
 
   return ResizeReflowIgnoreOverride(aWidth, aHeight, aOptions);
 }
@@ -9488,6 +9485,10 @@ bool PresShell::DoReflow(nsIFrame* target, bool aInterruptible,
     timeStart = TimeStamp::Now();
   }
 
+  if (mMobileViewportManager) {
+    mMobileViewportManager->UpdateSizesBeforeReflow();
+  }
+
   // Schedule a paint, but don't actually mark this frame as changed for
   // retained DL building purposes. If any child frames get moved, then
   // they will schedule paint again. We could probaby skip this, and just

layout/generic/ViewportFrame.cpp

@@ -386,15 +386,6 @@ nsSize ViewportFrame::AdjustViewportSizeForFixedPosition(
   // it has been set and it is larger than the computed size, otherwise use the
   // computed size.
   if (presShell->IsVisualViewportSizeSet()) {
-    if (RefPtr<MobileViewportManager> manager =
-            presShell->GetMobileViewportManager()) {
-      // Note that this runs during layout, and when we get here the root
-      // scrollframe has already been laid out. It may have added or removed
-      // scrollbars as a result of that layout, so we need to ensure the
-      // visual viewport is updated to account for that before we read the
-      // visual viewport size.
-      manager->UpdateVisualViewportSizeForPotentialScrollbarChange();
-    }
     if (presShell->GetDynamicToolbarState() == DynamicToolbarState::Collapsed &&
         result < presShell->GetVisualViewportSizeUpdatedByDynamicToolbar()) {
       // We need to use the viewport size updated by the dynamic toolbar in the

layout/generic/crashtests/crashtests.list

@@ -571,7 +571,7 @@ load 1001258-1.html
 load 1001994.html
 load chrome://reftest/content/crashtests/layout/generic/crashtests/1003441.xhtml
 load 1015562.html
-asserts(1-3) load 1015563-1.html
+asserts(1-2) load 1015563-1.html
 asserts(1-2) load 1015563-2.html
 load 1015844.html
 asserts-if(Android,0-358) pref(font.size.inflation.minTwips,200) load 1032450.html # Bug 1607658
@@ -704,7 +704,7 @@ load 1474768.html
 load 1478178.html
 load 1483972.html
 load 1486457.html
-asserts(14) load 1488762-1.html # asserts from integer overflow & bogus sizes
+asserts(9) load 1488762-1.html # asserts from integer overflow & bogus sizes
 load 1489287.html
 load 1489863.html
 load 1489770.html

layout/generic/nsGfxScrollFrame.cpp

@@ -1251,6 +1251,17 @@ void nsHTMLScrollFrame::Reflow(nsPresContext* aPresContext,
       mHelper.mSkippedScrollbarLayout = true;
     }
   }
+  if (mHelper.mIsRoot) {
+    if (RefPtr<MobileViewportManager> manager =
+            PresShell()->GetMobileViewportManager()) {
+      // Note that this runs during layout, and when we get here the root
+      // scrollframe has already been laid out. It may have added or removed
+      // scrollbars as a result of that layout, so we need to ensure the
+      // visual viewport is updated to account for that before we read the
+      // visual viewport size.
+      manager->UpdateVisualViewportSizeForPotentialScrollbarChange();
+    }
+  }
 
   aDesiredSize.SetOverflowAreasToDesiredBounds();
 

layout/painting/crashtests/crashtests.list

@@ -19,7 +19,7 @@ load 1504033.html
 load 1514544-1.html
 asserts(4-9) load 1547420-1.html
 load 1549909.html
-asserts(9) load 1551389-1.html # bug 847368
+asserts(6) load 1551389-1.html # bug 847368
 asserts(0-2) load 1555819-1.html
 load 1574392.html
 load 1589800-1.html

layout/svg/crashtests/crashtests.list

@@ -102,7 +102,7 @@ load 587336-1.html
 load 590291-1.svg
 load 601999-1.html
 load 605626-1.svg
-asserts(3) load 606914.xhtml # bug 606914, bug 718883
+asserts(2) load 606914.xhtml # bug 606914, bug 718883
 load 610594-1.html
 load 610954-1.html
 load 612662-1.svg