Add error handling for supervised user import flow.

chrome/browser/resources/options/managed_user_list_data.js

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+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.
+   * @constructor
+   * @class
+   */
+  function ManagedUserListData() {
+    this.callbacks_ = [];
+    this.errbacks_ = [];
+    this.requestInProgress_ = false;
+    this.managedUsers_ = null;
+  };
+
+  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.
+   * @param {Array.<Object>} managedUsers An array of managed user objects.
+   *     Each object is of the form:
+   *       managedUser = {
+   *         id: "Managed User ID",
+   *         name: "Managed User Name",
+   *         iconURL: "chrome://path/to/icon/image",
+   *         onCurrentDevice: true or false,
+   *         needAvatar: true or false
+   *       }
+   */
+  ManagedUserListData.receiveExistingManagedUsers = function(managedUsers) {
+    var instance = ManagedUserListData.getInstance();
+    var i;

[Bernhard Bauer, 2014/01/09 13:39:08]

Nit: You can inline the var into the for loop.

[Adrian Kuegel, 2014/01/09 14:12:42]

Right. I am just not used to inline this into the for loop because jslint.com
complains about that, and I used that tool when I started with javascript. I
currently work on another CL which also touches this file, and I can change it
there.

+    for (i = 0; i < instance.callbacks_.length; i++) {
+      instance.callbacks_[i](managedUsers);
+    }
+    instance.managedUsers_ = managedUsers;
+    instance.reset_();
+  };
+
+  /**
+   * 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.
+   */
+  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_();
+  };
+
+  /**
+   * 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.

[Bernhard Bauer, 2014/01/09 13:39:08]

It might be nice to do the whole thing with DOM futures... :)

That would actually make some of the behavior really nice, like caching the
|managed_users_| list (we just create the promise lazily and send out the
request to the browser when creating it, then keep it around, so future requests
will be immediately served by the fulfilled promise). 

[Adrian Kuegel, 2014/01/09 14:12:42]

I already have the caching in the code. I didn't know about DOM futures, I will
read about them now. Maybe they can make some of the code I have written
obsolete. However one functionality we need is that we can invalidate the cache,
because the underlying data can change.

[Bernhard Bauer, 2014/01/09 14:19:27]

We can still do that; just throw away the future.

[Adrian Kuegel, 2014/01/09 15:34:17]

From what I have read so far, it looks like it got renamed to Promise, and the
spec is still in development. So maybe we should wait a bit before it is
finalized, and then use it. I saw we have cr.Promise available, but that seems
to be a bit different from what the spec currently says. Anyway, it will
definitely be useful and simplifies the code.

[Bernhard Bauer, 2014/01/09 16:25:11]

Promises are available from Chrome 32 on, according to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

+   */
+  ManagedUserListData.requestExistingManagedUsers = function(callback,
+                                                             errback) {
+    var instance = ManagedUserListData.getInstance();

[Bernhard Bauer, 2014/01/09 13:39:08]

The usual way to do this is to make this a private instance method, and forward
to it from the (public) static method.

[Adrian Kuegel, 2014/01/09 14:12:42]

Ok, I didn't know this is a required pattern. I can change it.

[Bernhard Bauer, 2014/01/09 14:19:27]

It's not required, but this is how it is usually done, and there is something to
be said for consistency. It also avoids having to get the |instance| every time
(and reaching into it to call private methods; calling them on |this| instead
reads nicer to me).

+    instance.callbacks_.push(callback);
+    instance.errbacks_.push(errback);
+    if (!instance.requestInProgress_) {
+      if (instance.managedUsers_ != null) {
+        ManagedUserListData.receiveExistingManagedUsers(instance.managedUsers_);

[Bernhard Bauer, 2014/01/09 13:39:08]

Note that this will not only call |callback|, but also any already existing
callbacks on |instance|. Couldn't you just directly call |callback|?

Hm... Can we even have more than one callback/errback at any point? If not, why
don't we just store them directly on the instance?

[Adrian Kuegel, 2014/01/09 14:12:42]

I think that if |requestInProgress_| is false, the list of callbacks is empty.
So this will essentially just call |callback|. But you are right, I could just
directly call |callback|, and this would be easier to read.
Currently I think there can be more than one callback/errback, but I think it
could be avoided. I need to make sure not to add another request call in the
onNameChanged_() method if the previous request was not handled. Also I possibly
need a way to remove the callback if the corresponding overlay was closed,
because then I don't care about the result anymore (but I could then have opened
the other overlay that adds a new callback).
Anyway, before I go forward changing this I want to read up on DOM futures,
maybe this will solve the problem anyway.

+        return;
+      }
+      instance.requestInProgress_ = true;
+      chrome.send('requestManagedUserImportUpdate');
+    }
+  };
+
+  /**
+   * 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.
+   */
+  ManagedUserListData.reloadExistingManagedUsers = function() {
+    var instance = ManagedUserListData.getInstance();
+    if (instance.requestInProgress_)
+      return;
+
+    instance.managedUsers_ = null;
+    instance.requestInProgress_ = true;
+    chrome.send('requestManagedUserImportUpdate');
+  };
+
+  // Export
+  return {
+    ManagedUserListData: ManagedUserListData,
+  };
+});