Bug 1017394 In MozLoopService use promises rather than callbacks to make the code cleaner. r=mhammond

browser/components/loop/MozLoopAPI.jsm

@@ -98,7 +98,13 @@ function injectLoopAPI(targetWindow) {
       configurable: true,
       writable: true,
       value: function(callback) {
-        return MozLoopService.register(callback);
+        // We translate from a promise to a callback, as we can't pass promises from
+        // Promise.jsm across the priv versus unpriv boundary.
+        return MozLoopService.register().then(() => {
+          callback(null);
+        }, err => {
+          callback(err);
+        });
       }
     },
 

browser/components/loop/MozLoopService.jsm

@@ -8,6 +8,7 @@ const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://gre/modules/Promise.jsm");
 let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
 
 this.EXPORTED_SYMBOLS = ["MozLoopService"];
@@ -155,21 +156,22 @@ let MozLoopServiceInternal = {
   // The uri of the Loop server.
   loopServerUri: Services.prefs.getCharPref("loop.server"),
 
-  // Any callbacks needed when the registration process completes.
-  registrationCallbacks: [],
+  // The current deferred for the registration process. This is set if in progress
+  // or the registration was successful. This is null if a registration attempt was
+  // unsuccessful.
+  _registeredDeferred: null,
 
   /**
    * The initial delay for push registration. This ensures we don't start
    * kicking off straight after browser startup, just a few seconds later.
    */
   get initialRegistrationDelayMilliseconds() {
-    // Default to 5 seconds
-    let initialDelay = 5000;
     try {
       // Let a pref override this for developer & testing use.
-      initialDelay = Services.prefs.getIntPref("loop.initialDelay");
+      return Services.prefs.getIntPref("loop.initialDelay");
     } catch (x) {
-      // It is ok for the pref not to exist.
+      // Default to 5 seconds
+      return 5000;
     }
     return initialDelay;
   },
@@ -180,14 +182,12 @@ let MozLoopServiceInternal = {
    * In seconds since epoch.
    */
   get expiryTimeSeconds() {
-    let expiryTimeSeconds = 0;
     try {
-      expiryTimeSeconds = Services.prefs.getIntPref("loop.urlsExpiryTimeSeconds");
+      return Services.prefs.getIntPref("loop.urlsExpiryTimeSeconds");
     } catch (x) {
       // It is ok for the pref not to exist.
+      return 0;
     }
-
-    return expiryTimeSeconds;
   },
 
   /**
@@ -218,94 +218,27 @@ let MozLoopServiceInternal = {
     Services.prefs.setBoolPref("loop.do_not_disturb", Boolean(aFlag));
   },
 
-  /**
-   * Starts the initialization of the service, which goes and registers
-   * with the push server and the loop server.
-   *
-   * Callback parameters:
-   * - err null on successful initialization and registration,
-   *       false if initialization is complete, but registration has not taken
-   *             place,
-   *       <other> anything else if registration has failed.
-   *
-   * @param {Function} callback Optional, called when initialization finishes.
-   */
-  initialize: function(callback) {
-    if (this.registeredPushServer || this.initalizeTimer || this.registrationInProgress) {
-      if (callback)
-        callback(this.registeredPushServer ? null : false);
-      return;
-    }
-
-    function secondsToMilli(value) {
-      return value * 1000;
-    }
-
-    // If expiresTime is in the future then kick-off registration.
-    if (secondsToMilli(this.expiryTimeSeconds) > Date.now()) {
-      // Kick off the push notification service into registering after a timeout
-      // this ensures we're not doing too much straight after the browser's finished
-      // starting up.
-      this.initializeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
-      this.initializeTimer.initWithCallback(function() {
-        this.registerWithServers(callback);
-        this.initializeTimer = null;
-      }.bind(this),
-      this.initialRegistrationDelayMilliseconds, Ci.nsITimer.TYPE_ONE_SHOT);
-    }
-    else if (callback) {
-      // Callback with false as we haven't needed to do registration.
-      callback(false);
-    }
-  },
-
   /**
    * Starts registration of Loop with the push server, and then will register
    * with the Loop server. It will return early if already registered.
    *
-   * Callback parameters:
-   * - err null on successful registration, non-null otherwise.
-   *
-   * @param {Function} callback Called when the registration process finishes.
+   * @returns {Promise} a promise that is resolved with no params on completion, or
+   *          rejected with an error code or string.
    */
-  registerWithServers: function(callback) {
-    // If we've already registered, return straight away, but save the callback
-    // so we can let the caller know.
-    if (this.registeredLoopServer) {
-      callback(null);
-      return;
+  promiseRegisteredWithServers: function() {
+    if (this._registeredDeferred) {
+      return this._registeredDeferred.promise;
     }
 
-    // We need to register, so save the callback.
-    if (callback) {
-      this.registrationCallbacks.push(callback);
-    }
-
-    // If we're already in progress, just return straight away.
-    if (this.registrationInProgress) {
-      return;
-    }
-
-    this.registrationInProgress = true;
+    this._registeredDeferred = Promise.defer();
+    // We grab the promise early in case .initialize or its results sets
+    // it back to null on error.
+    let result = this._registeredDeferred.promise;
 
     PushHandlerHack.initialize(this.onPushRegistered.bind(this),
                                this.onHandleNotification.bind(this));
-  },
 
-  /**
-   * Handles the end of the registration process.
-   *
-   * @param {Object|null} err null on success, non-null otherwise.
-   */
-  endRegistration: function(err) {
-    // Reset the in progress flag
-    this.registrationInProgress = false;
-
-    // Call any callbacks, then release them.
-    this.registrationCallbacks.forEach(function(callback) {
-      callback(err);
-    });
-    this.registrationCallbacks.length = 0;
+    return result;
   },
 
   /**
@@ -316,7 +249,8 @@ let MozLoopServiceInternal = {
    */
   onPushRegistered: function(err, pushUrl) {
     if (err) {
-      this.endRegistration(err);
+      this._registeredDeferred.reject(err);
+      this._registeredDeferred = null;
       return;
     }
 
@@ -364,7 +298,8 @@ let MozLoopServiceInternal = {
       // XXX Bubble the precise details up to the UI somehow (bug 1013248).
       Cu.reportError("Failed to register with the loop server. Code: " +
         status + " Text: " + this.loopXhr.statusText);
-      this.endRegistration(status);
+      this._registeredDeferred.reject(status);
+      this._registeredDeferred = null;
       return;
     }
 
@@ -378,14 +313,17 @@ let MozLoopServiceInternal = {
       } else {
         // XXX Bubble the precise details up to the UI somehow (bug 1013248).
         console.warn("Loop server sent an invalid session token");
-        this.endRegistration("session-token-wrong-size");
+        this._registeredDeferred.reject("session-token-wrong-size");
+        this._registeredDeferred = null;
         return;
       }
     }
 
     // If we made it this far, we registered just fine.
     this.registeredLoopServer = true;
-    this.endRegistration(null);
+    this._registeredDeferred.resolve();
+    // No need to clear the promise here, everything was good, so we don't need
+    // to re-register.
   },
 
   /**
@@ -465,30 +403,39 @@ this.MozLoopService = {
   /**
    * Initialized the loop service, and starts registration with the
    * push and loop servers.
-   *
-   * Callback parameters:
-   * - err null on successful initialization and registration,
-   *       false if initialization is complete, but registration has not taken
-   *             place,
-   *       <other> anything else if registration has failed.
-   *
-   * @param {Function} callback Optional, called when initialization finishes.
    */
-  initialize: function(callback) {
-    MozLoopServiceInternal.initialize(callback);
+  initialize: function() {
+    // If expiresTime is in the future then kick-off registration.
+    if ((MozLoopServiceInternal.expiryTimeSeconds * 1000) > Date.now()) {
+      this._startInitializeTimer();
+    }
+  },
+
+  /**
+   * Internal function, exposed for testing purposes only. Used to start the
+   * initialize timer.
+   */
+  _startInitializeTimer: function() {
+    // Kick off the push notification service into registering after a timeout
+    // this ensures we're not doing too much straight after the browser's finished
+    // starting up.
+    this._initializeTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+    this._initializeTimer.initWithCallback(function() {
+      this.register();
+      this._initializeTimer = null;
+    }.bind(this),
+    MozLoopServiceInternal.initialRegistrationDelayMilliseconds, Ci.nsITimer.TYPE_ONE_SHOT);
   },
 
   /**
    * Starts registration of Loop with the push server, and then will register
    * with the Loop server. It will return early if already registered.
    *
-   * Callback parameters:
-   * - err null on successful registration, non-null otherwise.
-   *
-   * @param {Function} callback Called when the registration process finishes.
+   * @returns {Promise} a promise that is resolved with no params on completion, or
+   *          rejected with an error code or string.
    */
-  register: function(callback) {
-    MozLoopServiceInternal.registerWithServers(callback);
+  register: function() {
+    return MozLoopServiceInternal.promiseRegisteredWithServers();
   },
 
   /**

browser/components/loop/test/xpcshell/test_loopservice_initialize.js

@@ -8,51 +8,51 @@ var server;
 
 const kServerPushUrl = "http://localhost:3456";
 
+var startTimerCalled = false;
+
 /**
  * Tests that registration doesn't happen when the expiry time is
  * not set.
  */
-add_test(function test_initialize_no_expiry() {
-  MozLoopService.initialize(function(err) {
-    Assert.equal(err, false,
-      "should not register when no expiry time is set");
+add_task(function test_initialize_no_expiry() {
+  startTimerCalled = false;
 
-    run_next_test();
-  });
+  MozLoopService.initialize();
+
+  Assert.equal(startTimerCalled, false,
+    "should not register when no expiry time is set");
 });
 
 /**
  * Tests that registration doesn't happen when the expiry time is
  * in the past.
  */
-add_test(function test_initialize_expiry_past() {
+add_task(function test_initialize_expiry_past() {
   // Set time to be 2 seconds in the past.
   var nowSeconds = Date.now() / 1000;
   Services.prefs.setIntPref("loop.urlsExpiryTimeSeconds", nowSeconds - 2);
+  startTimerCalled = false;
 
-  MozLoopService.initialize(function(err) {
-    Assert.equal(err, false,
-      "should not register when expiry time is in past");
+  MozLoopService.initialize();
 
-    run_next_test();
-  });
+  Assert.equal(startTimerCalled, false,
+    "should not register when expiry time is in past");
 });
 
 /**
  * Tests that registration happens when the expiry time is in
  * the future.
  */
-add_test(function test_initialize_and_register() {
+add_task(function test_initialize_starts_timer() {
   // Set time to be 1 minute in the future
   var nowSeconds = Date.now() / 1000;
   Services.prefs.setIntPref("loop.urlsExpiryTimeSeconds", nowSeconds + 60);
+  startTimerCalled = false;
 
-  MozLoopService.initialize(function(err) {
-    Assert.equal(err, null,
-      "should not register when expiry time is in past");
+  MozLoopService.initialize();
 
-    run_next_test();
-  });
+  Assert.equal(startTimerCalled, true,
+    "should start the timer when expiry time is in the future");
 });
 
 function run_test()
@@ -70,8 +70,11 @@ function run_test()
   Services.prefs.setCharPref("services.push.serverURL", kServerPushUrl);
   Services.prefs.setCharPref("loop.server", "http://localhost:" + server.identity.primaryPort);
 
-  // Set the initial registration delay to a short value for fast run tests.
-  Services.prefs.setIntPref("loop.initialDelay", 10);
+  // Override MozLoopService's initializeTimer, so that we can verify the timeout is called
+  // correctly.
+  MozLoopService._startInitializeTimer = function() {
+    startTimerCalled = true;
+  };
 
   do_register_cleanup(function() {
     gMockWebSocketChannelFactory.unregister();

browser/components/loop/test/xpcshell/test_loopservice_registration.js

@@ -15,9 +15,10 @@ add_test(function test_register_offline() {
   // It should callback with failure if in offline mode
   Services.io.offline = true;
 
-  MozLoopService.register(function(err) {
-    Assert.equal(err, "offline", "Expected error of 'offline' to be returned");
-
+  MozLoopService.register().then(() => {
+    do_throw("should not succeed when offline");
+  }, err => {
+    Assert.equal(err, "offline", "should reject with 'offline' when offline");
     Services.io.offline = false;
     run_next_test();
   });
@@ -28,7 +29,9 @@ add_test(function test_register_offline() {
  * failure is reported.
  */
 add_test(function test_register_websocket_success_loop_server_fail() {
-  MozLoopService.register(function(err) {
+  MozLoopService.register().then(() => {
+    do_throw("should not succeed when loop server registration fails");
+  }, err => {
     // 404 is an expected failure indicated by the lack of route being set
     // up on the Loop server mock. This is added in the next test.
     Assert.equal(err, 404, "Expected no errors in websocket registration");
@@ -65,22 +68,18 @@ add_test(function test_registration_returns_hawk_session_token() {
     response.finish();
   });
 
-  MozLoopService.register(function(err) {
-
-    Assert.equal(err, null, "Should not cause an error to be called back on" +
-      " an otherwise valid request");
-
+  MozLoopService.register().then(() => {
     var hawkSessionPref;
     try {
       hawkSessionPref = Services.prefs.getCharPref("loop.hawk-session-token");
     } catch (ex) {
-      // avoid slowing/hanging the tests if the pref isn't there
-      dump("unexpected exception: " + ex + "\n");
     }
     Assert.equal(hawkSessionPref, fakeSessionToken, "Should store" +
       " Hawk-Session-Token header contents in loop.hawk-session-token pref");
 
     run_next_test();
+  }, err => {
+    do_throw("shouldn't error on a succesful request");
   });
 });
 
@@ -99,15 +98,14 @@ add_test(function test_register_success() {
     response.processAsync();
     response.finish();
   });
-
-  MozLoopService.register(function(err) {
-    Assert.equal(err, null, "Expected no errors in websocket registration");
-
+  MozLoopService.register().then(() => {
     let instances = gMockWebSocketChannelFactory.createdInstances;
     Assert.equal(instances.length, 1,
                  "Should create only one instance of websocket");
 
     run_next_test();
+  }, err => {
+    do_throw("shouldn't error on a succesful request");
   });
 });
 

browser/components/loop/test/xpcshell/test_loopservice_token_validation.js

@@ -20,7 +20,9 @@ add_test(function test_registration_handles_bogus_hawk_token() {
     response.finish();
   });
 
-  MozLoopService.register(function(err) {
+  MozLoopService.register().then(() => {
+    do_throw("should not succeed with a bogus token");
+  }, err => {
 
     Assert.equal(err, "session-token-wrong-size", "Should cause an error to be" +
       " called back if the session-token is not 64 characters long");