Issue 4131 - Simplify UI initialization logic

lib/ui.js

Index: lib/ui.js
===================================================================
--- a/lib/ui.js
+++ b/lib/ui.js
@@ -272,75 +272,60 @@ let notificationTimer = null;
 
 let UI = exports.UI =
 {
   /**
    * Gets called on startup, initializes UI integration.
    */
   init: function()
   {
-    // We should call initDone once both overlay and filters are loaded
-    let overlayLoaded = false;
-    let filtersLoaded = false;
-    let sessionRestored = false;
+    // We have to wait for multiple events before running start-up actions
+    let promises = [];

[Thomas Greiner, 2016/06/08 15:47:32]

Detail: That variable name is not very descriptive because it's like calling an
array of numbers "numbers". For instance, I could imagine naming it
"startupActions" based on your comment.

[Wladimir Palant, 2016/06/08 20:51:30]

Done.

 
     // Start loading overlay
-    let request = new XMLHttpRequest();
-    request.mozBackgroundRequest = true;
-    request.open("GET", "chrome://adblockplus/content/ui/overlay.xul");
-    request.channel.owner = Utils.systemPrincipal;
-    request.addEventListener("load", function(event)
+    promises.push(new Promise((resolve, reject) =>
     {
-      if (onShutdown.done)
-        return;
+      let request = new XMLHttpRequest();
+      request.mozBackgroundRequest = true;
+      request.open("GET", "chrome://adblockplus/content/ui/overlay.xul");
+      request.channel.owner = Utils.systemPrincipal;
+      request.addEventListener("load", event =>
+      {
+        if (onShutdown.done)
+          return;

[Thomas Greiner, 2016/06/08 15:47:32]

Instead of returning I'd suggest to resolve or reject the promise instead to
avoid it being in an unresolved state forever. Note that I couldn't find hints
in the standard as to what the browser is supposed to do with promises that
remain unresolved. So seems like it's up to the GC to decide what to do with
those.

Same also applies to the missing handling of the "error" event since the "load"
event is not being dispatched if an error occurs.

[Wladimir Palant, 2016/06/08 20:51:30]

We don't really want to do anything with that promise. If we resolve it, the
follow-up actions will run. If we reject, this will generate an error message.
Neither is desirable at this point, the extension has been shut down. Not doing
anything and allowing the promise to be garbage collected seems to be the best
course of action.
The original code didn't handle the "error" event either, simply because there
is no way this download would error out. In fact, chrome:// is a synchronous
protocol and there are no asynchronous error events. Still, fair enough - let's
fix this and reject the promise on error (had to replace protocol by http:// in
order to test this).

 
-      this.processOverlay(request.responseXML.documentElement);
+        this.processOverlay(request.responseXML.documentElement);
 
-      // Don't wait for the rest of the startup sequence, add icon already
-      this.addToolbarButton();
+        // Don't wait for the rest of the startup sequence, add icon already
+        this.addToolbarButton();
 
-      overlayLoaded = true;
-      if (overlayLoaded && filtersLoaded && sessionRestored)
-        this.initDone();
-    }.bind(this), false);
-    request.send(null);
+        resolve();
+      }, false);
+      request.send(null);
+    }));
 
     // Wait for filters to load
     if (FilterStorage._loading)
+      promises.push(FilterNotifier.once("load"));
+
+    // Wait for session to be restored
+    promises.push(new Promise((resolve, reject) =>
     {
-      let listener = function(action)
+      let window = this.currentWindow;
+      if (!window && "nsISessionStore" in Ci)
       {
-        if (action != "load")
-          return;
+        // No application windows yet, the application must be starting up. Wait
+        // for session to be restored before initializing our UI.
+        new SessionRestoreObserver(resolve);
+      }
+      else
+        resolve();
+    }));
 
-        FilterNotifier.removeListener(listener);
-        filtersLoaded = true;
-        if (overlayLoaded && filtersLoaded && sessionRestored)
-          this.initDone();
-      }.bind(this);
-      FilterNotifier.addListener(listener);
-    }
-    else
-      filtersLoaded = true;
-
-    // Initialize UI after the session is restored
-    let window = this.currentWindow;
-    if (!window && "nsISessionStore" in Ci)
-    {
-      // No application windows yet, the application must be starting up. Wait
-      // for session to be restored before initializing our UI.
-      new SessionRestoreObserver(function()
-      {
-        sessionRestored = true;
-        if (overlayLoaded && filtersLoaded && sessionRestored)
-          this.initDone();
-      }.bind(this));
-    }
-    else
-      sessionRestored = true;
+    Promise.all(promises).then(() => this.initDone());
   },
 
   /**
    * Provesses overlay document data and initializes overlay property.
    */
   processOverlay: function(/**Element*/ root)
   {
     Utils.splitAllLabels(root);