Replace own callback handling with Promises.

chrome/browser/resources/options/managed_user_list_data.js

Index: chrome/browser/resources/options/managed_user_list_data.js
diff --git a/chrome/browser/resources/options/managed_user_list_data.js b/chrome/browser/resources/options/managed_user_list_data.js
index cae63d59a5d72864b28da5724563f944782b6510..8746fd469ff3c29fd4a5aba8386ea6484d42e36d 100644
--- a/chrome/browser/resources/options/managed_user_list_data.js
+++ b/chrome/browser/resources/options/managed_user_list_data.js
@@ -6,34 +6,23 @@ cr.define('options', function() {
   /**
    * ManagedUserListData class.
    * Handles requests for retrieving a list of existing managed users which are
-   * supervised by the current profile. This list is cached in order to make it
-   * possible to serve future requests immediately. The first request will be
-   * handled asynchronously.
+   * supervised by the current profile. For each request a promise is returned,
+   * which is cached in order to reuse the retrieved managed users for future
+   * requests. The first request will be handled asynchronously.
    * @constructor
    * @class
    */
   function ManagedUserListData() {
-    this.callbacks_ = [];
-    this.errbacks_ = [];
-    this.requestInProgress_ = false;
+    this.promise_ = null;

[Bernhard Bauer, 2014/01/10 13:52:08]

You could get by without this initialization if you don't compare to null (just
use the values directly in the if-statements).

[Adrian Kuegel, 2014/01/10 16:52:01]

If I don't initialize those values, would I still list them here? From a
readability perspective I think that would make sense, so it is clear which
properties the class has. But at the same time this seems weird, because this is
probably a no-op.

[Bernhard Bauer, 2014/01/13 14:33:50]

No, you'd completely remove them here.
Another thing I have seen is to list the default values (null or undef) on the
prototype. As soon as you assign a value, it will override the value from the
protoype, and it allows you to add JSDoc annotations to the fields.

[Adrian Kuegel, 2014/01/13 15:43:37]

Ok, I removed them now.
Yes, I think I have seen that as well, especially in closure code. However, in
Chrome code it seems to be usually not the case that the variables are set to
their default value in the constructor. So I decided now not to do this.

+    this.resolve_ = null;
+    this.reject_ = null;
     this.managedUsers_ = null;

[Bernhard Bauer, 2014/01/10 13:52:08]

This isn't used anymore.

[Adrian Kuegel, 2014/01/10 16:52:01]

Right. I removed it now.

   };
 
   cr.addSingletonGetter(ManagedUserListData);
 
   /**
-   * Resets to the initial state of no pending requests.
-   * @private
-   */
-  ManagedUserListData.prototype.reset_ = function() {
-    this.callbacks_ = [];
-    this.errbacks_ = [];
-    this.requestInProgress_ = false;
-  }
-
-  /**
-   * Receives a list of managed users and passes the list to each of the
-   * callbacks.
+   * Receives a list of managed users and resolves the promise.
    * @param {Array.<Object>} managedUsers An array of managed user objects.
    *     Each object is of the form:
    *       managedUser = {
@@ -41,73 +30,84 @@ cr.define('options', function() {
    *         name: "Managed User Name",
    *         iconURL: "chrome://path/to/icon/image",
    *         onCurrentDevice: true or false,
+   *         nameConflict: true or false,
    *         needAvatar: true or false
    *       }
+   * @private
    */
-  ManagedUserListData.receiveExistingManagedUsers = function(managedUsers) {
-    var instance = ManagedUserListData.getInstance();
-    var i;
-    for (i = 0; i < instance.callbacks_.length; i++) {
-      instance.callbacks_[i](managedUsers);
-    }
-    instance.managedUsers_ = managedUsers;
-    instance.reset_();
+  ManagedUserListData.prototype.receiveExistingManagedUsers_ = function(
+      managedUsers) {
+    if (this.promise_ == null)

[Bernhard Bauer, 2014/01/10 13:52:08]

I'd rather you didn't change the production code to make the test work, but
change the test code. 

To answer your question, promises *always* resolve asynchronously (on the next
turn of the event loop, to be exact), not only the first time.

[Adrian Kuegel, 2014/01/10 16:52:01]

Ok, I changed it now. But I think it then makes sense to assert that |promise_|
is not null.
Ok, thanks for the explanation. I guess I then also need to add a few more
then() to the test code to make sure it always works.

[Bernhard Bauer, 2014/01/13 14:33:50]

Well, you'll get an exception if it is null, right?

[Adrian Kuegel, 2014/01/13 15:43:37]

I am not sure if I would get an exception if |promise_| is null, but |resolve_|
is not null (for example after calling resetPromise()). Of course I could also
reset |reject_| and |resolve_| in resetPromise(), maybe that is cleaner? What do
you think?

+      this.promise_ = this.createPromise_();
+    this.resolve_(managedUsers);
   };
 
   /**
    * Called when there is a signin error when retrieving the list of managed
-   * users. Calls the error callbacks which will display an appropriate error
-   * message to the user.
+   * users. Rejects the promise and resets the cached promise to null.
+   * @private
    */
-  ManagedUserListData.onSigninError = function() {
-    var instance = ManagedUserListData.getInstance();
-    var i;
-    for (i = 0; i < instance.errbacks_.length; i++) {
-      instance.errbacks_[i]();
-    }
-    // Reset the list of managed users in order to avoid showing stale data.
-    instance.managedUsers_ = null;
-    instance.reset_();
+  ManagedUserListData.prototype.onSigninError_ = function() {
+    if (!this.promise_)
+      return;
+
+    this.reject_();
+    this.promise_ = null;

[Bernhard Bauer, 2014/01/10 13:52:08]

You could call resetPromise_() if you already have it.

[Adrian Kuegel, 2014/01/10 16:52:01]

Done.

   };
 
   /**
-   * Handles the request for the list of existing managed users. If the data is
-   * already available, it will call |callback| immediately. Otherwise, it
-   * retrieves the list of existing managed users which is then processed in
-   * receiveExistingManagedUsers().
-   * @param {Object} callback The callback function which is called on success.
-   * @param {Object} errback the callback function which is called on error.
+   * Handles the request for the list of existing managed users by returning a
+   * promise for the requested data. If there is no cached promise yet, a new
+   * one will be created.
+   * @return {Promise} The promise containing the list of managed users.
+   * @private
    */
-  ManagedUserListData.requestExistingManagedUsers = function(callback,
-                                                             errback) {
-    var instance = ManagedUserListData.getInstance();
-    instance.callbacks_.push(callback);
-    instance.errbacks_.push(errback);
-    if (!instance.requestInProgress_) {
-      if (instance.managedUsers_ != null) {
-        ManagedUserListData.receiveExistingManagedUsers(instance.managedUsers_);
-        return;
-      }
-      instance.requestInProgress_ = true;
-      chrome.send('requestManagedUserImportUpdate');
-    }
+  ManagedUserListData.prototype.requestExistingManagedUsers_ = function() {
+    if (this.promise_ != null)
+      return this.promise_;
+    this.promise_ = this.createPromise_();
+    chrome.send('requestManagedUserImportUpdate');
+    return this.promise_;
   };
 
   /**
-   * Reload the list of existing managed users. Should be called when a new
-   * supervised user profile was created or a supervised user profile was
-   * deleted.
+   * Creates the promise containing the list of managed users. The promise is
+   * resolved in receiveExistingManagedUsers_() or rejected in
+   * onSigninError_(). The promise is cached, so that for future requests it can
+   * be resolved immediately.
+   * @return {Promise} The promise containing the list of managed users.
+   * @private
    */
-  ManagedUserListData.reloadExistingManagedUsers = function() {
-    var instance = ManagedUserListData.getInstance();
-    if (instance.requestInProgress_)
-      return;
+  ManagedUserListData.prototype.createPromise_ = function() {
+    var self = this;
+    return new Promise(function(resolve, reject) {
+      self.resolve_ = resolve;
+      self.reject_ = reject;
+    });
+  };
 
-    instance.managedUsers_ = null;
-    instance.requestInProgress_ = true;
-    chrome.send('requestManagedUserImportUpdate');
+  /**
+   * Resets the promise to null in order to avoid stale data. For the next
+   * request, a new promise will be created.
+   * @private
+   */
+  ManagedUserListData.prototype.resetPromise_ = function() {
+    this.promise_ = null;
   };
 
+  // Forward public APIs to private implementations.
+  [
+    'onSigninError',
+    'receiveExistingManagedUsers',
+    'requestExistingManagedUsers',
+    'resetPromise',
+  ].forEach(function(name) {
+    ManagedUserListData[name] = function() {
+      var instance = ManagedUserListData.getInstance();
+      return instance[name + '_'].apply(instance, arguments);
+    };
+  });
+
   // Export
   return {
     ManagedUserListData: ManagedUserListData,