Added better error and OAuth support in xhr.js.

Added better error and OAuth support in xhr.js.

The Xhr class can now use the remoting.Identity API to request OAuth
tokens, and it can convert HTTP errors into rejected promises.

Also replaced the responseType parameter with the simpler acceptJson
parameter.

Committed: https://crrev.com/d81682810727f6ae3947f259e7f4400a9df4da76
Cr-Commit-Position: refs/heads/master@{#322307}

[John Williams, 2015-03-21 00:57:41]

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

[John Williams, 2015-03-21 00:57:41]

The diffs in xhr_unittests.js are misleading.  What I did was:

1. Removed redundant checks for a bunch several tests so the same behavior isn't
checked over and over.
2. Added a bunch of new tests.
3. Removed the tests for defaultHandler, which no longer exists.

The changes in sinon_proto.js are technically not needed, but they make the
declarations more correct and I think they're too small to be worth putting in a
separate CL.

[rmsousa, 2015-03-21 01:49:56]

drive-by pony request (feel free to ignore)

https://codereview.chromium.org/1028683004/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:286: QUnit.test('GET with param string',
function(assert) {
"Since you're here": could you add a unit test for a DELETE with a 204 No
Response empty response (should treat as successful)? We've had a bug with that
recently (mistakenly treating it as an unknown error).

[John Williams, 2015-03-23 19:55:10]

https://codereview.chromium.org/1028683004/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:286: QUnit.test('GET with param string',
function(assert) {
On 2015/03/21 01:49:56, rmsousa wrote:
> "Since you're here": could you add a unit test for a DELETE with a 204 No
> Response empty response (should treat as successful)? We've had a bug with
that
> recently (mistakenly treating it as an unknown error).

Done.

[Jamie, 2015-03-23 23:11:33]

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

[Jamie, 2015-03-23 23:11:34]

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:81: this.aborted_ = false;
Do you need to distinguish between this case and |deferred_| == null?

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:116: *     treated as an error.
This seems like an odd thing to put into a general-purpose API, and it's only
used by a couple of callers. I think it would be better to remove it and have
those sites handle the expected errors explicitly.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:141: * resolved or rejected.
Is that the right contract for this call? Generally, the call that returned the
original Promise is waiting for it to be fulfilled (at which point it may, for
example, remove a UI spinner). By stating that abort() bypasses this
fulfillment, any call to abort() needs to know what that code would have done
and duplicate it. Rejecting the Promise with CANCELLED would seem like a more
consistent contract.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:172: var self = this;
We've tended to use |that|, rather than |self| when we have to use this pattern.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:178: }, function(error) {
Please use the more explicit catch() method, rather than a second parameter to
then().

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:281: var error =
remoting.Error.fromHttpStatus(xhr.status);
I don't think that this is the right place to do this translation.
remoting.Error is essentially a set of user-facing error conditions used mostly
for translation purposes. It has a mapping from HTTP status, but that mapping is
somewhat open to interpretation by the call-site, and is lossy. I think it would
be better to handle 2xx as resolve and anything else as reject, but just pass
the status code to the caller and let them decide what to do with it, otherwise
you're throwing away information that might be useful.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:29: returns(Promise.resolve('my_token'));
You could alternatively mock chrome.identity (this already exists) and use the
real remoting.Identity wrapper.

[John Williams, 2015-03-24 00:52:12]

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:81: this.aborted_ = false;
On 2015/03/23 23:11:33, Jamie wrote:
> Do you need to distinguish between this case and |deferred_| == null?

Yes, when useIdentity is true.  Although I didn't realize until just now that,
in the one place where abort() is used, useIdentity is false.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:116: *     treated as an error.
On 2015/03/23 23:11:34, Jamie wrote:
> This seems like an odd thing to put into a general-purpose API, and it's only
> used by a couple of callers. I think it would be better to remove it and have
> those sites handle the expected errors explicitly.

Done.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:141: * resolved or rejected.
On 2015/03/23 23:11:34, Jamie wrote:
> Is that the right contract for this call? Generally, the call that returned
the
> original Promise is waiting for it to be fulfilled (at which point it may, for
> example, remove a UI spinner). By stating that abort() bypasses this
> fulfillment, any call to abort() needs to know what that code would have done
> and duplicate it. Rejecting the Promise with CANCELLED would seem like a more
> consistent contract.

So far, this method has exactly one call site, and changing the semantics as you
suggest would either complicate the call site or cause a spurious error message
to be logged.

Basically I see this method as a necessary evil, so I'm not keen on adding more
features to it.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:172: var self = this;
On 2015/03/23 23:11:33, Jamie wrote:
> We've tended to use |that|, rather than |self| when we have to use this
pattern.

Done, but I'm not a big fan because in ordinary usage, "this" and "that"
necessarily refer to different things.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:178: }, function(error) {
On 2015/03/23 23:11:33, Jamie wrote:
> Please use the more explicit catch() method, rather than a second parameter to
> then().

OK.  It makes no difference here but I should point out that this is a semantic
change.  With P.then(A).catch(B), any errors generate by A will be handled by B,
but with P.then(A,B), B is only run when P itself is rejected, and it never sees
any errors that might be generated by A.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:281: var error =
remoting.Error.fromHttpStatus(xhr.status);
On 2015/03/23 23:11:34, Jamie wrote:
> I don't think that this is the right place to do this translation.
> remoting.Error is essentially a set of user-facing error conditions used
mostly
> for translation purposes. It has a mapping from HTTP status, but that mapping
is
> somewhat open to interpretation by the call-site, and is lossy. I think it
would
> be better to handle 2xx as resolve and anything else as reject, but just pass
> the status code to the caller and let them decide what to do with it,
otherwise
> you're throwing away information that might be useful.

I'm not sure I agree, but I'll compromise and just remove the error handling
entirely since there aren't many places that use it.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:29: returns(Promise.resolve('my_token'));
On 2015/03/23 23:11:34, Jamie wrote:
> You could alternatively mock chrome.identity (this already exists) and use the
> real remoting.Identity wrapper.

This way seems a lot easier, since it doesn't involve messing with callbacks.

[John Williams, 2015-03-24 17:27:36]

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:29: returns(Promise.resolve('my_token'));
On 2015/03/24 00:52:12, John Williams wrote:
> On 2015/03/23 23:11:34, Jamie wrote:
> > You could alternatively mock chrome.identity (this already exists) and use
the
> > real remoting.Identity wrapper.
> 
> This way seems a lot easier, since it doesn't involve messing with callbacks.

After taking a second look, I see I was misunderstanding what you meant, but I'm
not sure it's worth making the change.  It works out to about the same amount of
code, and all things being equal, I generally prefer code that depends on a
general-purpose library like sinon over code that depends on project-specific
utilities.

[Jamie, 2015-03-24 19:44:11]

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:81: this.aborted_ = false;
On 2015/03/24 00:52:12, John Williams wrote:
> On 2015/03/23 23:11:33, Jamie wrote:
> > Do you need to distinguish between this case and |deferred_| == null?
> 
> Yes, when useIdentity is true.  Although I didn't realize until just now that,
> in the one place where abort() is used, useIdentity is false.

I don't understand why you need it when useIdentity is true. deferred_ is set in
start() and if abort() is intended to prevent the corresponding promise from
being fulfilled, it seems like it should null it. That said, I'm hopeful that
abort() can be removed altogether (see below).

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:141: * resolved or rejected.
On 2015/03/24 00:52:12, John Williams wrote:
> On 2015/03/23 23:11:34, Jamie wrote:
> > Is that the right contract for this call? Generally, the call that returned
> the
> > original Promise is waiting for it to be fulfilled (at which point it may,
for
> > example, remove a UI spinner). By stating that abort() bypasses this
> > fulfillment, any call to abort() needs to know what that code would have
done
> > and duplicate it. Rejecting the Promise with CANCELLED would seem like a
more
> > consistent contract.
> 
> So far, this method has exactly one call site, and changing the semantics as
you
> suggest would either complicate the call site or cause a spurious error
message
> to be logged.
> 
> Basically I see this method as a necessary evil, so I'm not keen on adding
more
> features to it.

What's the call-site? I don't see it in the diffs, but that may simply be
because the new code is API-compatible with the old. Could we remove it and
simply ignore the result? Something like abort() is inherently racy, so the
caller should not be relying on it for cancelling a request before it takes
effect anyway.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:178: }, function(error) {
On 2015/03/24 00:52:12, John Williams wrote:
> On 2015/03/23 23:11:33, Jamie wrote:
> > Please use the more explicit catch() method, rather than a second parameter
to
> > then().
> 
> OK.  It makes no difference here but I should point out that this is a
semantic
> change.  With P.then(A).catch(B), any errors generate by A will be handled by
B,
> but with P.then(A,B), B is only run when P itself is rejected, and it never
sees
> any errors that might be generated by A.

Thanks for the clarification. I wasn't aware of that nuance.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:29: returns(Promise.resolve('my_token'));
On 2015/03/24 17:27:36, John Williams wrote:
> On 2015/03/24 00:52:12, John Williams wrote:
> > On 2015/03/23 23:11:34, Jamie wrote:
> > > You could alternatively mock chrome.identity (this already exists) and use
> the
> > > real remoting.Identity wrapper.
> > 
> > This way seems a lot easier, since it doesn't involve messing with
callbacks.
> 
> After taking a second look, I see I was misunderstanding what you meant, but
I'm
> not sure it's worth making the change.  It works out to about the same amount
of
> code, and all things being equal, I generally prefer code that depends on a
> general-purpose library like sinon over code that depends on project-specific
> utilities.

The problem is that this only mocks one method of remoting.identity (albeit the
only one we're using for this API right now). The advantage of activating the
chrome.identity mock is that it's designed to work with remoting.Identity more
broadly. It's also used by other unit tests, so it's more consistent. And as you
say, it's the same amount of code:

  chromeMocks.activate(['identity']);
  chromeMocks.identity.mock$setToken('my_token');

https://codereview.chromium.org/1028683004/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/host_list_api_impl.js (right):

https://codereview.chromium.org/1028683004/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/host_list_api_impl.js:72:
remoting.identity.getToken().then(onToken, remoting.Error.handler(onError));
Why have you reverted the useIdentity change as well?

[John Williams, 2015-03-24 23:34:41]

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:81: this.aborted_ = false;
On 2015/03/24 19:44:11, Jamie wrote:
> On 2015/03/24 00:52:12, John Williams wrote:
> > On 2015/03/23 23:11:33, Jamie wrote:
> > > Do you need to distinguish between this case and |deferred_| == null?
> > 
> > Yes, when useIdentity is true.  Although I didn't realize until just now
that,
> > in the one place where abort() is used, useIdentity is false.
> 
> I don't understand why you need it when useIdentity is true. deferred_ is set
in
> start() and if abort() is intended to prevent the corresponding promise from
> being fulfilled, it seems like it should null it. That said, I'm hopeful that
> abort() can be removed altogether (see below).

It's moot since I got rid of the method, but be case I'm trying to avoid looks
like this:

1. User calls start.
2. A token is requested from the identity API.
3. User calls abort().
4. The native XHR's abort() method is called, resulting in an exception, which
is caught and ignored.
5. The identity API returns a token.
6. The native XHR is sent, even though it should have been aborted.

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:141: * resolved or rejected.
On 2015/03/24 19:44:11, Jamie wrote:
> On 2015/03/24 00:52:12, John Williams wrote:
> > On 2015/03/23 23:11:34, Jamie wrote:
> > > Is that the right contract for this call? Generally, the call that
returned
> > the
> > > original Promise is waiting for it to be fulfilled (at which point it may,
> for
> > > example, remove a UI spinner). By stating that abort() bypasses this
> > > fulfillment, any call to abort() needs to know what that code would have
> done
> > > and duplicate it. Rejecting the Promise with CANCELLED would seem like a
> more
> > > consistent contract.
> > 
> > So far, this method has exactly one call site, and changing the semantics as
> you
> > suggest would either complicate the call site or cause a spurious error
> message
> > to be logged.
> > 
> > Basically I see this method as a necessary evil, so I'm not keen on adding
> more
> > features to it.
> 
> What's the call-site? I don't see it in the diffs, but that may simply be
> because the new code is API-compatible with the old. Could we remove it and
> simply ignore the result? Something like abort() is inherently racy, so the
> caller should not be relying on it for cancelling a request before it takes
> effect anyway.

It was API compatible, but you should see it now that I removed it :)

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xhr_unittest.js (right):

https://codereview.chromium.org/1028683004/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr_unittest.js:29: returns(Promise.resolve('my_token'));
On 2015/03/24 19:44:11, Jamie wrote:
> On 2015/03/24 17:27:36, John Williams wrote:
> > On 2015/03/24 00:52:12, John Williams wrote:
> > > On 2015/03/23 23:11:34, Jamie wrote:
> > > > You could alternatively mock chrome.identity (this already exists) and
use
> > the
> > > > real remoting.Identity wrapper.
> > > 
> > > This way seems a lot easier, since it doesn't involve messing with
> callbacks.
> > 
> > After taking a second look, I see I was misunderstanding what you meant, but
> I'm
> > not sure it's worth making the change.  It works out to about the same
amount
> of
> > code, and all things being equal, I generally prefer code that depends on a
> > general-purpose library like sinon over code that depends on
project-specific
> > utilities.
> 
> The problem is that this only mocks one method of remoting.identity (albeit
the
> only one we're using for this API right now). The advantage of activating the
> chrome.identity mock is that it's designed to work with remoting.Identity more
> broadly. It's also used by other unit tests, so it's more consistent. And as
you
> say, it's the same amount of code:
> 
>   chromeMocks.activate(['identity']);
>   chromeMocks.identity.mock$setToken('my_token');

Done.

https://codereview.chromium.org/1028683004/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/host_list_api_impl.js (right):

https://codereview.chromium.org/1028683004/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/host_list_api_impl.js:72:
remoting.identity.getToken().then(onToken, remoting.Error.handler(onError));
On 2015/03/24 19:44:11, Jamie wrote:
> Why have you reverted the useIdentity change as well?

Accident.

[Jamie, 2015-03-25 21:25:27]

lgtm

[John Williams, 2015-03-25 22:25:59]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-03-25 22:27:08]

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

[commit-bot: I haz the power, 2015-03-26 03:45:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-26 03:46:45]

Patchset 8 (id:??) landed as
https://crrev.com/d81682810727f6ae3947f259e7f4400a9df4da76
Cr-Commit-Position: refs/heads/master@{#322307}