Bug 991210 - [new tab page] Tiles are sometimes arranged all in a single line (wrapping as appropriate, e.g. to two lines with 5 items and then 4 items), instead of 3x3 grid [r=adw]
authorEd Lee <edilee@mozilla.com>
Tue, 15 Apr 2014 12:14:09 -0700
changeset 197265 cd86975315178a1d0b473c6e7c9289e55afcf9ea
parent 197264 578a88956f7d4816b8278bd3a9f33481ffe47cd7
child 197266 57f2b31da569d54177446191d94bd71a9d5c0629
push id3624
push userasasaki@mozilla.com
push dateMon, 09 Jun 2014 21:49:01 +0000
treeherdermozilla-beta@b1a5da15899a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs991210
milestone31.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 991210 - [new tab page] Tiles are sometimes arranged all in a single line (wrapping as appropriate, e.g. to two lines with 5 items and then 4 items), instead of 3x3 grid [r=adw] Always Grid_init before "load", defer size calculation to "load", and only wait for links cache for site rendering. Clean up timers triggered from previous tests to avoid unexpected updates.
browser/base/content/newtab/grid.js
browser/base/content/newtab/page.js
browser/base/content/test/newtab/browser.ini
browser/base/content/test/newtab/browser_newtab_bug991210.js
browser/base/content/test/newtab/browser_newtab_update.js
browser/base/content/test/newtab/head.js
toolkit/modules/NewTabUtils.jsm
--- a/browser/base/content/newtab/grid.js
+++ b/browser/base/content/newtab/grid.js
@@ -32,26 +32,30 @@ let gGrid = {
   get cells() this._cells,
 
   /**
    * All sites contained in the grid's cells. Sites may be empty.
    */
   get sites() [cell.site for each (cell in this.cells)],
 
   // Tells whether the grid has already been initialized.
-  get ready() !!this._node,
+  get ready() !!this._ready,
 
   /**
    * Initializes the grid.
    * @param aSelector The query selector of the grid.
    */
   init: function Grid_init() {
     this._node = document.getElementById("newtab-grid");
     this._createSiteFragment();
-    this._render();
+    this._renderGrid();
+    gLinks.populateCache(() => {
+      this._renderSites();
+      this._ready = true;
+    });
     addEventListener("load", this);
     addEventListener("resize", this);
   },
 
   /**
    * Creates a new site in the grid.
    * @param aLink The new site's link.
    * @param aCell The cell that will contain the new site.
@@ -64,24 +68,16 @@ let gGrid = {
   },
 
   /**
    * Handles all grid events.
    */
   handleEvent: function Grid_handleEvent(aEvent) {
     switch (aEvent.type) {
       case "load":
-        // Save the cell's computed height/width including margin and border
-        let refCell = document.querySelector(".newtab-cell");
-        this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop) * 2;
-        this._cellHeight = refCell.offsetHeight + this._cellMargin;
-        this._cellWidth = refCell.offsetWidth + this._cellMargin;
-        this._resizeGrid();
-        break;
-
       case "resize":
         this._resizeGrid();
         break;
     }
   },
 
   /**
    * Refreshes the grid and re-creates all sites.
@@ -92,18 +88,21 @@ let gGrid = {
       let node = cell.node;
       let child = node.firstElementChild;
 
       if (child)
         node.removeChild(child);
     }, this);
 
     // Render the grid again.
-    this._render();
-    this._resizeGrid();
+    if (this._shouldRenderGrid()) {
+      this._renderGrid();
+      this._resizeGrid();
+    }
+    this._renderSites();
   },
 
   /**
    * Locks the grid to block all pointer events.
    */
   lock: function Grid_lock() {
     this.node.setAttribute("locked", "true");
   },
@@ -181,30 +180,27 @@ let gGrid = {
 
     for (let i = 0; i < length; i++) {
       if (links[i])
         this.createSite(links[i], cells[i]);
     }
   },
 
   /**
-   * Renders the grid.
-   */
-  _render: function Grid_render() {
-    if (this._shouldRenderGrid()) {
-      this._renderGrid();
-    }
-
-    this._renderSites();
-  },
-
-  /**
    * Make sure the correct number of rows and columns are visible
    */
   _resizeGrid: function Grid_resizeGrid() {
+    // Save the cell's computed height/width including margin and border
+    if (this._cellMargin === undefined) {
+      let refCell = document.querySelector(".newtab-cell");
+      this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop) * 2;
+      this._cellHeight = refCell.offsetHeight + this._cellMargin;
+      this._cellWidth = refCell.offsetWidth + this._cellMargin;
+    }
+
     let availSpace = document.documentElement.clientHeight - this._cellMargin -
                      document.querySelector("#newtab-margin-top").offsetHeight;
     let visibleRows = Math.floor(availSpace / this._cellHeight);
     this._node.style.height = this._computeHeight() + "px";
     this._node.style.maxHeight = this._computeHeight(visibleRows) + "px";
     this._node.style.maxWidth = gGridPrefs.gridColumns * this._cellWidth +
                                 GRID_WIDTH_EXTRA + "px";
   },
--- a/browser/base/content/newtab/page.js
+++ b/browser/base/content/newtab/page.js
@@ -139,29 +139,27 @@ let gPage = {
         }
       }
     });
     this._mutationObserver.observe(document.documentElement, {
       attributes: true,
       attributeFilter: ["allow-background-captures"],
     });
 
-    gLinks.populateCache(function () {
-      // Initialize and render the grid.
-      gGrid.init();
+    // Initialize and render the grid.
+    gGrid.init();
 
-      // Initialize the drop target shim.
-      gDropTargetShim.init();
+    // Initialize the drop target shim.
+    gDropTargetShim.init();
 
 #ifdef XP_MACOSX
-      // Workaround to prevent a delay on MacOSX due to a slow drop animation.
-      document.addEventListener("dragover", this, false);
-      document.addEventListener("drop", this, false);
+    // Workaround to prevent a delay on MacOSX due to a slow drop animation.
+    document.addEventListener("dragover", this, false);
+    document.addEventListener("drop", this, false);
 #endif
-    }.bind(this));
   },
 
   /**
    * Updates the 'page-disabled' attributes of the respective DOM nodes.
    * @param aValue Whether the New Tab Page is enabled or not.
    */
   _updateAttributes: function Page_updateAttributes(aValue) {
     // Set the nodes' states.
--- a/browser/base/content/test/newtab/browser.ini
+++ b/browser/base/content/test/newtab/browser.ini
@@ -11,16 +11,17 @@ skip-if = e10s # Bug ?????? - about:newt
 [browser_newtab_bug725996.js]
 [browser_newtab_bug734043.js]
 [browser_newtab_bug735987.js]
 skip-if = os == "mac" # Intermittent failures, bug 898317
 [browser_newtab_bug752841.js]
 [browser_newtab_bug765628.js]
 [browser_newtab_bug876313.js]
 [browser_newtab_bug991111.js]
+[browser_newtab_bug991210.js]
 [browser_newtab_disable.js]
 [browser_newtab_drag_drop.js]
 [browser_newtab_drag_drop_ext.js]
 [browser_newtab_drop_preview.js]
 [browser_newtab_focus.js]
 [browser_newtab_perwindow_private_browsing.js]
 [browser_newtab_reset.js]
 [browser_newtab_sponsored_icon_click.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/newtab/browser_newtab_bug991210.js
@@ -0,0 +1,41 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const PRELOAD_PREF = "browser.newtab.preload";
+
+function runTests() {
+  // turn off preload to ensure that a newtab page loads
+  Services.prefs.setBoolPref(PRELOAD_PREF, false);
+
+  // add a test provider that waits for load
+  let afterLoadProvider = {
+    getLinks: function(callback) {
+      this.callback = callback;
+    },
+    addObserver: function() {},
+  };
+  NewTabUtils.links.addProvider(afterLoadProvider);
+
+  // wait until about:newtab loads before calling provider callback
+  addNewTabPageTab();
+  let browser = gWindow.gBrowser.selectedTab.linkedBrowser;
+  yield browser.addEventListener("load", function onLoad() {
+    browser.removeEventListener("load", onLoad, true);
+    // afterLoadProvider.callback has to be called asynchronously to make grid
+    // initilize after "load" event was handled
+    executeSoon(() => afterLoadProvider.callback([]));
+  }, true);
+
+  let {_cellMargin, _cellHeight, _cellWidth, node} = getGrid();
+  isnot(_cellMargin, null, "grid has a computed cell margin");
+  isnot(_cellHeight, null, "grid has a computed cell height");
+  isnot(_cellWidth, null, "grid has a computed cell width");
+  let {height, maxHeight, maxWidth} = node.style;
+  isnot(height, "", "grid has a computed grid height");
+  isnot(maxHeight, "", "grid has a computed grid max-height");
+  isnot(maxWidth, "", "grid has a computed grid max-width");
+
+  // restore original state
+  NewTabUtils.links.removeProvider(afterLoadProvider);
+  Services.prefs.clearUserPref(PRELOAD_PREF);
+}
--- a/browser/base/content/test/newtab/browser_newtab_update.js
+++ b/browser/base/content/test/newtab/browser_newtab_update.js
@@ -1,21 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /**
  * Checks that newtab is updated as its links change.
  */
 
 function runTests() {
-  if (NewTabUtils.allPages.updateScheduledForHiddenPages) {
-    // Wait for dynamic updates triggered by the previous test to finish.
-    yield whenPagesUpdated(null, true);
-  }
-
   // First, start with an empty page.  setLinks will trigger a hidden page
   // update because it calls clearHistory.  We need to wait for that update to
   // happen so that the next time we wait for a page update below, we catch the
   // right update and not the one triggered by setLinks.
   //
   // Why this weird way of yielding?  First, these two functions don't return
   // promises, they call TestRunner.next when done.  Second, the point at which
   // setLinks is done is independent of when the page update will happen, so
--- a/browser/base/content/test/newtab/head.js
+++ b/browser/base/content/test/newtab/head.js
@@ -9,32 +9,40 @@ Services.prefs.setBoolPref(PREF_NEWTAB_E
 Services.prefs.setCharPref(PREF_NEWTAB_DIRECTORYSOURCE, "data:application/json,{}");
 
 let tmp = {};
 Cu.import("resource://gre/modules/Promise.jsm", tmp);
 Cu.import("resource://gre/modules/NewTabUtils.jsm", tmp);
 Cc["@mozilla.org/moz/jssubscript-loader;1"]
   .getService(Ci.mozIJSSubScriptLoader)
   .loadSubScript("chrome://browser/content/sanitize.js", tmp);
-let {Promise, NewTabUtils, Sanitizer} = tmp;
+Cu.import("resource://gre/modules/Timer.jsm", tmp);
+let {Promise, NewTabUtils, Sanitizer, clearTimeout} = tmp;
 
 let uri = Services.io.newURI("about:newtab", null, null);
 let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
 
 let isMac = ("nsILocalFileMac" in Ci);
 let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);
 let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc);
 let gWindow = window;
 
 registerCleanupFunction(function () {
   while (gWindow.gBrowser.tabs.length > 1)
     gWindow.gBrowser.removeTab(gWindow.gBrowser.tabs[1]);
 
   Services.prefs.clearUserPref(PREF_NEWTAB_ENABLED);
   Services.prefs.clearUserPref(PREF_NEWTAB_DIRECTORYSOURCE);
+
+  // Stop any update timers to prevent unexpected updates in later tests
+  let timer = NewTabUtils.allPages._scheduleUpdateTimeout;
+  if (timer) {
+    clearTimeout(timer);
+    delete NewTabUtils.allPages._scheduleUpdateTimeout;
+  }
 });
 
 /**
  * Provide the default test function to start our test runner.
  */
 function test() {
   TestRunner.run();
 }
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -276,20 +276,16 @@ let AllPages = {
     if (!this._scheduleUpdateTimeout) {
       this._scheduleUpdateTimeout = Timer.setTimeout(() => {
         delete this._scheduleUpdateTimeout;
         this.update(null, true);
       }, SCHEDULE_UPDATE_TIMEOUT_MS);
     }
   },
 
-  get updateScheduledForHiddenPages() {
-    return !!this._scheduleUpdateTimeout;
-  },
-
   /**
    * Implements the nsIObserver interface to get notified when the preference
    * value changes or when a new copy of a page thumbnail is available.
    */
   observe: function AllPages_observe(aSubject, aTopic, aData) {
     if (aTopic == "nsPref:changed") {
       // Clear the cached value.
       this._enabled = null;