Bug 1551659 - Remove MVMContext::ResizeEventFlag and related code. r=botond,hiro
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 25 Sep 2019 19:35:29 +0000
changeset 494985 ab74e2147c5c1761ac324bbdf993d6aa0891ca96
parent 494984 76d03f0c306db43e2645fdf836bffea74aa11bc1
child 494986 7914ef375e3c8fb2c6148b97decff55594a27f54
push id96351
push userealvarez@mozilla.com
push dateWed, 25 Sep 2019 20:16:48 +0000
treeherderautoland@ab74e2147c5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond, hiro
bugs1551659, 1583534, 1528052
milestone71.0a1
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 1551659 - Remove MVMContext::ResizeEventFlag and related code. r=botond,hiro D46944 / bug 1583534 is what fixes the root cause of bug 1528052 by not having the first call to ResizeReflow have a wrong old size of 0x0. This removes the code that bug introduces to suppress resize events, which fixes this bug. I think our behavior now is pretty sane. In particular, consider the test-case: <!doctype html> <meta name="viewport" content="width=device-width, initial-scale=1.0"> <a href="" target="_blank">Open me in a separate tab</a> <pre id="log"></pre> <script> // This shouldn't be needed, but otherwise Fenix doesn't show the tooltip on // longpress... document.querySelector("a").href = location.href; function logSize() { log.innerText += window.innerWidth + "x" + window.innerHeight + "\n"; } logSize(); onresize = logSize; </script> (Hosted at https://crisal.io/tmp/gecko-mobile-resize.html for convenience) Right now on trunk, when you click the link from GVE or Fenix, we're only getting an initial size of 0x0 (which is not great, btw), and only after first paint we get the real device size, but content doesn't get a resize event. This is obviously wrong, every time the layout viewport changes we should fire resize events. Pages that get opened in new tabs and get refreshed when resized may get an extra reload with this approach, but this seems not avoidable unless widget sets the viewport size right in advance (which from discussion with snorp and agi doesn't seem possible in the general case). What used to happen is that we were triggering a redundant resize reflow from the initial paint which didn't update the layout viewport (because the content viewer and co had the right viewport from the previous navigation). Now that we optimize those away, I think our behavior should be correct. Differential Revision: https://phabricator.services.mozilla.com/D46956
gfx/layers/apz/test/gtest/mvm/TestMobileViewportManager.cpp
layout/base/GeckoMVMContext.cpp
layout/base/GeckoMVMContext.h
layout/base/MVMContext.h
layout/base/MobileViewportManager.cpp
layout/base/PresShell.cpp
layout/base/PresShellForwards.h
--- a/gfx/layers/apz/test/gtest/mvm/TestMobileViewportManager.cpp
+++ b/gfx/layers/apz/test/gtest/mvm/TestMobileViewportManager.cpp
@@ -77,17 +77,17 @@ class MockMVMContext : public MVMContext
   bool IsInReaderMode() const { return false; }
   bool IsDocumentLoading() const { return false; }
 
   void SetResolutionAndScaleTo(float aResolution,
                                ResolutionChangeOrigin aOrigin) {
     mResolution = aResolution;
     mMVM->ResolutionUpdated(aOrigin);
   }
-  void Reflow(const CSSSize& aNewSize, ResizeEventFlag) {
+  void Reflow(const CSSSize& aNewSize) {
     mICBSize = aNewSize;
     mContentSize = mLayoutFunction(mICBSize);
   }
 
   // Allow test code to modify the input metrics.
   void SetMinScale(CSSToScreenScale aMinScale) { mMinScale = aMinScale; }
   void SetMaxScale(CSSToScreenScale aMaxScale) { mMaxScale = aMaxScale; }
   void SetInitialScale(CSSToScreenScale aInitialScale) {
--- a/layout/base/GeckoMVMContext.cpp
+++ b/layout/base/GeckoMVMContext.cpp
@@ -171,25 +171,19 @@ void GeckoMVMContext::UpdateDisplayPortM
     nsLayoutUtils::SetDisplayPortBaseIfNotSet(root->GetContent(),
                                               displayportBase);
     nsIScrollableFrame* scrollable = do_QueryFrame(root);
     nsLayoutUtils::CalculateAndSetDisplayPortMargins(
         scrollable, nsLayoutUtils::RepaintMode::Repaint);
   }
 }
 
-void GeckoMVMContext::Reflow(const CSSSize& aNewSize,
-                             ResizeEventFlag aResizeEventFlag) {
+void GeckoMVMContext::Reflow(const CSSSize& aNewSize) {
   MOZ_ASSERT(mPresShell);
 
-  ResizeReflowOptions reflowOptions = ResizeReflowOptions::NoOption;
-  if (aResizeEventFlag == ResizeEventFlag::Suppress) {
-    reflowOptions |= ResizeReflowOptions::SuppressResizeEvent;
-  }
-
   RefPtr<PresShell> presShell = mPresShell;
   presShell->ResizeReflowIgnoreOverride(
-      nsPresContext::CSSPixelsToAppUnits(aNewSize.width),
-      nsPresContext::CSSPixelsToAppUnits(aNewSize.height),
-      reflowOptions);
+      CSSPixel::ToAppUnits(aNewSize.width),
+      CSSPixel::ToAppUnits(aNewSize.height),
+      ResizeReflowOptions::NoOption);
 }
 
 }  // namespace mozilla
--- a/layout/base/GeckoMVMContext.h
+++ b/layout/base/GeckoMVMContext.h
@@ -49,18 +49,17 @@ class GeckoMVMContext : public MVMContex
   bool AllowZoomingForDocument() const override;
   bool IsInReaderMode() const override;
   bool IsDocumentLoading() const override;
 
   void SetResolutionAndScaleTo(float aResolution,
                                ResolutionChangeOrigin aOrigin) override;
   void SetVisualViewportSize(const CSSSize& aSize) override;
   void UpdateDisplayPortMargins() override;
-  MOZ_CAN_RUN_SCRIPT_BOUNDARY
-  void Reflow(const CSSSize& aNewSize, ResizeEventFlag) override;
+  MOZ_CAN_RUN_SCRIPT_BOUNDARY void Reflow(const CSSSize& aNewSize) override;
 
  private:
   RefPtr<dom::Document> mDocument;
   // raw ref since the presShell owns this
   PresShell* MOZ_NON_OWNING_REF mPresShell;
   nsCOMPtr<dom::EventTarget> mEventTarget;
 };
 
--- a/layout/base/MVMContext.h
+++ b/layout/base/MVMContext.h
@@ -54,18 +54,14 @@ class MVMContext {
   virtual bool IsInReaderMode() const = 0;
   virtual bool IsDocumentLoading() const = 0;
 
   virtual void SetResolutionAndScaleTo(float aResolution,
                                        ResolutionChangeOrigin aOrigin) = 0;
   virtual void SetVisualViewportSize(const CSSSize& aSize) = 0;
   virtual void UpdateDisplayPortMargins() = 0;
 
-  enum class ResizeEventFlag {
-    IfNecessary,  // resize events will be fired if necessary.
-    Suppress,     // resize events will never be fired.
-  };
-  virtual void Reflow(const CSSSize& aNewSize, ResizeEventFlag) = 0;
+  virtual void Reflow(const CSSSize& aNewSize) = 0;
 };
 
 }  // namespace mozilla
 
 #endif  // MVMContext_h_
--- a/layout/base/MobileViewportManager.cpp
+++ b/layout/base/MobileViewportManager.cpp
@@ -590,19 +590,17 @@ void MobileViewportManager::RefreshViewp
   }
 
   // Update internal state.
   mMobileViewportSize = viewport;
 
   RefPtr<MobileViewportManager> strongThis(this);
 
   // Kick off a reflow.
-  mContext->Reflow(viewport,
-                   mIsFirstPaint ? MVMContext::ResizeEventFlag::Suppress
-                                 : MVMContext::ResizeEventFlag::IfNecessary);
+  mContext->Reflow(viewport);
 
   // We are going to fit the content to the display width if the initial-scale
   // is not specied and if the content is still wider than the display width.
   ShrinkToDisplaySizeIfNeeded(viewportInfo, displaySize);
 
   mIsFirstPaint = false;
 }
 
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -1894,19 +1894,18 @@ nsresult PresShell::ResizeReflowIgnoreOv
     return NS_OK;
   }
 
   // Historically we never fired resize events if there was no root frame by the
   // time this function got called.
   const bool initialized = mDidInitialize;
   RefPtr<PresShell> kungFuDeathGrip(this);
 
-  auto postResizeEventIfNeeded = [this, initialized, aOptions]() {
-    if (initialized && !mIsDestroying && !mResizeEventPending &&
-        !(aOptions & ResizeReflowOptions::SuppressResizeEvent)) {
+  auto postResizeEventIfNeeded = [this, initialized]() {
+    if (initialized && !mIsDestroying && !mResizeEventPending) {
       mResizeEventPending = true;
       if (MOZ_LIKELY(!mDocument->GetBFCacheEntry())) {
         mPresContext->RefreshDriver()->AddResizeEventFlushObserver(this);
       }
     }
   };
 
   if (!(aOptions & ResizeReflowOptions::BSizeLimit)) {
--- a/layout/base/PresShellForwards.h
+++ b/layout/base/PresShellForwards.h
@@ -31,27 +31,22 @@ enum class CaptureFlags {
 
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(CaptureFlags)
 
 enum class ResizeReflowOptions : uint32_t {
   NoOption = 0,
   // the resulting BSize can be less than the given one, producing
   // shrink-to-fit sizing in the block dimension
   BSizeLimit = 1 << 0,
-  // suppress resize events even if the content size is changed due to the
-  // reflow.  This flag is used for mobile since on mobile we need to do an
-  // additional reflow to zoom the content by the initial-scale or auto scaling
-  // and we don't want any resize events during the initial paint.
-  SuppressResizeEvent = 1 << 1,
   // Invalidate layout, but don't reflow.
   //
   // TODO(emilio): Ideally this should just become the default, or we should
   // unconditionally not reflow and rely on the caller to do so, having a
   // separate API for shrink-to-fit.
-  SuppressReflow = 1 << 2,
+  SuppressReflow = 1 << 1,
 };
 
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(ResizeReflowOptions)
 
 // This is actually pref-controlled, but we use this value if we fail to get
 // the pref for any reason.
 #define PAINTLOCK_EVENT_DELAY 5