Issue 4131 - Simplify UI initialization logic

lib/ui.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 259 matching lines @@
  */
 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);
 
     let specialElements = {"abp-status-popup": true, "abp-status": true, "abp-toolbarbutton": true, "abp-menuitem": true, "abp-bottombar-container": true};
@@ skipping 1543 matching lines @@
   ["abp-command-contribute-hide", "command", UI.hideContributeButton.bind(UI)],
   ["abp-command-toggleshownotifications", "command", Notification.toggleIgnoreCategory.bind(Notification, "*", null)]
 ];
 
 onShutdown.add(function()
 {
   for (let window of UI.applicationWindows)
     if (UI.isBottombarOpen(window))
       UI.toggleBottombar(window);
 });