Remember and restore the account last used destination is registered for.

chrome/browser/resources/print_preview/cloud_print_interface.js

Index: chrome/browser/resources/print_preview/cloud_print_interface.js
diff --git a/chrome/browser/resources/print_preview/cloud_print_interface.js b/chrome/browser/resources/print_preview/cloud_print_interface.js
index 71906609e34f38e0294056e4763df5003bd7bf08..af93ddd271641868486aaec13f84e937cf13b70f 100644
--- a/chrome/browser/resources/print_preview/cloud_print_interface.js
+++ b/chrome/browser/resources/print_preview/cloud_print_interface.js
@@ -47,11 +47,12 @@ cr.define('cloudprint', function() {
     this.userSessionIndex_ = {};
 
     /**
-     * Last received XSRF token. Sent as a parameter in every request.
-     * @type {string}
+     * Stores last received XSRF tokens for each user account. Sent as
+     * a parameter with every request.
+     * @type {!Object.<string, string>}
      * @private
      */
-    this.xsrfToken_ = '';
+    this.xsrfTokens_ = {};
 
     /**
      * Pending requests delayed until we get access token.
@@ -175,15 +176,7 @@ cr.define('cloudprint', function() {
     search: function(opt_origin) {
       var origins =
           opt_origin && [opt_origin] || CloudPrintInterface.CLOUD_ORIGINS_;
-      // Terminate outstanding search requests for all requested origins.
-      this.outstandingCloudSearchRequests_ =
-          this.outstandingCloudSearchRequests_.filter(function(request) {
-            if (origins.indexOf(request.origin) >= 0) {
-              request.xhr.abort();
-              return false;
-            }
-            return true;
-          });
+      this.abortSearchRequests_(origins);
       this.search_(true, origins);
       this.search_(false, origins);
     },
@@ -206,9 +199,13 @@ cr.define('cloudprint', function() {
         params.push(new HttpParam('q', '^recent'));
       }
       origins.forEach(function(origin) {
-        var cpRequest =
-            this.buildRequest_('GET', 'search', params, origin,
-                               this.onSearchDone_.bind(this, isRecent));
+        var cpRequest = this.buildRequest_(
+            'GET',
+            'search',
+            params,
+            origin,
+            this.userInfo_.activeUser,

[Toscano, 2014/04/11 04:24:00]

Hmm, seems like a hidden contract that the search function always searches for
the active user. I'm not sure it makes sense for cloud print interface to have a
dependency on "active user". Why not have the caller decide who's destinations
to search for, just like it already decides which origin to use. Also, using the
active user here makes it inconsistent with the "printer" method.

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

Done.

+            this.onSearchDone_.bind(this, isRecent));
         this.outstandingCloudSearchRequests_.push(cpRequest);
         this.sendOrQueueRequest_(cpRequest);
       }, this);
@@ -241,9 +238,13 @@ cr.define('cloudprint', function() {
                       '__google__chrome_version=' + chromeVersion),
         new HttpParam('tag', '__google__os=' + navigator.platform)
       ];
-      var cpRequest = this.buildRequest_('POST', 'submit', params,
-                                         destination.origin,
-                                         this.onSubmitDone_.bind(this));
+      var cpRequest = this.buildRequest_(
+          'POST',
+          'submit',
+          params,
+          destination.origin,
+          destination.account,
+          this.onSubmitDone_.bind(this));
       this.sendOrQueueRequest_(cpRequest);
     },
 
@@ -251,36 +252,46 @@ cr.define('cloudprint', function() {
      * Sends a Google Cloud Print printer API request.
      * @param {string} printerId ID of the printer to lookup.
      * @param {!print_preview.Destination.Origin} origin Origin of the printer.
+     * @param {string=} account Account this printer is registered for. When
+     *     provided for COOKIES {@code origin}, and users sessions are still not
+     *     known, will be checked against the response (both success and failure
+     *     to get printer) and, if the active user account is not the one
+     *     requested, {@code account} is activated and printer request reissued.
      */
-    printer: function(printerId, origin) {
+    printer: function(printerId, origin, account) {
       var params = [
         new HttpParam('printerid', printerId),
         new HttpParam('use_cdd', 'true'),
         new HttpParam('printer_connection_status', 'true')
       ];
-      var cpRequest =
-          this.buildRequest_('GET', 'printer', params, origin,
-                             this.onPrinterDone_.bind(this, printerId));
-      this.sendOrQueueRequest_(cpRequest);
+      this.sendOrQueueRequest_(this.buildRequest_(
+          'GET',
+          'printer',
+          params,
+          origin,
+          account,
+          this.onPrinterDone_.bind(this, printerId)));
     },
 
     /**
      * Sends a Google Cloud Print update API request to accept (or reject) the
      * terms-of-service of the given printer.
-     * @param {string} printerId ID of the printer to accept the
-     *     terms-of-service for.
-     * @param {!print_preview.Destination.Origin} origin Origin of the printer.
-     * @param {boolean} isAccepted Whether the user accepted the
-     *     terms-of-service.
+     * @param {!print_preview.Destination} destination Destination to accept ToS
+     *     for.
+     * @param {boolean} isAccepted Whether the user accepted ToS or not.
      */
-    updatePrinterTosAcceptance: function(printerId, origin, isAccepted) {
+    updatePrinterTosAcceptance: function(destination, isAccepted) {
       var params = [
-        new HttpParam('printerid', printerId),
+        new HttpParam('printerid', destination.id),
         new HttpParam('is_tos_accepted', isAccepted)
       ];
-      var cpRequest =
-          this.buildRequest_('POST', 'update', params, origin,
-                             this.onUpdatePrinterTosAcceptanceDone_.bind(this));
+      var cpRequest = this.buildRequest_(
+          'POST',
+          'update',
+          params,

[Vitaly Buka (NO REVIEWS), 2014/04/11 02:32:57]

inconsistent

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

Done.

+          destination.origin,
+          destination.account,
+          this.onUpdatePrinterTosAcceptanceDone_.bind(this));
       this.sendOrQueueRequest_(cpRequest);
     },
 
@@ -293,6 +304,10 @@ cr.define('cloudprint', function() {
           this.nativeLayer_,
           print_preview.NativeLayer.EventType.ACCESS_TOKEN_READY,
           this.onAccessTokenReady_.bind(this));
+      this.tracker_.add(
+          this.userInfo_,
+          print_preview.UserInfo.EventType.ACTIVE_USER_CHANGED,
+          this.onActiveUserChanged_.bind(this));
     },
 
     /**
@@ -302,25 +317,28 @@ cr.define('cloudprint', function() {
      * @param {Array.<!HttpParam>} params HTTP parameters to include in the
      *     request.
      * @param {!print_preview.Destination.Origin} origin Origin for destination.
+     * @param {?string} account Account the request is sent for. Can be
+     *     {@code null} or empty string if the request is not cookie bound or
+     *     is sent on behalf of the primary user.
      * @param {function(number, Object, !print_preview.Destination.Origin)}
      *     callback Callback to invoke when request completes.
      * @return {!CloudPrintRequest} Partially prepared request.
      * @private
      */
-    buildRequest_: function(method, action, params, origin, callback) {
+    buildRequest_: function(method, action, params, origin, account, callback) {
       var url = this.baseUrl_ + '/' + action + '?xsrf=';
       if (origin == print_preview.Destination.Origin.COOKIES) {
-        if (!this.xsrfToken_) {
+        var xsrfToken = this.xsrfTokens_[account];
+        if (!xsrfToken) {
           // TODO(rltoscano): Should throw an error if not a read-only action or
           // issue an xsrf token request.
         } else {
-          url = url + this.xsrfToken_;
+          url = url + xsrfToken;
         }
-        params = params || [];
-        if (this.userInfo_.activeUser) {
-          var index = this.userSessionIndex_[this.userInfo_.activeUser] || 0;
+        if (account) {
+          var index = this.userSessionIndex_[account] || 0;
           if (index > 0) {
-            params.push(new HttpParam('user', index));
+            url += '&user=' + index;
           }
         }
       }
@@ -356,7 +374,7 @@ cr.define('cloudprint', function() {
         xhr.setRequestHeader(header, headers[header]);
       }
 
-      return new CloudPrintRequest(xhr, body, origin, callback);
+      return new CloudPrintRequest(xhr, body, origin, account, callback);
     },
 
     /**
@@ -407,6 +425,49 @@ cr.define('cloudprint', function() {
     },
 
     /**
+     * Updates user info and session index from the {@code request} response.
+     * @param {!CloudPrintRequest} request Request to extract user info from.
+     * @private
+     */
+    setUsers_: function(request) {
+      if (request.origin == print_preview.Destination.Origin.COOKIES) {
+        var activeUser = request.result['request']['user'];

[Toscano, 2014/04/11 04:24:00]

Maybe move this to right above line 440 since it's only used there?

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

Done.

+        var users = request.result['request']['users'] || [];
+        this.userSessionIndex_ = {};
+        for (var i = 0; i < users.length; i++) {
+          this.userSessionIndex_[users[i]] = i;
+        }
+        this.userInfo_.setUsers(activeUser, users);
+      }
+    },
+
+    /**
+     * Terminates search requests for requested {@code origins}.
+     * @param {!Array.<print_preview.Destination.Origin>} origins Origins
+     *     to terminate search requests for.
+     * @private
+     */
+    abortSearchRequests_: function(origins) {
+      this.outstandingCloudSearchRequests_ =
+          this.outstandingCloudSearchRequests_.filter(function(request) {
+            if (origins.indexOf(request.origin) >= 0) {
+              request.xhr.abort();
+              return false;
+            }
+            return true;
+          });
+    },
+
+    /**
+     * Called when active user account changes. Terminates all outstanding
+     * cookie based search requests.
+     * @private
+     */
+    onActiveUserChanged_: function() {
+      this.abortSearchRequests_([print_preview.Destination.Origin.COOKIES]);

[Toscano, 2014/04/11 04:24:00]

Hmm, wouldn't it be simpler to only abort if and only if another search request
was made? Seems like a coupling of userInfo active user change and search for
printers is unnecessary. For example, if there was another way to change the
active user that didn't require an immediate search request would have to deal
with this dependency.

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

The thing is, search request reply _is_ the source of information about the
current state of user sessions, so changing the user, we should make sure there
are no search requests in flight, otherwise, as soon as they are done, the
active user will be changed accordingly to the user info in the reply,
overriding our state with stale info (there's no guarantee in the order replies
arrive).

On 2014/04/11 04:24:00, Toscano wrote:

[Toscano, 2014/04/12 02:09:08]

But wait, how can you change the user if the first reply hasn't finished yet?
Are you talking about the case where recent search finishes, and then the user
switches, and then the longer search finishes? But in this case, why does the
longer search replace the set of user sessions?

On 2014/04/11 19:09:29, Aleksey Shlyapnikov wrote:

[Aleksey Shlyapnikov, 2014/04/12 03:44:03]

It's assumed that all search requests issued under the same active account
return the same set of sessions (which is not true in general, but close enough
for our purpose here). To the first part of your question, I was concerned about
printer caps request getting too long (the case when it switches active
account), but playing with it I realized that it's more user friendly to let
search requests finish in this case, so I agree, let's abort only when another
search request was made.

On 2014/04/12 02:09:08, Toscano wrote:

+    },
+
+    /**
      * Called when a native layer receives access token.
      * @param {Event} evt Contains the authentication type and access token.
      * @private
@@ -424,7 +485,7 @@ cr.define('cloudprint', function() {
                                        'Bearer ' + event.accessToken);
           this.sendRequest_(request);
         } else {  // No valid token.
-          // Without abort status does not exists.
+          // Without abort status does not exist.
           request.xhr.abort();
           request.callback(request);
         }
@@ -443,8 +504,9 @@ cr.define('cloudprint', function() {
         if (request.xhr.status == 200) {
           request.result = JSON.parse(request.xhr.responseText);
           if (request.origin == print_preview.Destination.Origin.COOKIES &&
-              request.result['success']) {
-            this.xsrfToken_ = request.result['xsrf_token'];
+              request.result['success'] &&
+              request.account) {
+            this.xsrfTokens_[request.account] = request.result['xsrf_token'];
           }
         }
         request.status = request.xhr.status;
@@ -460,15 +522,21 @@ cr.define('cloudprint', function() {
      * @private
      */
     onSearchDone_: function(isRecent, request) {
+      var lastRequestForThisOrigin = true;
       this.outstandingCloudSearchRequests_ =
           this.outstandingCloudSearchRequests_.filter(function(item) {
+            if (item != request && item.origin == request.origin) {
+              lastRequestForThisOrigin = false;
+            }
             return item != request;
           });
+      var activeUser = '';
+      if (request.origin == print_preview.Destination.Origin.COOKIES) {
+        activeUser = request.result['request']['user'];
+      }
+      var event = null;
       if (request.xhr.status == 200 && request.result['success']) {
-        var activeUser = '';
-        if (request.origin == print_preview.Destination.Origin.COOKIES) {
-          activeUser = request.result['request']['user'];
-        }
+        // Extract printers.
         var printerListJson = request.result['printers'] || [];
         var printerList = [];
         printerListJson.forEach(function(printerJson) {
@@ -479,25 +547,21 @@ cr.define('cloudprint', function() {
             console.error('Unable to parse cloud print destination: ' + err);
           }
         });
-        var searchDoneEvent =
-            new Event(CloudPrintInterface.EventType.SEARCH_DONE);
-        searchDoneEvent.printers = printerList;
-        searchDoneEvent.origin = request.origin;
-        searchDoneEvent.isRecent = isRecent;
-        if (request.origin == print_preview.Destination.Origin.COOKIES) {
-          var users = request.result['request']['users'] || [];
-          this.userSessionIndex_ = {};
-          for (var i = 0; i < users.length; i++) {
-            this.userSessionIndex_[users[i]] = i;
-          }
-          this.userInfo_.setUsers(activeUser, users);
-        }
-        this.dispatchEvent(searchDoneEvent);
+        // Extract users.
+        this.setUsers_(request);

[Vitaly Buka (NO REVIEWS), 2014/04/11 02:32:57]

setUsers_ -> extractUsers_

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

It updates userInfo_ state, not just extracts user info from the request.
Changed the comment to make it more explicit.

On 2014/04/11 02:32:57, Vitaly Buka wrote:

+        // Dispatch SEARCH_DONE event.
+        event = new Event(CloudPrintInterface.EventType.SEARCH_DONE);
+        event.origin = request.origin;
+        event.printers = printerList;
+        event.isRecent = isRecent;
       } else {
-        var errorEvent = this.createErrorEvent_(
-            CloudPrintInterface.EventType.SEARCH_FAILED, request);
-        this.dispatchEvent(errorEvent);
+        event = this.createErrorEvent_(
+            CloudPrintInterface.EventType.SEARCH_FAILED,
+            request);
       }
+      event.user = activeUser;
+      event.searchDone = lastRequestForThisOrigin;
+      this.dispatchEvent(event);
     },
 
     /**
@@ -525,6 +589,29 @@ cr.define('cloudprint', function() {
      * @private
      */
     onPrinterDone_: function(destinationId, request) {
+      // Special handling of the first printer request. It does not matter at
+      // this point, whether printer was found or not.
+      if (request.origin == print_preview.Destination.Origin.COOKIES &&
+          request.account &&
+          request.result['request']['user'] &&
+          request.result['request']['users'] &&
+          request.account != request.result['request']['user']) {
+        var users = request.result['request']['users'];

[Toscano, 2014/04/11 04:24:00]

Let's keep user parsing logic in place. Why not this.setUsers_(request) and then
use the userSessionIndex map below?

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

Done.

On 2014/04/11 04:24:00, Toscano wrote:

+        // In case the user account is known, but not the primary one,
+        // activate it.
+        if (users.indexOf(request.account) > 0) {
+          this.setUsers_(request);
+          this.userInfo_.activeUser = request.account;
+          // Repeat the request for the newly activated account.
+          this.printer(
+              request.result['request']['params']['printerid'],
+              request.origin,
+              request.account);
+          // Stop processing this request, wait for the new response.
+          return;
+        }
+      }
+      // Process response.
       if (request.xhr.status == 200 && request.result['success']) {
         var activeUser = '';
         if (request.origin == print_preview.Destination.Origin.COOKIES) {
@@ -574,11 +661,14 @@ cr.define('cloudprint', function() {
    * @param {!XMLHttpRequest} xhr Partially prepared http request.
    * @param {string} body Data to send with POST requests.
    * @param {!print_preview.Destination.Origin} origin Origin for destination.
+   * @param {?string} account Account the request is sent for. Can be
+   *     {@code null} or empty string if the request is not cookie bound or
+   *     is sent on behalf of the primary user.

[Toscano, 2014/04/11 04:24:00]

If it's nullable, maybe it should be opt_account?

[Aleksey Shlyapnikov, 2014/04/11 19:09:29]

I don't see the benefit of optional here, it's a private class.

On 2014/04/11 04:24:00, Toscano wrote:

    * @param {function(!CloudPrintRequest)} callback Callback to invoke when
    *     request completes.
    * @constructor
    */
-  function CloudPrintRequest(xhr, body, origin, callback) {
+  function CloudPrintRequest(xhr, body, origin, account, callback) {
     /**
      * Partially prepared http request.
      * @type {!XMLHttpRequest}
@@ -598,6 +688,12 @@ cr.define('cloudprint', function() {
     this.origin = origin;
 
     /**
+     * User account this request is expected to be executed for.
+     * @type {?string}
+     */
+    this.account = account;
+
+    /**
      * Callback to invoke when request completes.
      * @type {function(!CloudPrintRequest)}
      */