Bug 1208534 - Part 1: Ensure about:logins animated CSS spinner is painted before janky main-thread load. r=ally

Right now, in response to "load" (on the window), we're:

1) updating the DOM to show the spinner;
2) loading the logins with a main-thread janking synchronous load;
3) updating the DOM to hide the spinner.

This is all on the main-thread, so we only see a layout and paint
after 3).  Thus no interstitial is ever visible, and the logins list
pops in after a long delay.

This patch ensures that 2) occurs at least one layout after 1).  This
allows a paint to occur with the interstitial visible.  Since the
animated CSS spinner is carefully designed to hit the off-main-thread
animation pipeline, it animates smoothly even though the main-thread
janking synchronous load blocks JavaScript progress.

There is a small race window between the promises resolving and the
_logins member being accessed by the filter.  It's not clear that this
was ever well guarded, so I haven't tried to mitigate.

--HG--
extra : commitid : 9nKfLhK3JOa
extra : rebase_source : 8eb67ac9322372aa6e049d7154542c31e9de0d43

mobile/android/chrome/content/aboutLogins.js

@@ -4,8 +4,9 @@
 
 var Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils;
 
+Cu.import("resource://services-common/utils.js"); /*global: CommonUtils */
 Cu.import("resource://gre/modules/Messaging.jsm");
-Cu.import("resource://gre/modules/Services.jsm")
+Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/TelemetryStopwatch.jsm");
 
@@ -49,39 +50,97 @@ var Logins = {
   _filterTimer: null,
   _selectedLogin: null,
 
-  _getLogins: function() {
-    let logins;
+  // Load the logins list, displaying interstitial UI (see
+  // #logins-list-loading-body) while loading.  There are careful
+  // jank-avoiding measures taken in this function; be careful when
+  // modifying it!
+  //
+  // Returns a Promise that resolves to the list of logins, ordered by
+  // hostname.
+  _promiseLogins: function() {
     let contentBody = document.getElementById("content-body");
     let emptyBody = document.getElementById("empty-body");
     let filterIcon = document.getElementById("filter-button");
 
+    let showSpinner = () => {
       this._toggleListBody(true);
       emptyBody.classList.add("hidden");
+    };
 
+    let getAllLogins = () => {
+      let logins = [];
       try {
         TelemetryStopwatch.start("PWMGR_ABOUT_LOGINS_GET_ALL_LOGINS_MS");
         logins = Services.logins.getAllLogins();
         TelemetryStopwatch.finish("PWMGR_ABOUT_LOGINS_GET_ALL_LOGINS_MS");
       } catch(e) {
-      // Master password was not entered
-      debug("Master password permissions error: " + e);
-      logins = [];
-    }
-    this._toggleListBody(false);
-
-    if (!logins.length) {
-      emptyBody.classList.remove("hidden");
-
-      filterIcon.classList.add("hidden");
-      contentBody.classList.add("hidden");
-    } else {
-      emptyBody.classList.add("hidden");
-
-      filterIcon.classList.remove("hidden");
+        // It's likely that the Master Password was not entered; give
+        // a hint to the next person.
+        throw new Error("Possible Master Password permissions error: " + e.toString());
       }
 
       logins.sort((a, b) => a.hostname.localeCompare(b.hostname));
-    return this._logins = logins;
+
+      return logins;
+    };
+
+    let hideSpinner = (logins) => {
+      this._toggleListBody(false);
+
+      if (!logins.length) {
+        contentBody.classList.add("hidden");
+        filterIcon.classList.add("hidden");
+        emptyBody.classList.remove("hidden");
+      } else {
+        contentBody.classList.remove("hidden");
+        emptyBody.classList.add("hidden");
+      }
+
+      return logins;
+    };
+
+    // Return a promise that is resolved after a paint.
+    let waitForPaint = () => {
+      // We're changing 'display'.  We need to wait for the new value to take
+      // effect; otherwise, we'll block and never paint a change.  Since
+      // requestAnimationFrame callback is generally triggered *before* any
+      // style flush and layout, we wait for two animation frames.  This
+      // approach was cribbed from
+      // https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/browser/base/content/browser-fullScreen.js?offset=0#469.
+      return new Promise(function(resolve, reject) {
+        requestAnimationFrame(() => {
+          requestAnimationFrame(() => {
+            resolve();
+          });
+        });
+      });
+    };
+
+    // getAllLogins janks the main-thread.  We need to paint before that jank;
+    // by throwing the janky load onto the next tick, we paint the spinner; the
+    // spinner is CSS animated off-main-thread.
+    return Promise.resolve()
+      .then(showSpinner)
+      .then(waitForPaint)
+      .then(getAllLogins)
+      .then(hideSpinner);
+  },
+
+  // Reload the logins list, displaying interstitial UI while loading.
+  // Update the stored and displayed list upon completion.
+  _reloadList: function() {
+    this._promiseLogins()
+      .then((logins) => {
+        this._logins = logins;
+        this._loadList(logins);
+      })
+      .catch((e) => {
+        // There's no way to recover from errors, sadly.  Log and make
+        // it obvious that something is up.
+        this._logins = [];
+        debug("Failed to _reloadList!");
+        Cu.reportError(e);
+      });
   },
 
   _toggleListBody: function(isLoading) {
@@ -95,7 +154,6 @@ var Logins = {
       loadingBody.classList.add("hidden");
       contentBody.classList.remove("hidden");
     }
-
   },
 
   init: function () {
@@ -105,8 +163,6 @@ var Logins = {
     document.getElementById("update-btn").addEventListener("click", this._onSaveEditLogin.bind(this), false);
     document.getElementById("password-btn").addEventListener("click", this._onPasswordBtn.bind(this), false);
 
-    this._loadList(this._getLogins());
-
     let filterInput = document.getElementById("filter-input");
     let filterContainer = document.getElementById("filter-input-container");
 
@@ -147,6 +203,8 @@ var Logins = {
     this._showList();
 
     this._updatePasswordBtn(true);
+
+    this._reloadList();
   },
 
   uninit: function () {
@@ -439,8 +497,7 @@ var Logins = {
   observe: function (subject, topic, data) {
     switch(topic) {
       case "passwordmgr-storage-changed": {
-        // Reload logins content.
-        this._loadList(this._getLogins());
+        this._reloadList();
         break;
       }
     }