Bug 1077652 - Simplify about:newtab page update mechanism and correct behavior to work better with preloading r=gijs

browser/base/content/newtab/page.js

@@ -4,6 +4,9 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 #endif
 
+// The amount of time we wait while coalescing updates for hidden pages.
+const SCHEDULE_UPDATE_TIMEOUT_MS = 1000;
+
 /**
  * This singleton represents the whole 'New Tab Page' and takes care of
  * initializing all its components.
@@ -69,16 +72,39 @@ let gPage = {
   },
 
   /**
-   * Updates the whole page and the grid when the storage has changed.
-   * @param aOnlyIfHidden If true, the page is updated only if it's hidden in
-   *                      the preloader.
+   * Updates the page's grid right away for visible pages. If the page is
+   * currently hidden, i.e. in a background tab or in the preloader, then we
+   * batch multiple update requests and refresh the grid once after a short
+   * delay. Accepts a single parameter the specifies the reason for requesting
+   * a page update. The page may decide to delay or prevent a requested updated
+   * based on the given reason.
    */
-  update: function Page_update(aOnlyIfHidden=false) {
-    let skipUpdate = aOnlyIfHidden && !document.hidden;
-    // The grid might not be ready yet as we initialize it asynchronously.
-    if (gGrid.ready && !skipUpdate) {
-      gGrid.refresh();
+  update(reason = "") {
+    // Update immediately if we're visible.
+    if (!document.hidden) {
+      // Ignore updates where reason=links-changed as those signal that the
+      // provider's set of links changed. We don't want to update visible pages
+      // in that case, it is ok to wait until the user opens the next tab.
+      if (reason != "links-changed" && gGrid.ready) {
+        gGrid.refresh();
+      }
+
+      return;
     }
+
+    // Bail out if we scheduled before.
+    if (this._scheduleUpdateTimeout) {
+      return;
+    }
+
+    this._scheduleUpdateTimeout = setTimeout(() => {
+      // Refresh if the grid is ready.
+      if (gGrid.ready) {
+        gGrid.refresh();
+      }
+
+      this._scheduleUpdateTimeout = null;
+    }, SCHEDULE_UPDATE_TIMEOUT_MS);
   },
 
   /**
@@ -170,6 +196,15 @@ let gPage = {
         }
         break;
       case "visibilitychange":
+        // Cancel any delayed updates for hidden pages now that we're visible.
+        if (this._scheduleUpdateTimeout) {
+          clearTimeout(this._scheduleUpdateTimeout);
+          this._scheduleUpdateTimeout = null;
+
+          // An update was pending so force an update now.
+          this.update();
+        }
+
         setTimeout(() => this.onPageFirstVisible());
         removeEventListener("visibilitychange", this);
         break;

browser/base/content/test/newtab/browser_newtab_bug752841.js

@@ -44,6 +44,10 @@ function runTests() {
       "New page grid is updated correctly.");
 
     gBrowser.removeTab(newTab);
+
+    // Wait until the original tab is visible again.
+    let doc = existingTab.linkedBrowser.contentDocument;
+    yield waitForCondition(() => !doc.hidden).then(TestRunner.next);
   }
 
   gBrowser.removeTab(existingTab);

browser/base/content/test/newtab/browser_newtab_update.js

@@ -1,44 +1,32 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+"use strict";
+
 /**
  * Checks that newtab is updated as its links change.
  */
-
 function runTests() {
   // 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
-  // calling whenPagesUpdated cannot wait until that time.
-  setLinks([]);
-  whenPagesUpdated(null, true);
-  yield null;
-  yield null;
+  yield whenPagesUpdatedAnd(resolve => setLinks([], resolve));
 
   // Strategy: Add some visits, open a new page, check the grid, repeat.
-  fillHistory([link(1)]);
-  yield whenPagesUpdated(null, true);
+  yield fillHistoryAndWaitForPageUpdate([1]);
   yield addNewTabPageTab();
   checkGrid("1,,,,,,,,");
 
-  fillHistory([link(2)]);
-  yield whenPagesUpdated(null, true);
+  yield fillHistoryAndWaitForPageUpdate([2]);
   yield addNewTabPageTab();
   checkGrid("2,1,,,,,,,");
 
-  fillHistory([link(1)]);
-  yield whenPagesUpdated(null, true);
+  yield fillHistoryAndWaitForPageUpdate([1]);
   yield addNewTabPageTab();
   checkGrid("1,2,,,,,,,");
 
-  // Wait for fillHistory to add all links before waiting for an update
-  yield fillHistory([link(2), link(3), link(4)], TestRunner.next);
-  yield whenPagesUpdated(null, true);
+  yield fillHistoryAndWaitForPageUpdate([2, 3, 4]);
   yield addNewTabPageTab();
   checkGrid("2,1,3,4,,,,,");
 
@@ -46,6 +34,16 @@ function runTests() {
   is(getCell(1).site.link.type, "history", "added link is history");
 }
 
+function fillHistoryAndWaitForPageUpdate(links) {
+  return whenPagesUpdatedAnd(resolve => fillHistory(links.map(link), resolve));
+}
+
+function whenPagesUpdatedAnd(promiseConstructor) {
+  let promise1 = new Promise(whenPagesUpdated);
+  let promise2 = new Promise(promiseConstructor);
+  return Promise.all([promise1, promise2]).then(TestRunner.next);
+}
+
 function link(id) {
   return { url: "http://example" + id + ".com/", title: "site#" + id };
 }

browser/base/content/test/newtab/head.js

@@ -214,7 +214,7 @@ function getCell(aIndex) {
  *          {url: "http://example2.com/", title: "site#2"},
  *          {url: "http://example3.com/", title: "site#3"}]
  */
-function setLinks(aLinks) {
+function setLinks(aLinks, aCallback = TestRunner.next) {
   let links = aLinks;
 
   if (typeof links == "string") {
@@ -233,7 +233,7 @@ function setLinks(aLinks) {
       fillHistory(links, function () {
         NewTabUtils.links.populateCache(function () {
           NewTabUtils.allPages.update();
-          TestRunner.next();
+          aCallback();
         }, true);
       });
     });
@@ -249,7 +249,7 @@ function clearHistory(aCallback) {
   PlacesUtils.history.removeAllPages();
 }
 
-function fillHistory(aLinks, aCallback) {
+function fillHistory(aLinks, aCallback = TestRunner.next) {
   let numLinks = aLinks.length;
   if (!numLinks) {
     if (aCallback)
@@ -323,6 +323,33 @@ function restore() {
   NewTabUtils.restore();
 }
 
+/**
+ * Wait until a given condition becomes true.
+ */
+function waitForCondition(aConditionFn, aMaxTries=50, aCheckInterval=100) {
+  return new Promise((resolve, reject) => {
+    let tries = 0;
+
+    function tryNow() {
+      tries++;
+
+      if (aConditionFn()) {
+        resolve();
+      } else if (tries < aMaxTries) {
+        tryAgain();
+      } else {
+        reject("Condition timed out: " + aConditionFn.toSource());
+      }
+    }
+
+    function tryAgain() {
+      setTimeout(tryNow, aCheckInterval);
+    }
+
+    tryAgain();
+  });
+}
+
 /**
  * Creates a new tab containing 'about:newtab'.
  */
@@ -349,7 +376,7 @@ function addNewTabPageTabPromise() {
 
   // The new tab page might have been preloaded in the background.
   if (browser.contentDocument.readyState == "complete") {
-    whenNewTabLoaded();
+    waitForCondition(() => !browser.contentDocument.hidden).then(whenNewTabLoaded);
     return deferred.promise;
   }
 
@@ -617,18 +644,14 @@ function createDragEvent(aEventType, aData) {
 /**
  * Resumes testing when all pages have been updated.
  * @param aCallback Called when done. If not specified, TestRunner.next is used.
- * @param aOnlyIfHidden If true, this resumes testing only when an update that
- *                      applies to pre-loaded, hidden pages is observed.  If
- *                      false, this resumes testing when any update is observed.
  */
-function whenPagesUpdated(aCallback, aOnlyIfHidden=false) {
+function whenPagesUpdated(aCallback = TestRunner.next) {
   let page = {
     observe: _ => _,
-    update: function (onlyIfHidden=false) {
-      if (onlyIfHidden == aOnlyIfHidden) {
-        NewTabUtils.allPages.unregister(this);
-        executeSoon(aCallback || TestRunner.next);
-      }
+
+    update() {
+      NewTabUtils.allPages.unregister(this);
+      executeSoon(aCallback);
     }
   };
 

toolkit/modules/NewTabUtils.jsm

@@ -22,10 +22,6 @@ XPCOMUtils.defineLazyModuleGetter(this, "PageThumbs",
 XPCOMUtils.defineLazyModuleGetter(this, "BinarySearch",
   "resource://gre/modules/BinarySearch.jsm");
 
-XPCOMUtils.defineLazyGetter(this, "Timer", () => {
-  return Cu.import("resource://gre/modules/Timer.jsm", {});
-});
-
 XPCOMUtils.defineLazyGetter(this, "gPrincipal", function () {
   let uri = Services.io.newURI("about:newtab", null, null);
   return Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
@@ -61,9 +57,6 @@ const LINKS_GET_LINKS_LIMIT = 100;
 // The gather telemetry topic.
 const TOPIC_GATHER_TELEMETRY = "gather-telemetry";
 
-// The amount of time we wait while coalescing updates for hidden pages.
-const SCHEDULE_UPDATE_TIMEOUT_MS = 1000;
-
 /**
  * Calculate the MD5 hash for a string.
  * @param aValue
@@ -281,30 +274,16 @@ let AllPages = {
   /**
    * Updates all currently active pages but the given one.
    * @param aExceptPage The page to exclude from updating.
-   * @param aHiddenPagesOnly If true, only pages hidden in the preloader are
-   *                         updated.
+   * @param aReason The reason for updating all pages.
    */
-  update: function AllPages_update(aExceptPage, aHiddenPagesOnly=false) {
+  update(aExceptPage, aReason = "") {
     this._pages.forEach(function (aPage) {
-      if (aExceptPage != aPage)
-        aPage.update(aHiddenPagesOnly);
+      if (aExceptPage != aPage) {
+        aPage.update(aReason);
+      }
     });
   },
 
-  /**
-   * Many individual link changes may happen in a small amount of time over
-   * multiple turns of the event loop.  This method coalesces updates by waiting
-   * a small amount of time before updating hidden pages.
-   */
-  scheduleUpdateForHiddenPages: function AllPages_scheduleUpdateForHiddenPages() {
-    if (!this._scheduleUpdateTimeout) {
-      this._scheduleUpdateTimeout = Timer.setTimeout(() => {
-        delete this._scheduleUpdateTimeout;
-        this.update(null, true);
-      }, SCHEDULE_UPDATE_TIMEOUT_MS);
-    }
-  },
-
   /**
    * Implements the nsIObserver interface to get notified when the preference
    * value changes or when a new copy of a page thumbnail is available.
@@ -1016,8 +995,9 @@ let Links = {
       updatePages = true;
     }
 
-    if (updatePages)
-      AllPages.scheduleUpdateForHiddenPages();
+    if (updatePages) {
+      AllPages.update(null, "links-changed");
+    }
   },
 
   /**
@@ -1025,7 +1005,7 @@ let Links = {
    */
   onManyLinksChanged: function Links_onManyLinksChanged(aProvider) {
     this._populateProviderCache(aProvider, () => {
-      AllPages.scheduleUpdateForHiddenPages();
+      AllPages.update(null, "links-changed");
     }, true);
   },