Bug 1439875: Size the XUL window before doing layout. r=smaug
☠☠ backed out by be470e9c8c14 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 21 Feb 2018 12:47:05 +0100
changeset 408222 7b33ad14ce8c8174dee30ce014b8757f377b7fca
parent 408183 58eb13b394f4329a93eaa0f78f06fbafc92cd079
child 408223 9ded550729036d82f6998093bcc4150163b863f6
push id33630
push usershindli@mozilla.com
push dateThu, 15 Mar 2018 10:14:59 +0000
treeherdermozilla-central@fcb11e93adf5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1439875, 345560
milestone61.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 1439875: Size the XUL window before doing layout. r=smaug The only subtle thing is the mCenterAfterLoad stuff, which is gated after a mChromeLoaded. Other than that it follows the same pattern as bug 345560. MozReview-Commit-ID: 8qDiA2yn9DB
dom/xul/XULDocument.cpp
xpfe/appshell/nsIXULWindow.idl
xpfe/appshell/nsXULWindow.cpp
xpfe/appshell/nsXULWindow.h
--- a/dom/xul/XULDocument.cpp
+++ b/dom/xul/XULDocument.cpp
@@ -2658,29 +2658,27 @@ XULDocument::DoneWalking()
         // mInitialLayoutComplete will be false will follow the else branch
         // there too.  See the big comment there for how such reentry can
         // happen.
         mDocumentLoaded = true;
 
         NotifyPossibleTitleChange(false);
 
         // Before starting layout, check whether we're a toplevel chrome
-        // window.  If we are, set our chrome flags now, so that we don't have
-        // to restyle the whole frame tree after StartLayout.
-        nsCOMPtr<nsIDocShellTreeItem> item = GetDocShell();
-        if (item) {
+        // window.  If we are, setup some state so that we don't have to restyle
+        // the whole tree after StartLayout.
+        if (nsCOMPtr<nsIDocShellTreeItem> item = GetDocShell()) {
             nsCOMPtr<nsIDocShellTreeOwner> owner;
             item->GetTreeOwner(getter_AddRefs(owner));
-            nsCOMPtr<nsIXULWindow> xulWin = do_GetInterface(owner);
-            if (xulWin) {
+            if (nsCOMPtr<nsIXULWindow> xulWin = do_GetInterface(owner)) {
                 nsCOMPtr<nsIDocShell> xulWinShell;
                 xulWin->GetDocShell(getter_AddRefs(xulWinShell));
                 if (SameCOMIdentity(xulWinShell, item)) {
-                    // We're the chrome document!  Apply our chrome flags now.
-                    xulWin->ApplyChromeFlags();
+                    // We're the chrome document!
+                    xulWin->BeforeStartLayout();
                 }
             }
         }
 
         nsContentUtils::DispatchTrustedEvent(this,
                 static_cast<nsIDocument*>(this),
                 NS_LITERAL_STRING("MozBeforeInitialXULLayout"),
                 true,
--- a/xpfe/appshell/nsIXULWindow.idl
+++ b/xpfe/appshell/nsIXULWindow.idl
@@ -123,22 +123,24 @@ interface nsIXULWindow : nsISupports
   nsIXULWindow createNewWindow(in int32_t aChromeFlags,
                                in nsITabParent aOpeningTab,
                                in mozIDOMWindowProxy aOpener,
                                in unsigned long long aNextTabParentId);
 
   attribute nsIXULBrowserWindow XULBrowserWindow;
 
   /**
-   * Back-door method to force application of chrome flags at a particular
-   * time.  Do NOT call this unless you know what you're doing!  In particular,
+   * Back-door method to make sure some stuff is done when the document is
+   * ready for layout, that would cause expensive computation otherwise later.
+   *
+   * Do NOT call this unless you know what you're doing!  In particular,
    * calling this when this XUL window doesn't yet have a document in its
    * docshell could cause problems.
    */
-  [noscript] void applyChromeFlags();
+  [noscript] void beforeStartLayout();
 
   /**
    * Given the dimensions of some content area held within this
    * XUL window, and assuming that that content area will change
    * its dimensions in linear proportion to the dimensions of this
    * XUL window, changes the size of the XUL window so that the
    * content area reaches a particular size.
    *
--- a/xpfe/appshell/nsXULWindow.cpp
+++ b/xpfe/appshell/nsXULWindow.cpp
@@ -269,18 +269,19 @@ NS_IMETHODIMP nsXULWindow::GetChromeFlag
 }
 
 NS_IMETHODIMP nsXULWindow::SetChromeFlags(uint32_t aChromeFlags)
 {
   NS_ASSERTION(!mChromeFlagsFrozen,
                "SetChromeFlags() after AssumeChromeFlagsAreFrozen()!");
 
   mChromeFlags = aChromeFlags;
-  if (mChromeLoaded)
-    NS_ENSURE_SUCCESS(ApplyChromeFlags(), NS_ERROR_FAILURE);
+  if (mChromeLoaded) {
+    ApplyChromeFlags();
+  }
   return NS_OK;
 }
 
 NS_IMETHODIMP nsXULWindow::AssumeChromeFlagsAreFrozen()
 {
   mChromeFlagsFrozen = true;
   return NS_OK;
 }
@@ -1078,92 +1079,17 @@ GetWindowOuterInnerDiff(nsIWidget* aWind
 void nsXULWindow::OnChromeLoaded()
 {
   nsresult rv = EnsureContentTreeOwner();
 
   if (NS_SUCCEEDED(rv)) {
     mChromeLoaded = true;
     ApplyChromeFlags();
     SyncAttributesToWidget();
-
-    int32_t specWidth = -1, specHeight = -1;
-    bool gotSize = false;
-    bool isContent = false;
-
-    GetHasPrimaryContent(&isContent);
-
-    CSSIntSize windowDiff = mWindow
-      ? RoundedToInt(GetWindowOuterInnerDiff(mWindow) /
-                     mWindow->GetDefaultScale())
-      : CSSIntSize();
-
-    // If this window has a primary content and fingerprinting resistance is
-    // enabled, we enforce this window to rounded dimensions.
-    if (isContent && nsContentUtils::ShouldResistFingerprinting()) {
-      ForceRoundedDimensions();
-    } else if (!mIgnoreXULSize) {
-      gotSize = LoadSizeFromXUL(specWidth, specHeight);
-      specWidth += windowDiff.width;
-      specHeight += windowDiff.height;
-    }
-
-    bool positionSet = !mIgnoreXULPosition;
-    nsCOMPtr<nsIXULWindow> parentWindow(do_QueryReferent(mParentWindow));
-#if defined(XP_UNIX) && !defined(XP_MACOSX)
-    // don't override WM placement on unix for independent, top-level windows
-    // (however, we think the benefits of intelligent dependent window placement
-    // trump that override.)
-    if (!parentWindow)
-      positionSet = false;
-#endif
-    if (positionSet) {
-      // We have to do this before sizing the window, because sizing depends
-      // on the resolution of the screen we're on. But positioning needs to
-      // know the size so that it can constrain to screen bounds.... as an
-      // initial guess here, we'll use the specified size (if any).
-      positionSet = LoadPositionFromXUL(specWidth, specHeight);
-    }
-
-    if (gotSize) {
-      SetSpecifiedSize(specWidth, specHeight);
-    }
-
-    if (mIntrinsicallySized) {
-      // (if LoadSizeFromXUL set the size, mIntrinsicallySized will be false)
-      nsCOMPtr<nsIContentViewer> cv;
-      mDocShell->GetContentViewer(getter_AddRefs(cv));
-      if (cv) {
-        nsCOMPtr<nsIDocShellTreeItem> docShellAsItem = do_QueryInterface(mDocShell);
-        nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
-        docShellAsItem->GetTreeOwner(getter_AddRefs(treeOwner));
-        if (treeOwner) {
-          // GetContentSize can fail, so initialise |width| and |height| to be
-          // on the safe side.
-          int32_t width = 0, height = 0;
-          if (NS_SUCCEEDED(cv->GetContentSize(&width, &height))) {
-            treeOwner->SizeShellTo(docShellAsItem, width, height);
-            // Update specified size for the final LoadPositionFromXUL call.
-            specWidth = width + windowDiff.width;
-            specHeight = height + windowDiff.height;
-          }
-        }
-      }
-    }
-
-    // Now that we have set the window's final size, we can re-do its
-    // positioning so that it is properly constrained to the screen.
-    if (positionSet) {
-      LoadPositionFromXUL(specWidth, specHeight);
-    }
-
-    LoadMiscPersistentAttributesFromXUL();
-
-    if (mCenterAfterLoad && !positionSet) {
-      Center(parentWindow, parentWindow ? false : true, false);
-    }
+    SizeShell();
 
     if (mShowAfterLoad) {
       SetVisibility(true);
       // At this point the window may have been closed during Show(), so
       // nsXULWindow::Destroy may already have been called. Take care!
     }
   }
   mPersistentAttributesMask |= PAD_POSITION | PAD_SIZE | PAD_MISC;
@@ -2257,20 +2183,23 @@ bool nsXULWindow::GetContentScrollbarVis
 }
 
 // during spinup, attributes that haven't been loaded yet can't be dirty
 void nsXULWindow::PersistentAttributesDirty(uint32_t aDirtyFlags)
 {
   mPersistentAttributesDirty |= aDirtyFlags & mPersistentAttributesMask;
 }
 
-NS_IMETHODIMP nsXULWindow::ApplyChromeFlags()
+void
+nsXULWindow::ApplyChromeFlags()
 {
   nsCOMPtr<dom::Element> window = GetWindowDOMElement();
-  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
+  if (!window) {
+    return;
+  }
 
   if (mChromeLoaded) {
     // The two calls in this block don't need to happen early because they
     // don't cause a global restyle on the document.  Not only that, but the
     // scrollbar stuff needs a content area to toggle the scrollbars on anyway.
     // So just don't do these until mChromeLoaded is true.
 
     // Scrollbars have their own special treatment.
@@ -2299,22 +2228,109 @@ NS_IMETHODIMP nsXULWindow::ApplyChromeFl
   if (! (mChromeFlags & nsIWebBrowserChrome::CHROME_STATUSBAR))
     newvalue.AppendLiteral("status ");
 
   if (! (mChromeFlags & nsIWebBrowserChrome::CHROME_EXTRA))
     newvalue.AppendLiteral("extrachrome ");
 
   // Note that if we're not actually changing the value this will be a no-op,
   // so no need to compare to the old value.
-  ErrorResult rv;
+  IgnoredErrorResult rv;
   window->SetAttribute(NS_LITERAL_STRING("chromehidden"), newvalue, rv);
+}
 
+NS_IMETHODIMP
+nsXULWindow::BeforeStartLayout()
+{
+  ApplyChromeFlags();
+  SyncAttributesToWidget();
+  SizeShell();
   return NS_OK;
 }
 
+void
+nsXULWindow::SizeShell()
+{
+  int32_t specWidth = -1, specHeight = -1;
+  bool gotSize = false;
+  bool isContent = false;
+
+  GetHasPrimaryContent(&isContent);
+
+  CSSIntSize windowDiff = mWindow
+    ? RoundedToInt(GetWindowOuterInnerDiff(mWindow) /
+                   mWindow->GetDefaultScale())
+    : CSSIntSize();
+
+  // If this window has a primary content and fingerprinting resistance is
+  // enabled, we enforce this window to rounded dimensions.
+  if (isContent && nsContentUtils::ShouldResistFingerprinting()) {
+    ForceRoundedDimensions();
+  } else if (!mIgnoreXULSize) {
+    gotSize = LoadSizeFromXUL(specWidth, specHeight);
+    specWidth += windowDiff.width;
+    specHeight += windowDiff.height;
+  }
+
+  bool positionSet = !mIgnoreXULPosition;
+  nsCOMPtr<nsIXULWindow> parentWindow(do_QueryReferent(mParentWindow));
+#if defined(XP_UNIX) && !defined(XP_MACOSX)
+  // don't override WM placement on unix for independent, top-level windows
+  // (however, we think the benefits of intelligent dependent window placement
+  // trump that override.)
+  if (!parentWindow)
+    positionSet = false;
+#endif
+  if (positionSet) {
+    // We have to do this before sizing the window, because sizing depends
+    // on the resolution of the screen we're on. But positioning needs to
+    // know the size so that it can constrain to screen bounds.... as an
+    // initial guess here, we'll use the specified size (if any).
+    positionSet = LoadPositionFromXUL(specWidth, specHeight);
+  }
+
+  if (gotSize) {
+    SetSpecifiedSize(specWidth, specHeight);
+  }
+
+  if (mIntrinsicallySized) {
+    // (if LoadSizeFromXUL set the size, mIntrinsicallySized will be false)
+    nsCOMPtr<nsIContentViewer> cv;
+    mDocShell->GetContentViewer(getter_AddRefs(cv));
+    if (cv) {
+      nsCOMPtr<nsIDocShellTreeItem> docShellAsItem = do_QueryInterface(mDocShell);
+      nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
+      docShellAsItem->GetTreeOwner(getter_AddRefs(treeOwner));
+      if (treeOwner) {
+        // GetContentSize can fail, so initialise |width| and |height| to be
+        // on the safe side.
+        int32_t width = 0, height = 0;
+        if (NS_SUCCEEDED(cv->GetContentSize(&width, &height))) {
+          treeOwner->SizeShellTo(docShellAsItem, width, height);
+          // Update specified size for the final LoadPositionFromXUL call.
+          specWidth = width + windowDiff.width;
+          specHeight = height + windowDiff.height;
+        }
+      }
+    }
+  }
+
+  // Now that we have set the window's final size, we can re-do its
+  // positioning so that it is properly constrained to the screen.
+  if (positionSet) {
+    LoadPositionFromXUL(specWidth, specHeight);
+  }
+
+  LoadMiscPersistentAttributesFromXUL();
+
+  if (mChromeLoaded && mCenterAfterLoad && !positionSet) {
+    Center(parentWindow, parentWindow ? false : true, false);
+  }
+}
+
 NS_IMETHODIMP nsXULWindow::GetXULBrowserWindow(nsIXULBrowserWindow * *aXULBrowserWindow)
 {
   NS_IF_ADDREF(*aXULBrowserWindow = mXULBrowserWindow);
   return NS_OK;
 }
 
 NS_IMETHODIMP nsXULWindow::SetXULBrowserWindow(nsIXULBrowserWindow * aXULBrowserWindow)
 {
--- a/xpfe/appshell/nsXULWindow.h
+++ b/xpfe/appshell/nsXULWindow.h
@@ -88,16 +88,18 @@ protected:
    NS_IMETHOD EnsureChromeTreeOwner();
    NS_IMETHOD EnsureContentTreeOwner();
    NS_IMETHOD EnsurePrimaryContentTreeOwner();
    NS_IMETHOD EnsurePrompter();
    NS_IMETHOD EnsureAuthPrompter();
    NS_IMETHOD ForceRoundedDimensions();
    NS_IMETHOD GetAvailScreenSize(int32_t* aAvailWidth, int32_t* aAvailHeight);
 
+   void ApplyChromeFlags();
+   void SizeShell();
    void OnChromeLoaded();
    void StaggerPosition(int32_t &aRequestedX, int32_t &aRequestedY,
                         int32_t aSpecWidth, int32_t aSpecHeight);
    bool       LoadPositionFromXUL(int32_t aSpecWidth, int32_t aSpecHeight);
    bool       LoadSizeFromXUL(int32_t& aSpecWidth, int32_t& aSpecHeight);
    void       SetSpecifiedSize(int32_t aSpecWidth, int32_t aSpecHeight);
    bool       LoadMiscPersistentAttributesFromXUL();
    void       SyncAttributesToWidget();