Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow(). r?florian draft
authorKris Maglione <maglione.k@gmail.com>
Wed, 30 Aug 2017 11:34:20 -0700
changeset 656129 1a587441fb6591bbf139ab3dd0256e11ad41a205
parent 656128 8f7e7e77a775c4f3a27ede3df5c1f07fc82e98cf
child 656130 05083b460030d284e510d7d586eb4c3509eda21a
push id77076
push usermaglione.k@gmail.com
push dateWed, 30 Aug 2017 19:02:00 +0000
reviewersflorian
bugs1356532
milestone57.0a1
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow(). r?florian MozReview-Commit-ID: LBbElXZOEnf
browser/base/content/test/performance/browser_urlbar_search_reflows.js
browser/base/content/test/performance/head.js
toolkit/content/widgets/autocomplete.xml
toolkit/modules/BrowserUtils.jsm
--- a/browser/base/content/test/performance/browser_urlbar_search_reflows.js
+++ b/browser/base/content/test/performance/browser_urlbar_search_reflows.js
@@ -30,49 +30,20 @@ const EXPECTED_REFLOWS_FIRST_OPEN = [
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml"
     ],
     times: 1, // This number should only ever go down - never up.
   },
 
   {
     stack: [
-      "adjustHeight@chrome://global/content/bindings/autocomplete.xml",
-      "onxblpopupshown@chrome://global/content/bindings/autocomplete.xml"
-    ],
-    times: 5, // This number should only ever go down - never up.
-  },
-
-  {
-    stack: [
-      "_handleOverflow@chrome://global/content/bindings/autocomplete.xml",
-      "handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml",
-      "set_siteIconStart@chrome://global/content/bindings/autocomplete.xml",
-    ],
-    times: 6, // This number should only ever go down - never up.
-  },
-
-  {
-    stack: [
-      "adjustHeight@chrome://global/content/bindings/autocomplete.xml",
-      "_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml",
+      "forceReflow@resource://gre/modules/BrowserUtils.jsm",
     ],
     times: 3, // This number should only ever go down - never up.
-  },
-
-  {
-    stack: [
-      "_handleOverflow@chrome://global/content/bindings/autocomplete.xml",
-      "handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml",
-      "_reuseAcItem@chrome://global/content/bindings/autocomplete.xml",
-      "_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml",
-      "_invalidate@chrome://global/content/bindings/autocomplete.xml",
-      "invalidate@chrome://global/content/bindings/autocomplete.xml"
-    ],
-    times: 390, // This number should only ever go down - never up.
+    minTimes: 2,
   },
 
   {
     stack: [
       "_openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
@@ -89,44 +60,16 @@ const EXPECTED_REFLOWS_FIRST_OPEN = [
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
     ],
   },
 ];
 
 /* These reflows happen everytime the awesomebar panel opens. */
 const EXPECTED_REFLOWS_SECOND_OPEN = [
-  {
-    stack: [
-      "adjustHeight@chrome://global/content/bindings/autocomplete.xml",
-      "onxblpopupshown@chrome://global/content/bindings/autocomplete.xml"
-    ],
-    times: 3, // This number should only ever go down - never up.
-  },
-
-  {
-    stack: [
-      "adjustHeight@chrome://global/content/bindings/autocomplete.xml",
-      "_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml",
-    ],
-    times: 3, // This number should only ever go down - never up.
-  },
-
-  {
-    stack: [
-      "_handleOverflow@chrome://global/content/bindings/autocomplete.xml",
-      "handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml",
-      "_reuseAcItem@chrome://global/content/bindings/autocomplete.xml",
-      "_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml",
-      "_invalidate@chrome://global/content/bindings/autocomplete.xml",
-      "invalidate@chrome://global/content/bindings/autocomplete.xml"
-    ],
-    times: 444, // This number should only ever go down - never up.
-  },
-
   // Bug 1384256
   {
     stack: [
       "_openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
     ],
@@ -138,16 +81,24 @@ const EXPECTED_REFLOWS_SECOND_OPEN = [
     stack: [
       "openPopup@chrome://global/content/bindings/popup.xml",
       "_openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openAutocompletePopup@chrome://browser/content/urlbarBindings.xml",
       "openPopup@chrome://global/content/bindings/autocomplete.xml",
       "set_popupOpen@chrome://global/content/bindings/autocomplete.xml",
     ],
   },
+
+  {
+    stack: [
+      "forceReflow@resource://gre/modules/BrowserUtils.jsm",
+    ],
+    times: 3, // This number should only ever go down - never up.
+    minTimes: 1,
+  },
 ];
 
 const SEARCH_TERM = "urlbar-reflows";
 
 add_task(async function setup() {
   await addDummyHistoryEntries();
 });
 
--- a/browser/base/content/test/performance/head.js
+++ b/browser/base/content/test/performance/head.js
@@ -1,13 +1,22 @@
 "use strict";
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
   "resource://testing-common/PlacesTestUtils.jsm");
 
+const {ReflowScheduler} = Cu.import("resource://gre/modules/BrowserUtils.jsm", {});
+
+// Force a reflow that doesn't immediately dirty the layout state, so
+// that promiseLayoutFlushed callbacks can query layout state with
+// impunity.
+ReflowScheduler.prototype.maybeForceReflow = function maybeForceReflow() {
+  this.forceReflow();
+};
+
 /**
  * Async utility function for ensuring that no unexpected uninterruptible
  * reflows occur during some period of time in a window.
  *
  * @param testFn (async function)
  *        The async function that will exercise the browser activity that is
  *        being tested for reflows.
  *
@@ -49,16 +58,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
  *        Note that line numbers are not included in the stacks.
  *
  *        Order of the reflows doesn't matter. Expected reflows that aren't seen
  *        will cause an assertion failure. When this argument is not passed,
  *        it defaults to the empty Array, meaning no reflows are expected.
  * @param window (browser window, optional)
  *        The browser window to monitor. Defaults to the current window.
  */
+let times = {};
 async function withReflowObserver(testFn, expectedReflows = [], win = window) {
   let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor)
                .getInterface(Ci.nsIDOMWindowUtils);
   let dirtyFrameFn = () => {
     try {
       dwu.ensureDirtyRootFrame();
     } catch (e) {
       // If this fails, we should probably make note of it, but it's not fatal.
@@ -83,17 +93,22 @@ async function withReflowObserver(testFn
       // Gather information about the current code path, slicing out the current
       // frame.
       let path = (new Error().stack).split("\n").slice(1).map(line => {
         return line.replace(/:\d+:\d+$/, "");
       }).join("|");
 
       let pathWithLineNumbers = (new Error().stack).split("\n").slice(1);
 
-      // Just in case, dirty the frame now that we've reflowed.
+      // Reflows triggered by promiseLayoutFlushed callbacks get a free pass.
+      if (path.startsWith("forceReflow@resource://gre/modules/BrowserUtils.jsm|" +
+                          "maybeForceReflow@chrome://mochitests/content/browser/browser/base/content/test/performance/head.js")) {
+        return;
+      }
+
       dirtyFrameFn();
 
       // Stack trace is empty. Reflow was triggered by native code, which
       // we ignore.
       if (path === "") {
         return;
       }
 
@@ -103,16 +118,17 @@ async function withReflowObserver(testFn
         return;
       }
 
       let index = expectedReflows.findIndex(reflow => path.startsWith(reflow.stack.join("|")));
 
       if (index != -1) {
         Assert.ok(true, "expected uninterruptible reflow: '" +
                   JSON.stringify(pathWithLineNumbers, null, "\t") + "'");
+        --expectedReflows[index].minTimes;
         if (--expectedReflows[index].times == 0) {
           expectedReflows.splice(index, 1);
         }
       } else {
         Assert.ok(false, "unexpected uninterruptible reflow \n" +
                          JSON.stringify(pathWithLineNumbers, null, "\t") + "\n");
       }
     },
@@ -134,19 +150,20 @@ async function withReflowObserver(testFn
 
   els.addListenerForAllEvents(win, dirtyFrameFn, true);
 
   try {
     dirtyFrameFn();
     await testFn(dirtyFrameFn);
   } finally {
     for (let remainder of expectedReflows) {
-      Assert.ok(false,
+      Assert.ok(remainder.minTimes <= 0,
                 `Unused expected reflow: ${JSON.stringify(remainder.stack, null, "\t")}\n` +
-                `This reflow was supposed to be hit ${remainder.times} more time(s).\n` +
+                `This reflow was supposed to be hit ${remainder.minTimes || remainder.times} ` +
+                `more time(s).\n` +
                 "This is probably a good thing - just remove it from the " +
                 "expected list.");
     }
 
     els.removeListenerForAllEvents(win, dirtyFrameFn, true);
     docShell.removeWeakReflowObserver(observer);
   }
 }
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -1431,20 +1431,17 @@ extends="chrome://global/content/binding
            items' site icons (favicons). -->
       <property name="siteIconStart"
                 onget="return this._siteIconStart;">
         <setter>
           <![CDATA[
           if (val != this._siteIconStart) {
             this._siteIconStart = val;
             for (let item of this.richlistbox.childNodes) {
-              let changed = item.adjustSiteIconStart(val);
-              if (changed) {
-                item.handleOverUnderflow();
-              }
+              item.adjustSiteIconStart(val);
             }
           }
           return val;
           ]]>
         </setter>
       </property>
 
       <property name="overflowPadding"
@@ -2079,21 +2076,17 @@ extends="chrome://global/content/binding
           ]]>
         </body>
       </method>
 
       <method name="_onChanged">
         <body>
           <![CDATA[
             let popup = this.parentNode.parentNode;
-            let iconChanged = this.adjustSiteIconStart(popup._siteIconStart);
-
-            if (iconChanged) {
-              this.handleOverUnderflow();
-            }
+            this.adjustSiteIconStart(popup._siteIconStart);
           ]]>
         </body>
       </method>
 
 
       <method name="_reuseAcItem">
         <body>
           <![CDATA[
@@ -2107,19 +2100,16 @@ extends="chrome://global/content/binding
                 action.type != "searchengine" ||
                 !popup.overrideSearchEngineName ||
                 action.params.engineName == popup.overrideSearchEngineName) {
 
               this.collapsed = false;
               // Call adjustSiteIconStart only after setting collapsed=
               // false.  The calculations it does may be wrong otherwise.
               this.adjustSiteIconStart(popup._siteIconStart);
-              // The popup may have changed size between now and the last
-              // time the item was shown, so always handle over/underflow.
-              this.handleOverUnderflow();
 
               return true;
             }
 
             return false;
           ]]>
         </body>
       </method>
@@ -2374,108 +2364,124 @@ extends="chrome://global/content/binding
            @param newStart The new x-coordinate, relative to the leading edge of
                   the window.  Pass undefined to reset the icon's position to
                   whatever is specified in CSS.
            @return True if the icon's position changed, false if not. -->
       <method name="adjustSiteIconStart">
         <parameter name="newStart"/>
         <body>
           <![CDATA[
-          if (typeof(newStart) != "number") {
-            this._typeIconSpacer.style.removeProperty("width");
-            return true;
-          }
-
-          let utils = window.QueryInterface(Ci.nsIInterfaceRequestor)
-                            .getInterface(Ci.nsIDOMWindowUtils);
-          let rect = utils.getBoundsWithoutFlushing(this._siteIcon);
-
-          let dir = this.getAttribute("dir");
-          let delta = dir == "rtl" ? rect.right - Math.round(newStart)
-                                   : Math.round(newStart) - rect.left;
-          if (delta) {
-            let currentSpacerWidth = this._typeIconSpacer.style.width || "0px";
-            this._typeIconSpacer.style.width =
-              (parseInt(currentSpacerWidth, 10) + delta) + "px";
-            return true;
-          }
-
-          return false;
+          this._siteIconStart = Math.round(newStart);
+          this.handleOverUnderflow();
           ]]>
         </body>
       </method>
 
       <!-- This method truncates the displayed strings as necessary. -->
       <method name="_handleOverflow">
         <body><![CDATA[
-          let itemRect = this.parentNode.getBoundingClientRect();
-          let titleRect = this._titleText.getBoundingClientRect();
-          let tagsRect = this._tagsText.getBoundingClientRect();
-          let separatorRect = this._separator.getBoundingClientRect();
-          let urlRect = this._urlText.getBoundingClientRect();
-          let actionRect = this._actionText.getBoundingClientRect();
-          let separatorURLActionWidth =
-            separatorRect.width + Math.max(urlRect.width, actionRect.width);
-
-          // Total width for the title and URL/action is the width of the item
-          // minus the start of the title text minus a little optional extra padding.
-          // This extra padding amount is basically arbitrary but keeps the text
-          // from getting too close to the popup's edge.
-          let dir = this.getAttribute("dir");
-          let titleStart = dir == "rtl" ? itemRect.right - titleRect.right
-                                        : titleRect.left - itemRect.left;
-
-          let popup = this.parentNode.parentNode;
-          let itemWidth = itemRect.width - titleStart - popup.overflowPadding;
-
-          if (this._tags.hasAttribute("empty")) {
-            // The tags box is not displayed in this case.
-            tagsRect.width = 0;
+          if (this._overflowPromise) {
+            return;
           }
 
-          let titleTagsWidth = titleRect.width + tagsRect.width;
-          if (titleTagsWidth + separatorURLActionWidth > itemWidth) {
-            // Title + tags + URL/action overflows the item width.
+          this._overflowPromise = (async () => {
+            if (typeof this._siteIconStart != "number") {
+              this.typeIconSpacer.style.removeProperty("width");
+            }
 
-            // The percentage of the item width allocated to the title and tags.
-            let titleTagsPct = 0.66;
+            let [itemRect, iconSpacerRect, siteIconRect, titleRect, tagsRect, separatorRect, urlRect, actionRect] =
+              await BrowserUtils.promiseSlowLayoutFlush(document, "layout", () => [
+                this.parentNode.getBoundingClientRect(),
+                this._typeIconSpacer.getBoundingClientRect(),
+                this._siteIcon.getBoundingClientRect(),
+                this._titleText.getBoundingClientRect(),
+                this._tagsText.getBoundingClientRect(),
+                this._separator.getBoundingClientRect(),
+                this._urlText.getBoundingClientRect(),
+                this._actionText.getBoundingClientRect(),
+              ]);
+
+            this._overflowPromise = null;
 
-            let titleTagsAvailable = itemWidth - separatorURLActionWidth;
-            let titleTagsMaxWidth = Math.max(
-              titleTagsAvailable,
-              itemWidth * titleTagsPct
-            );
-            if (titleTagsWidth > titleTagsMaxWidth) {
-              // Title + tags overflows the max title + tags width.
+            let dir = this.getAttribute("dir");
+            let typeIconDelta = 0;
+            if (typeof this._siteIconStart == "number") {
+              typeIconDelta = dir == "rtl" ? siteIconRect.right - this._siteIconStart
+                                           : this._siteIconStart - siteIconRect.left;
 
-              // The percentage of the title + tags width allocated to the
-              // title.
-              let titlePct = 0.33;
+              if (typeIconDelta != 0) {
+                let newWidth = typeIconDelta + iconSpacerRect.width;
+                BrowserUtils.promiseAnimationFrame(window, () => {
+                  this._typeIconSpacer.style.width = `${newWidth}px`;
+                });
+              }
+            }
+
+            let separatorURLActionWidth =
+              separatorRect.width + Math.max(urlRect.width, actionRect.width) + typeIconDelta;
+
+            // Total width for the title and URL/action is the width of the item
+            // minus the start of the title text minus a little optional extra padding.
+            // This extra padding amount is basically arbitrary but keeps the text
+            // from getting too close to the popup's edge.
+            let titleStart = dir == "rtl" ? itemRect.right - titleRect.right
+                                          : titleRect.left - itemRect.left;
+
+            let popup = this.parentNode.parentNode;
+            let itemWidth = itemRect.width - titleStart - popup.overflowPadding;
 
-              let titleAvailable = titleTagsMaxWidth - tagsRect.width;
-              let titleMaxWidth = Math.max(
-                titleAvailable,
-                titleTagsMaxWidth * titlePct
-              );
-              let tagsAvailable = titleTagsMaxWidth - titleRect.width;
-              let tagsMaxWidth = Math.max(
-                tagsAvailable,
-                titleTagsMaxWidth * (1 - titlePct)
+            if (this._tags.hasAttribute("empty")) {
+              // The tags box is not displayed in this case.
+              tagsRect.width = 0;
+            }
+
+            let titleTagsWidth = titleRect.width + tagsRect.width;
+            if (titleTagsWidth + separatorURLActionWidth > itemWidth) {
+              // Title + tags + URL/action overflows the item width.
+
+              // The percentage of the item width allocated to the title and tags.
+              let titleTagsPct = 0.66;
+
+              let titleTagsAvailable = itemWidth - separatorURLActionWidth;
+              let titleTagsMaxWidth = Math.max(
+                titleTagsAvailable,
+                itemWidth * titleTagsPct
               );
-              this._titleText.style.maxWidth = titleMaxWidth + "px";
-              this._tagsText.style.maxWidth = tagsMaxWidth + "px";
+
+              await BrowserUtils.promiseAnimationFrame(window, () => {
+                if (titleTagsWidth > titleTagsMaxWidth) {
+                  // Title + tags overflows the max title + tags width.
+
+                  // The percentage of the title + tags width allocated to the
+                  // title.
+                  let titlePct = 0.33;
+
+                  let titleAvailable = titleTagsMaxWidth - tagsRect.width;
+                  let titleMaxWidth = Math.max(
+                    titleAvailable,
+                    titleTagsMaxWidth * titlePct
+                  );
+                  let tagsAvailable = titleTagsMaxWidth - titleRect.width;
+                  let tagsMaxWidth = Math.max(
+                    tagsAvailable,
+                    titleTagsMaxWidth * (1 - titlePct)
+                  );
+                  this._titleText.style.maxWidth = titleMaxWidth + "px";
+                  this._tagsText.style.maxWidth = tagsMaxWidth + "px";
+                }
+                let urlActionMaxWidth = Math.max(
+                  itemWidth - titleTagsWidth,
+                  itemWidth * (1 - titleTagsPct)
+                );
+                urlActionMaxWidth -= separatorRect.width;
+                this._urlText.style.maxWidth = urlActionMaxWidth + "px";
+                this._actionText.style.maxWidth = urlActionMaxWidth + "px";
+              });
             }
-            let urlActionMaxWidth = Math.max(
-              itemWidth - titleTagsWidth,
-              itemWidth * (1 - titleTagsPct)
-            );
-            urlActionMaxWidth -= separatorRect.width;
-            this._urlText.style.maxWidth = urlActionMaxWidth + "px";
-            this._actionText.style.maxWidth = urlActionMaxWidth + "px";
-          }
+          })();
         ]]></body>
       </method>
 
       <method name="handleOverUnderflow">
         <body>
           <![CDATA[
           this._removeMaxWidths();
           this._handleOverflow();
--- a/toolkit/modules/BrowserUtils.jsm
+++ b/toolkit/modules/BrowserUtils.jsm
@@ -42,25 +42,31 @@ ReflowScheduler.get = function(doc) {
     reflowSchedulers.set(doc, scheduler);
   }
   return scheduler;
 }
 
 ReflowScheduler.prototype = {
   QueryInterface: XPCOMUtils.generateQI(["nsIReflowObserver", "nsISupportsWeakReference"]),
 
+  // This is overridden by mochitests to allow us skip dirtying the
+  // layout state for layoutFlushed callbacks.
+  maybeForceReflow() {},
+
   _onReflow() {
     this._doc.docShell.removeWeakReflowObserver(this);
     this._reflowRequested = false;
 
     if (this._idleRequest) {
       this._win.cancelIdleCallback(this._idleRequest);
       this._idleRequest = null;
     }
 
+    this.maybeForceReflow();
+
     for (let callback of this.reflowCallbacks.slice()) {
       try {
         callback();
       } catch (e) {
         Cu.reportError(e);
       }
     }
   },