Changed identity API to use promises instead of callbacks.

remoting/webapp/crd/js/identity.js

Index: remoting/webapp/crd/js/identity.js
diff --git a/remoting/webapp/crd/js/identity.js b/remoting/webapp/crd/js/identity.js
index 161e083e318069d929c937062d2f39fdf102e469..0ae2dbf457a9f890c4a74adc70116bd895ff7063 100644
--- a/remoting/webapp/crd/js/identity.js
+++ b/remoting/webapp/crd/js/identity.js
@@ -31,8 +31,8 @@ remoting.Identity = function(opt_consentDialog) {
   this.email_ = '';
   /** @type {string} @private */
   this.fullName_ = '';
-  /** @type {Array.<remoting.Identity.Callbacks>} */
-  this.pendingCallbacks_ = [];
+  /** @type {Promise<string>} */
+  this.pendingPromise_ = null;
 };
 
 /**
@@ -51,117 +51,118 @@ remoting.Identity.ConsentDialog = function() {};
 remoting.Identity.ConsentDialog.prototype.show = function() {};
 
 /**
- * Call a function with an access token.
+ * Get an access token.
  *
- * @param {function(string):void} onOk Function to invoke with access token if
- *     an access token was successfully retrieved.
- * @param {function(remoting.Error):void} onError Function to invoke with an
- *     error code on failure.
- * @return {void} Nothing.
+ * @return {!Promise<string>} A promise resolved the an access token or
+ *     rejected with a remoting.Error.
  */
-remoting.Identity.prototype.callWithToken = function(onOk, onError) {
-  this.pendingCallbacks_.push(new remoting.Identity.Callbacks(onOk, onError));
-  if (this.pendingCallbacks_.length == 1) {
-    chrome.identity.getAuthToken(
-        { 'interactive': false },
-        this.onAuthComplete_.bind(this, false));
+remoting.Identity.prototype.getToken = function() {
+  /** @const */
+  var that = this;
+
+  if (this.pendingPromise_ == null) {
+    this.pendingPromise_ = new Promise(function(

[kelvinp, 2015/02/18 19:45:24]

I think base.Deferred will work better as you want to return a promise in one
function but resolves/rejects it in other. 

var that = this;
if (!this.getAuthTokenDeferred_) {
  this.getAuthTokenDeferred_ = new base.Deferred();
  chrome.identity.getAuthToken({ interactive: false },
                               that.onAuthComplete.bind(that, false)) 
}

return this.getAuthTokenDeferred_.promise();
... 

In onAuthComplete
getAuthTokenDeferred_.resolves(token) or
getAuthTokenDeferred_.reject(error)

[John Williams, 2015/02/18 20:28:22]

Done.

+        /** function(string) */ resolve,

[kelvinp, 2015/02/18 19:45:24]

Is this a new syntax of annotating parameters? I like it as it seems to be
shorter. However, we use @param elsewhere.  Let's talk about it and we should be
consistent throughout the codebase.

[John Williams, 2015/02/18 20:28:22]

It's a recent addition to the syntax.  I like it for documenting parameter types
for functions that are nested inside another expression because the @param
syntax is very heavy and easy to get wrong.  The syntax used in a lot of the
codebase is actually wrong because when the doc comment doesn't immediately
precede the "function" keyword, the version of jscompiler in git just ignores it
silently.  Newer versions of jscompiler actually report this case as an error,
so when you use the correct syntax and indent according to standard rules, you
get this mess:

  new Promise(
      /**
       * @param {function(string)} resolve
       * @param {function(remoting.Error)} reject
       */
      function(resolve, reject) {
        ...
      });

Since the new syntax is shorter by about 2 lines, and less indented by 4
columns, I greatly prefer it.

As for consistency, I think the new syntax should be preferred when it avoids
excessive indentation, but the old form should be used elsewhere.  I don't think
it's worth changing existing uses except the ones that are actually broken.

+        /** function(remoting.Error) */ reject) {
+      chrome.identity.getAuthToken(
+          { 'interactive': false },
+          that.onAuthComplete_.bind(that, resolve, reject, false));
+    });
   }
+  return this.pendingPromise_;
 };
 
 /**
- * Call a function with a fresh access token.
+ * Get a fresh access token.

[kelvinp, 2015/02/18 19:45:24]

Nit: Gets

[John Williams, 2015/02/18 20:28:22]

Done.

  *
- * @param {function(string):void} onOk Function to invoke with access token if
- *     an access token was successfully retrieved.
- * @param {function(remoting.Error):void} onError Function to invoke with an
- *     error code on failure.
- * @return {void} Nothing.
+ * @return {!Promise<string>} A promise resolved the an access token or

[kelvinp, 2015/02/18 19:45:24]

s/resolved the an/resolved with an/

+ *     rejected with a remoting.Error.
  */
-remoting.Identity.prototype.callWithNewToken = function(onOk, onError) {
+remoting.Identity.prototype.getNewToken = function() {
   /** @type {remoting.Identity} */
   var that = this;
 
-  /**
-   * @param {string} token
-   */
-  function revokeToken(token) {
-    chrome.identity.removeCachedAuthToken(
-        {'token': token },
-        that.callWithToken.bind(that, onOk, onError));
-  }
-
-  this.callWithToken(revokeToken, onError);
+  return this.getToken().then(function(/** string */ token) {
+    return new Promise(function(resolve, reject) {
+      chrome.identity.removeCachedAuthToken({'token': token }, function() {
+        resolve(that.getToken());
+      });
+    });
+  });
 };
 
 /**
  * Remove the cached auth token, if any.
  *
- * @param {function():void=} opt_onDone Completion callback.
- * @return {void} Nothing.
+ * @return {!Promise<null>} A promise resolved with the operation completes.
  */
-remoting.Identity.prototype.removeCachedAuthToken = function(opt_onDone) {
-  var onDone = (opt_onDone) ? opt_onDone : base.doNothing;
-
-  /** @param {string} token */
-  var onToken = function(token) {
-    if (token) {
-      chrome.identity.removeCachedAuthToken({'token': token}, onDone);
-    } else {
-      onDone();
-    }
-  };
-  chrome.identity.getAuthToken({'interactive': false}, onToken);
+remoting.Identity.prototype.removeCachedAuthToken = function() {
+  return new Promise(function(resolve, reject) {
+    /** @param {string} token */
+    var onToken = function(token) {
+      if (token) {
+        chrome.identity.removeCachedAuthToken(
+            {'token': token}, resolve.bind(null, null));
+      } else {
+        resolve(null);
+      }
+    };
+    chrome.identity.getAuthToken({'interactive': false}, onToken);
+  });
 };
 
 /**
- * Get the user's email address and full name.
- * The full name will be null unless the webapp has requested and been
- * granted the userinfo.profile permission.
+ * Get the user's email address and full name.  The full name will be
+ * null unless the webapp has requested and been granted the
+ * userinfo.profile permission.
+ *
+ * TODO(jrw): Type declarations say the name can't be null.  Are the
+ * types wrong, or is the documentation wrong?
  *
- * @param {function(string,string):void} onOk Callback invoked when the user's
- *     email address and full name are available.
- * @param {function(remoting.Error):void} onError Callback invoked if an
- *     error occurs.
- * @return {void} Nothing.
+ * @return {!Promise<{email: string, name: string}>} Promise

[kelvinp, 2015/02/18 19:45:24]

no space between : and type name

[John Williams, 2015/02/18 20:28:22]

Done.

+ *     resolved with the user's email address and full name, or rejected
+ *     with a remoting.Error.
  */
-remoting.Identity.prototype.getUserInfo = function(onOk, onError) {
+remoting.Identity.prototype.getUserInfo = function() {
   if (this.isAuthenticated()) {
-    onOk(this.email_, this.fullName_);
-    return;
+    /**
+     * The temp variable is needed to work around a compiler bug.
+     * @type {{email: string, name: string}}
+     */
+    var result = {email: this.email_, name: this.fullName_};
+    return Promise.resolve(result);
   }
 
   /** @type {remoting.Identity} */
   var that = this;
 
-  /**
-   * @param {string} email
-   * @param {string} name
-   */
-  var onResponse = function(email, name) {
-    that.email_ = email;
-    that.fullName_ = name;
-    onOk(email, name);
-  };
-
-  this.callWithToken(
-      remoting.oauth2Api.getUserInfo.bind(
-          remoting.oauth2Api, onResponse, onError),
-      onError);
+  return this.getToken().then(function(token) {
+    return new Promise(function(resolve, reject) {
+      /**
+       * @param {string} email
+       * @param {string} name
+       */
+      var onResponse = function(email, name) {
+        that.email_ = email;
+        that.fullName_ = name;
+        resolve({email: email, name: name});
+      };
+
+      remoting.oauth2Api.getUserInfo(onResponse, reject, token);
+    });
+  });
 };
 
 /**
  * Get the user's email address.
  *
- * @param {function(string):void} onOk Callback invoked when the email
- *     address is available.
- * @param {function(remoting.Error):void} onError Callback invoked if an
- *     error occurs.
- * @return {void} Nothing.
+ * @return {!Promise<string>} Promise resolved with the user's email
+ *     address or rejected with a remoting.Error.
  */
-remoting.Identity.prototype.getEmail = function(onOk, onError) {
-  this.getUserInfo(function(email, name) {
-    onOk(email);
-  }, onError);
+remoting.Identity.prototype.getEmail = function() {
+  this.getUserInfo().then(function(userInfo) {
+    return userInfo.email;
+  });
 };
 
 /**
@@ -189,19 +190,19 @@ remoting.Identity.prototype.getCachedUserFullName = function() {
 /**
  * Callback for the getAuthToken API.
  *
+ * @param {function(string):void} resolve
+ * @param {function(remoting.Error):void} reject
  * @param {boolean} interactive The value of the "interactive" parameter to
  *     getAuthToken.
  * @param {?string} token The auth token, or null if the request failed.
  * @private
  */
-remoting.Identity.prototype.onAuthComplete_ = function(interactive, token) {
+remoting.Identity.prototype.onAuthComplete_ = function(
+    resolve, reject, interactive, token) {

[kelvinp, 2015/02/18 19:45:24]

As commented earlier, use base.Deferred instead

[John Williams, 2015/02/18 20:28:22]

Done.

   // Pass the token to the callback(s) if it was retrieved successfully.
   if (token) {
-    while (this.pendingCallbacks_.length > 0) {
-      var callback = /** @type {remoting.Identity.Callbacks} */
-          (this.pendingCallbacks_.shift());
-      callback.onOk(token);
-    }
+    resolve(token);
+    this.pendingPromise_ = null;
     return;
   }
 
@@ -212,11 +213,7 @@ remoting.Identity.prototype.onAuthComplete_ = function(interactive, token) {
         chrome.runtime.lastError ? chrome.runtime.lastError.message
                                  : 'Unknown error.';
     console.error(error_message);
-    while (this.pendingCallbacks_.length > 0) {
-      var callback = /** @type {remoting.Identity.Callbacks} */
-          (this.pendingCallbacks_.shift());
-      callback.onError(remoting.Error.NOT_AUTHENTICATED);
-    }
+    reject(remoting.Error.NOT_AUTHENTICATED);
     return;
   }
 
@@ -227,26 +224,12 @@ remoting.Identity.prototype.onAuthComplete_ = function(interactive, token) {
       (this.consentDialog_) ? this.consentDialog_.show() : Promise.resolve();
   showConsentDialog.then(function() {
     chrome.identity.getAuthToken({'interactive': true},
-                                 that.onAuthComplete_.bind(that, true));
+                                 that.onAuthComplete_.bind(
+                                     that, resolve, reject, true));

[kelvinp, 2015/02/18 19:45:24]

indentation

[John Williams, 2015/02/18 20:28:22]

Done.

   });
 };
 
 /**
- * Internal representation for pair of callWithToken callbacks.
- *
- * @param {function(string):void} onOk
- * @param {function(remoting.Error):void} onError
- * @constructor
- * @private
- */
-remoting.Identity.Callbacks = function(onOk, onError) {
-  /** @type {function(string):void} */
-  this.onOk = onOk;
-  /** @type {function(remoting.Error):void} */
-  this.onError = onError;
-};
-
-/**
  * Returns whether the web app has authenticated with the Google services.
  *
  * @return {boolean}