From 3fde34e83554937303373e102095d50d9a1e4cca Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Fri, 2 Oct 2015 16:24:31 -0700 Subject: [PATCH] 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. --- mobile/android/chrome/content/aboutLogins.js | 121 ++++++++++++++----- 1 file changed, 89 insertions(+), 32 deletions(-) diff --git a/mobile/android/chrome/content/aboutLogins.js b/mobile/android/chrome/content/aboutLogins.js index dbd89294471..0d8a1c961a6 100644 --- a/mobile/android/chrome/content/aboutLogins.js +++ b/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"); - this._toggleListBody(true); - emptyBody.classList.add("hidden"); - - 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 { + let showSpinner = () => { + this._toggleListBody(true); emptyBody.classList.add("hidden"); + }; - filterIcon.classList.remove("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) { + // 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; + logins.sort((a, b) => a.hostname.localeCompare(b.hostname)); + + 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; } }