Handle authentication failures in the v2 app by restarting the app

Handle authentication failures in the v2 app by restarting the app

This CL handles the case when a token is revoked, e.g. via
https://security.google.com/settings/security/permissions.

We need to instruct chrome.identity to remove the invalid cached
token and fetch a new one.  The app would then be reloaded app as
some of the app components (wcs sandbox) cached the invalid token
and cannot be cleanly reinitialized.

Summary of changes:
1. Implement app reloading using chrome.app.AppWindow API's and
   base.Ipc so that it can be called from foreground pages
2. Implement handleAuthFailure in remoting.identity and remoting.oauth2.
   In remoting.identity, we would remove the cached token and request a
   new one by prompting the user for consent.
3. Modify the call sites to use the new method.

BUG=339677
Committed: https://crrev.com/2589f69a7fb7be7586f075af0b9029517543ed31
Cr-Commit-Position: refs/heads/master@{#313807}

[kelvinp, 2015-01-23 21:52:09]

kelvinp@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[kelvinp, 2015-01-23 22:04:36]

Patchset #1 (id:1) has been deleted

[kelvinp, 2015-01-23 22:25:07]

PTAL

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/activation_handler.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/activation_handler.js:6: var remoting = remoting || {};
This file is existing code pulled out from background.js reparented to the
remoting namespace.

[kelvinp, 2015-01-23 23:23:16]

kelvinp@chromium.org changed reviewers:
+ garykac@chromium.org

[kelvinp, 2015-01-23 23:23:16]

Adding Gary for FYI

[Jamie, 2015-01-23 23:33:27]

I think it's worth splitting this into a CL that introduces the new IPC class
and then a follow-up that uses it to fix the bug. The former is pretty much
good-to-go modulo some nits (and unit-tests ^_^), and it will be easier to focus
on the details of the latter if it can be reviewed in isolation.

https://codereview.chromium.org/868203002/diff/20001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:63: 'webapp/base/js/remote_methods.js',
Communicating between the main and background pages is not really "remote" (at
least, not in the context of a remote access application). I suggest calling
this something like ipc.js.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
File remoting/webapp/base/js/remote_methods.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:30: *    
base.remoteMethods.invoke('my.service.name', [arg1, arg2]).then(
Would it be a lot of work to allow args to be passed without wrapping them in an
array? It seems a bit more Javascripty.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:51: base.RemoteMethods = function() {
I suggest base.Ipc as a name instead.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:51: base.RemoteMethods = function() {
With a suitable chrome.runtime.sendMessage mock, I think this class could be
unit-tested very easily. If you do that, you might want to consider pulling this
out into a separate CL.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:68: base.RemoteMethods.Request_ =
function(requestType, params) {
If this is a private type, does it need to be exposed on base.RemoteMethods?

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:78: * @param {string} requestType
Since we're talking about methods, I think methodName would be a more suitable
parameter name. A brief description for the first two parameters would also be
useful, particularly explaining how the method parameters are passed to the
handler.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:79: * @param {Function} handler
s/Function/function(*):void/

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:118: sendResponse({error :
'Unsupported request:= ' + requestType});
Nit: s/:=/:/

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:130: * Invokes a method on a remote
page
Blank (comment) line after method description.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:132: * @param {Array} params
@return?

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:137: [null, new
base.RemoteMethods.Request_(requestType, params)]);
I don't think you need the leading null, since the docs say the extensionId is
optional.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:145: Promise.resolve(response);
Don't these reject/resolves need to be returned?

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/remote_methods.js:150: })();
Although we have too many globals in our code, I think this is a good example of
a class that should be a singleton, since the duplicate-checking in register
needs to be done globally. I suggest you create an instance here.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/activation_handler.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/activation_handler.js:6: var remoting = remoting || {};
On 2015/01/23 22:25:07, kelvinp wrote:
> This file is existing code pulled out from background.js reparented to the
> remoting namespace.

Was that done because background.js was getting too big, or because it's needed
in remoting.js as well? The gyp changes suggest the latter, but if so , then the
code to handle new window should not be here. Do you need it just for access to
the IPC names? If so, then I wonder if a separate js file containing shared
constants is the way to go?

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/app_launcher.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/app_launcher.js:103: 'restart() can only be called from
the background page');
It would be better if this code file was simply not included in the main page.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/app_launcher.js:105: return
chrome.app.window.create(APP_MAIN_URL, {'id' : id, 'frame': 'none'});
Please add a comment explaining why we're not re-using the launch method (I
think it's just because we want to reuse the window id, size etc).

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_event_handlers.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_event_handlers.js:90: { event: 'click', id:
'auth-button', fn: handleAuthFailure },
I'm not sure this will work. I think "auth-button" is also used to grant consent
for chrome.identity (which was fine with the old code, because it's conditional
on !appsv2). Now, first-time sign-in is likely to be broken because as well as
asking the user to authenticate, we'll relaunch the app.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:283:
remoting.setMode(remoting.AppMode.HOME);
We should still show an error in this case. Would it be substantially more
complex to do that? I don't want to waste a lot of time polishing this UX, but
relaunching without telling the user you're going to do so is jarring

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:93: var revokeToken =
base.Promise.as(this.removeCachedAuthToken_, []);
Using Promise.as to Promisify an API we own is a little odd. We should instead
just make that API use promises natively. That's out of scope for this CL, but
it would be more consistent if you used callbacks for this new API. I think it
would be more readable too--I'm having trouble working out exactly what's going
on here.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:132:
remoting.Identity.prototype.removeCachedAuthToken_ = function(onDone) {
Underscore and @private should go hand-in-hand. Please either remove the
underscore or add @private.

[kelvinp, 2015-01-27 21:34:07]

Patchset #2 (id:40001) has been deleted

[kelvinp, 2015-01-28 00:26:46]

PTAL

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/activation_handler.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/activation_handler.js:6: var remoting = remoting || {};
On 2015/01/23 23:33:26, Jamie wrote:
> On 2015/01/23 22:25:07, kelvinp wrote:
> > This file is existing code pulled out from background.js reparented to the
> > remoting namespace.
> 
> Was that done because background.js was getting too big, or because it's
needed
> in remoting.js as well? The gyp changes suggest the latter, but if so , then
the
> code to handle new window should not be here. Do you need it just for access
to
> the IPC names? If so, then I wonder if a separate js file containing shared
> constants is the way to go?

Ya, mainly for the IPC names, but I also think it is cleaner to keep
background.js small.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/app_launcher.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/app_launcher.js:103: 'restart() can only be called from
the background page');
On 2015/01/23 23:33:26, Jamie wrote:
> It would be better if this code file was simply not included in the main page.

Done.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/app_launcher.js:105: return
chrome.app.window.create(APP_MAIN_URL, {'id' : id, 'frame': 'none'});
On 2015/01/23 23:33:26, Jamie wrote:
> Please add a comment explaining why we're not re-using the launch method (I
> think it's just because we want to reuse the window id, size etc).

Done.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_event_handlers.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_event_handlers.js:90: { event: 'click', id:
'auth-button', fn: handleAuthFailure },
On 2015/01/23 23:33:26, Jamie wrote:
> I'm not sure this will work. I think "auth-button" is also used to grant
consent
> for chrome.identity (which was fine with the old code, because it's
conditional
> on !appsv2). Now, first-time sign-in is likely to be broken because as well as
> asking the user to authenticate, we'll relaunch the app.

Done.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:283:
remoting.setMode(remoting.AppMode.HOME);
On 2015/01/23 23:33:26, Jamie wrote:
> We should still show an error in this case. Would it be substantially more
> complex to do that? I don't want to waste a lot of time polishing this UX, but
> relaunching without telling the user you're going to do so is jarring

Ya, it would be some work to show the error cleanly, as the reconnect dialog is
hardcoded to reconnect (which doesn't fix any auth problem).  As discussed, I
think this would be a very rare scenario.  I would prefer to leave the code as
it is.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:93: var revokeToken =
base.Promise.as(this.removeCachedAuthToken_, []);
On 2015/01/23 23:33:27, Jamie wrote:
> Using Promise.as to Promisify an API we own is a little odd. We should instead
> just make that API use promises natively. That's out of scope for this CL, but
> it would be more consistent if you used callbacks for this new API. I think it
> would be more readable too--I'm having trouble working out exactly what's
going
> on here.

Agreed. I have converted the private API (removeCachedToken_) to use promise as
well.  The promise here is nice because it guarantees that if a token revoke is
in progress, we won't try to invoke another revoke.  If I use callbacks, I would
need to create an array of token revoke callbacks and notify each one of them,
when they are done. I also add more comments to explain each step.  Let me know
if there are other ways that I can clarify it better.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:132:
remoting.Identity.prototype.removeCachedAuthToken_ = function(onDone) {
On 2015/01/23 23:33:27, Jamie wrote:
> Underscore and @private should go hand-in-hand. Please either remove the
> underscore or add @private.

Done.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:120:
base.IPC.invoke(remoting.ActivationHandler.IPC.RELAUNCH,
Will be fixed later after I address your feedback from the first CL.

[Jamie, 2015-01-28 20:52:09]

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/activation_handler.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/activation_handler.js:6: var remoting = remoting || {};
On 2015/01/28 00:26:45, kelvinp wrote:
> On 2015/01/23 23:33:26, Jamie wrote:
> > On 2015/01/23 22:25:07, kelvinp wrote:
> > > This file is existing code pulled out from background.js reparented to the
> > > remoting namespace.
> > 
> > Was that done because background.js was getting too big, or because it's
> needed
> > in remoting.js as well? The gyp changes suggest the latter, but if so , then
> the
> > code to handle new window should not be here. Do you need it just for access
> to
> > the IPC names? If so, then I wonder if a separate js file containing shared
> > constants is the way to go?
> 
> Ya, mainly for the IPC names, but I also think it is cleaner to keep
> background.js small.

Refactoring this code is fine, but I'm a bit concerned that it means that the
main page now includes code to create context menu items and other tasks that
only make sense from the background page. TBH, I feel that duplicating the
method name might be the lesser of two evils here.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:29: remoting.Identity =
function(consentCallback) {
As discussed, I don't think we need any changes to this API. The consent
callback should remain, because this is fairly low-level code that should not be
tied to any particular UX. Similarly, relaunching the app after revoking the
token should not be the responsibility of this class--instead, just call
removeCachedAuthToken and relaunch in the callback.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/auth_dialog.js (right):

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:31: this.borderElement_ =
rootElement.querySelector('#auth-dialog-border');
In general, I like this model of using querySelector to find sub-elements, but
it doesn't really make sense if those elements are referred to by id. Can you
change these to use classes instead of ids? It can be a follow-up CL if you
prefer.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:40: * @type {base.Deferred}
Unrelated to this CL: Whenever I see base.Deferred used, I have to remind myself
what it does. I think the problem is that the name is too generic. Maybe
something more like base.Promise.Deferred would be better (though it still
doesn't really encapsulate what it does).

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:65: this.onAuthButtonDeferred_.resolve();
Maybe assert that the deferred object is non-null (and, correspondingly, that it
is null when you create it in show).

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:92: remoting.AuthDialog.show = function()
{
Having both a static and a non-static show() method is a little confusing. Can
the non-static one be made private?

[kelvinp, 2015-01-29 01:05:35]

Thank you very much for your feedback.
I like the code much better now.
PTAL

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/activation_handler.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/activation_handler.js:6: var remoting = remoting || {};
On 2015/01/28 20:52:09, Jamie wrote:
> On 2015/01/28 00:26:45, kelvinp wrote:
> > On 2015/01/23 23:33:26, Jamie wrote:
> > > On 2015/01/23 22:25:07, kelvinp wrote:
> > > > This file is existing code pulled out from background.js reparented to
the
> > > > remoting namespace.
> > > 
> > > Was that done because background.js was getting too big, or because it's
> > needed
> > > in remoting.js as well? The gyp changes suggest the latter, but if so ,
then
> > the
> > > code to handle new window should not be here. Do you need it just for
access
> > to
> > > the IPC names? If so, then I wonder if a separate js file containing
shared
> > > constants is the way to go?
> > 
> > Ya, mainly for the IPC names, but I also think it is cleaner to keep
> > background.js small.
> 
> Refactoring this code is fine, but I'm a bit concerned that it means that the
> main page now includes code to create context menu items and other tasks that
> only make sense from the background page. TBH, I feel that duplicating the
> method name might be the lesser of two evils here.

Done.

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:29: remoting.Identity =
function(consentCallback) {
On 2015/01/28 20:52:09, Jamie wrote:
> As discussed, I don't think we need any changes to this API. The consent
> callback should remain, because this is fairly low-level code that should not
be
> tied to any particular UX. Similarly, relaunching the app after revoking the
> token should not be the responsibility of this class--instead, just call
> removeCachedAuthToken and relaunch in the callback.

Done.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/auth_dialog.js (right):

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:31: this.borderElement_ =
rootElement.querySelector('#auth-dialog-border');
On 2015/01/28 20:52:09, Jamie wrote:
> In general, I like this model of using querySelector to find sub-elements, but
> it doesn't really make sense if those elements are referred to by id. Can you
> change these to use classes instead of ids? It can be a follow-up CL if you
> prefer.

That's exactly my motivation of the change.  I tried but that would require
fixing a few browser tests as that's how we find the auth button.  I would do a
follow-up cl instead.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:40: * @type {base.Deferred}
On 2015/01/28 20:52:09, Jamie wrote:
> Unrelated to this CL: Whenever I see base.Deferred used, I have to remind
myself
> what it does. I think the problem is that the name is too generic. Maybe
> something more like base.Promise.Deferred would be better (though it still
> doesn't really encapsulate what it does).

Acknowledged. Deferred is actually a standard name for such operations and it is
used in many open source frameworks (JQuery, angular, etc)
http://api.jquery.com/category/deferred-object/
https://docs.angularjs.org/api/ng/service/$q

I realized that chrome has a native implementation of deferred recently.  You
can do Promise.defer() in the console.  However, it is marked as obsolete for
Mozilla 
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Prom...
and that's the only reason why I haven't switched over to it yet in case chrome
may follow suit later.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:65: this.onAuthButtonDeferred_.resolve();
On 2015/01/28 20:52:09, Jamie wrote:
> Maybe assert that the deferred object is non-null (and, correspondingly, that
it
> is null when you create it in show).

Done.  I will assert null in show().  However, I don't think I need to assert
here as if it is null, resolve() will blow up anyways.

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:92: remoting.AuthDialog.show = function()
{
On 2015/01/28 20:52:09, Jamie wrote:
> Having both a static and a non-static show() method is a little confusing. Can
> the non-static one be made private?

Done.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/base/js/...
File remoting/webapp/base/js/auth_init.js (right):

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_init.js:34: var oauth2 = /** @type {*} */
(remoting.oauth2);
hack to cast remoting.oauth2 => remoting.identity

[Jamie, 2015-01-29 01:52:34]

LGTM!

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/auth_dialog.js (right):

https://codereview.chromium.org/868203002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_dialog.js:31: this.borderElement_ =
rootElement.querySelector('#auth-dialog-border');
On 2015/01/29 01:05:34, kelvinp wrote:
> On 2015/01/28 20:52:09, Jamie wrote:
> > In general, I like this model of using querySelector to find sub-elements,
but
> > it doesn't really make sense if those elements are referred to by id. Can
you
> > change these to use classes instead of ids? It can be a follow-up CL if you
> > prefer.
> 
> That's exactly my motivation of the change.  I tried but that would require
> fixing a few browser tests as that's how we find the auth button.  I would do
a
> follow-up cl instead.

Acknowledged.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_event_handlers.js (right):

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_event_handlers.js:42:
remoting.handleAuthFailureAndRelaunch();
Now that this is no longer a member function (no |this|) there's no need to
define the wrapper class--you can just pass
remoting.handleAuthFailureAndRelaunch directly.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:102:
remoting.Identity.prototype.removeCachedAuthToken = function(onDone) {
For methods where it's reasonable not to care about the callback, onDone should
be optional (it will allow you to remove base.doNothing, below).

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:111: chrome.identity.getAuthToken({
'interactive': false }, onToken);
Nit: No space after "{" or before "}" for inline-objects (this is consistent
with the previous code, and with the code you've added below).

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:224:
this.consentDialog_.show().then(function() {
I really like this new Promise-based approach. As discussed, however, please
verify that {interactive: true} is expected to work if invoked asynchronously
(via Promise.resolve()) from a UI event handler.

[kelvinp, 2015-01-29 19:40:14]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-01-29 19:40:32]

FYI

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_event_handlers.js (right):

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_event_handlers.js:42:
remoting.handleAuthFailureAndRelaunch();
On 2015/01/29 01:52:34, Jamie wrote:
> Now that this is no longer a member function (no |this|) there's no need to
> define the wrapper class--you can just pass
> remoting.handleAuthFailureAndRelaunch directly.

Done.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:102:
remoting.Identity.prototype.removeCachedAuthToken = function(onDone) {
On 2015/01/29 01:52:34, Jamie wrote:
> For methods where it's reasonable not to care about the callback, onDone
should
> be optional (it will allow you to remove base.doNothing, below).

Done.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:111: chrome.identity.getAuthToken({
'interactive': false }, onToken);
On 2015/01/29 01:52:34, Jamie wrote:
> Nit: No space after "{" or before "}" for inline-objects (this is consistent
> with the previous code, and with the code you've added below).

Done.

https://codereview.chromium.org/868203002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:224:
this.consentDialog_.show().then(function() {
On 2015/01/29 01:52:34, Jamie wrote:
> I really like this new Promise-based approach. As discussed, however, please
> verify that {interactive: true} is expected to work if invoked asynchronously
> (via Promise.resolve()) from a UI event handler.

Courage replied.  Turns out the user interaction is merely a UX guideline rather
than a restriction. Done.

[commit-bot: I haz the power, 2015-01-29 19:41:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/868203002/100001

[commit-bot: I haz the power, 2015-01-29 19:44:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-29 19:44:26]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kelvinp, 2015-01-29 21:33:04]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-29 21:34:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/868203002/120001

[commit-bot: I haz the power, 2015-01-29 22:25:01]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-01-29 22:27:05]

Patchset 5 (id:??) landed as
https://crrev.com/2589f69a7fb7be7586f075af0b9029517543ed31
Cr-Commit-Position: refs/heads/master@{#313807}