Bug 1439875: Size the XUL window before doing layout. r=smaug draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 21 Feb 2018 12:47:05 +0100
changeset 766846 ce214004308cd976036b8120123464d5151b9e75
parent 766845 b3d94aa6bbd8e0e784adb05966df183fd4e7837e
child 766847 21de93e48d94ae56a333e0ab36effcbd6bdf94e3
push id102414
push userbmo:emilio@crisal.io
push dateTue, 13 Mar 2018 13:33:32 +0000
reviewerssmaug
bugs1439875, 345560
milestone61.0a1
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();