Updated remoting.xhr API to use promises. Removed access to the native

Updated remoting.xhr API to use promises.

Removed access to the native XHR object used by the API.

This is a larger change than one might expect for two reasons: First,
because the native XHR object only allows the response content to be
retrieved while the onreadystatechange handler is executing, and
second, because the unit test for dns_blackhole_checker.js relied on
synchronous semantics which cannot be duplicated with promises because
when a promise is resolved, its "then" handlers are not called until
the next event cycle.

Committed: https://crrev.com/5b7c9e08d4e50f577eb7e058c21ef2f897cfbef1
Cr-Commit-Position: refs/heads/master@{#321608}

[John Williams, 2015-03-11 18:24:34]

jrw@chromium.org changed reviewers:
+ jamiewalch@chromium.org, sergeyu@chromium.org

[Jamie, 2015-03-12 20:15:43]

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/dns_...
File remoting/webapp/crd/js/dns_blackhole_checker.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/dns_...
remoting/webapp/crd/js/dns_blackhole_checker.js:156: * @param
{remoting.Xhr.Response} xhrr
This name looks like a typo. I think it should be called xhrResponse (or maybe
just response, where the context is obvious).

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

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:34: params.responseType == 'none');
Nit: Indentation.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:91: this.startInternal_(method, url, content,
headers, withCredentials);
I would prefer that the ctor doesn't implicitly send the XHR. An explicit
start() method that returns a Promise seems like a better design.

I think it's also clearer because as you have it right now, you have to proxy
the then() method, and the call-site reads as <noun>.then() instead of the more
conventional <noun>.<verb>().then().

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:127: *    entirely.  Default: 'text'.
Maybe make this an enumerated type so that the test in the ctor can be skipped
in favour of compiler-enforced checking?

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:186: * the promise to resolve for any HTTP status
code.
There's no other mention of resolveOnErrorStatus. Is this a stale comment?

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/unittests/d...
File remoting/webapp/unittests/dns_blackhole_checker_unittest.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/unittests/d...
remoting/webapp/unittests/dns_blackhole_checker_unittest.js:96:
then(checkState.bind(null, remoting.SignalStrategy.State.CONNECTED));
I don't think these calls to checkState need to be asynchronous. The existing
code should work fine for everything after respond() has completed because it's
all synchronous.

[John Williams, 2015-03-14 03:35:33]

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/dns_...
File remoting/webapp/crd/js/dns_blackhole_checker.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/dns_...
remoting/webapp/crd/js/dns_blackhole_checker.js:156: * @param
{remoting.Xhr.Response} xhrr
On 2015/03/12 20:15:43, Jamie wrote:
> This name looks like a typo. I think it should be called xhrResponse (or maybe
> just response, where the context is obvious).

Done.

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

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:34: params.responseType == 'none');
On 2015/03/12 20:15:43, Jamie wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:91: this.startInternal_(method, url, content,
headers, withCredentials);
On 2015/03/12 20:15:43, Jamie wrote:
> I would prefer that the ctor doesn't implicitly send the XHR. An explicit
> start() method that returns a Promise seems like a better design.
> 
> I think it's also clearer because as you have it right now, you have to proxy
> the then() method, and the call-site reads as <noun>.then() instead of the
more
> conventional <noun>.<verb>().then().

Done.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:127: *    entirely.  Default: 'text'.
On 2015/03/12 20:15:43, Jamie wrote:
> Maybe make this an enumerated type so that the test in the ctor can be skipped
> in favour of compiler-enforced checking?

Done, but I'm not sure I like the result.  Enumerated types are just so wordy in
Closure code.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:186: * the promise to resolve for any HTTP status
code.
On 2015/03/12 20:15:43, Jamie wrote:
> There's no other mention of resolveOnErrorStatus. Is this a stale comment?

Fixed.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/unittests/d...
File remoting/webapp/unittests/dns_blackhole_checker_unittest.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/unittests/d...
remoting/webapp/unittests/dns_blackhole_checker_unittest.js:96:
then(checkState.bind(null, remoting.SignalStrategy.State.CONNECTED));
On 2015/03/12 20:15:43, Jamie wrote:
> I don't think these calls to checkState need to be asynchronous. The existing
> code should work fine for everything after respond() has completed because
it's
> all synchronous.

Done.

[Jamie, 2015-03-16 18:06:34]

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/sess...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/sess...
remoting/webapp/crd/js/session_connector_impl.js:334:
this.pendingXhr_.then(this.onIT2MeHostInfo_.bind(this, hostId));
Why has this been deleted entirely?

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr.js
File remoting/webapp/crd/js/xhr.js (left):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
This file is missing from the latest patch-set.

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/it2me_connect_flow.js (right):

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:96: new remoting.Xhr({
Don't you need to return this?

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/session_connector_impl.js:98: // TODO(jrw): Why are there
member variables "declared" here??
IIRC, since this code would otherwise be duplicated in the ctor, I cheated and
put the JSDoc comments here. If you like, you can add a comment to that effect
in place of your TODO.

[John Williams, 2015-03-16 23:15:00]

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/sess...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/sess...
remoting/webapp/crd/js/session_connector_impl.js:334:
this.pendingXhr_.then(this.onIT2MeHostInfo_.bind(this, hostId));
On 2015/03/16 18:06:34, Jamie wrote:
> Why has this been deleted entirely?

It was deleted in the master branch.  I'm trying to avoid rebasing during
reviews but this is one of the branches I had already rebased when you made that
suggestion.

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr.js
File remoting/webapp/crd/js/xhr.js (left):

https://codereview.chromium.org/1003433002/diff/1/remoting/webapp/crd/js/xhr....
remoting/webapp/crd/js/xhr.js:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2015/03/16 18:06:34, Jamie wrote:
> This file is missing from the latest patch-set.

No idea what happened there.  Saved by the reflog.

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/it2me_connect_flow.js (right):

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:96: new remoting.Xhr({
On 2015/03/16 18:06:34, Jamie wrote:
> Don't you need to return this?

Sure, if I was actually calling this method :-)  Pardon me while I write some
unit tests.

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/session_connector_impl.js:98: // TODO(jrw): Why are there
member variables "declared" here??
On 2015/03/16 18:06:34, Jamie wrote:
> IIRC, since this code would otherwise be duplicated in the ctor, I cheated and
> put the JSDoc comments here. If you like, you can add a comment to that effect
> in place of your TODO.

Done.

[John Williams, 2015-03-16 23:16:23]

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/it2me_connect_flow.js (right):

https://codereview.chromium.org/1003433002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:96: new remoting.Xhr({
On 2015/03/16 23:15:00, John Williams wrote:
> On 2015/03/16 18:06:34, Jamie wrote:
> > Don't you need to return this?
> 
> Sure, if I was actually calling this method :-)  Pardon me while I write some
> unit tests.

Hey, wait, I removed this when I realized this wasn't new code I was writing,
and it just looks mean and sarcastic.

My actually response is that I fixed it, and I'm trying to track down why
jscompiler didn't report it earlier.

[kelvinp, 2015-03-17 01:32:45]

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

[kelvinp, 2015-03-17 01:32:46]

I like the idea :)

Just a few comments.

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:199: // XXX return
manifest.name;
what does //XXX stands for?

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:57: }).then(function(/**
XMLHttpRequest */ xhr) {
You need to fix that as well.

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:111:
remoting.It2MeConnectFlow.prototype.onHostInfo_ = function(xhr) {
This needed to be updated as well.

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:173: remoting.Xhr.Response;
Why is this needed?

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/qunit_proto.js (right):

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/js_prot...
remoting/webapp/js_proto/qunit_proto.js:103: // TODO(jrw): Mark these as
deprecated.
I am working on a change that will move QUnit to use the new syntax.

[John Williams, 2015-03-17 17:39:38]

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:199: // XXX return
manifest.name;
On 2015/03/17 01:32:46, kelvinp wrote:
> what does //XXX stands for?

"Don't commit this." Oops. I was testing to see if I could get JSCompile to
issue an error. Apparently the answer is no.

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:57: }).then(function(/**
XMLHttpRequest */ xhr) {
On 2015/03/17 01:32:46, kelvinp wrote:
> You need to fix that as well.

God bless JSCompiler and its rigorous type system.  This is why I'm teaching
myself Haskell.

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:111:
remoting.It2MeConnectFlow.prototype.onHostInfo_ = function(xhr) {
On 2015/03/17 01:32:46, kelvinp wrote:
> This needed to be updated as well.

Done.

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xhr.js:173: remoting.Xhr.Response;
On 2015/03/17 01:32:46, kelvinp wrote:
> Why is this needed?

I'm not sure I understand the question.  Are you asking just about 173?  It's
needed in order to declare a typedef (although I'm considering promoting this to
a full class).

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/qunit_proto.js (right):

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/js_prot...
remoting/webapp/js_proto/qunit_proto.js:103: // TODO(jrw): Mark these as
deprecated.
On 2015/03/17 01:32:46, kelvinp wrote:
> I am working on a change that will move QUnit to use the new syntax.

Acknowledged.

[kelvinp, 2015-03-17 22:06:21]

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:199: // XXX return
manifest.name;
On 2015/03/17 17:39:38, John Williams wrote:
> On 2015/03/17 01:32:46, kelvinp wrote:
> > what does //XXX stands for?
> 
> "Don't commit this." Oops. I was testing to see if I could get JSCompile to
> issue an error. Apparently the answer is no.

Acknowledged.

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:111:
remoting.It2MeConnectFlow.prototype.onHostInfo_ = function(xhr) {
On 2015/03/17 17:39:38, John Williams wrote:
> On 2015/03/17 01:32:46, kelvinp wrote:
> > This needed to be updated as well.
> 
> Done.

I don't think it is done yet, probably need
s/XMLHttpRequest/remoting.Xhr.Response right?

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

https://codereview.chromium.org/1003433002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:187: remoting.Xhr.prototype.onReadyStateChange_ =
function() {
Probably worth adding a comment saying that if you don't cache the result of the
XHR, it will be overwritten to an empty string.

https://codereview.chromium.org/1003433002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:237: return JSON.parse(this.text_);
This may throw, should we wrap in try catch and return null on failure?

[John Williams, 2015-03-18 20:07:23]

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

https://codereview.chromium.org/1003433002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/it2me_connect_flow.js:111:
remoting.It2MeConnectFlow.prototype.onHostInfo_ = function(xhr) {
On 2015/03/17 22:06:20, kelvinp wrote:
> On 2015/03/17 17:39:38, John Williams wrote:
> > On 2015/03/17 01:32:46, kelvinp wrote:
> > > This needed to be updated as well.
> > 
> > Done.
> 
> I don't think it is done yet, probably need
> s/XMLHttpRequest/remoting.Xhr.Response right?

Done.

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

https://codereview.chromium.org/1003433002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:187: remoting.Xhr.prototype.onReadyStateChange_ =
function() {
On 2015/03/17 22:06:20, kelvinp wrote:
> Probably worth adding a comment saying that if you don't cache the result of
the
> XHR, it will be overwritten to an empty string.

Done.

https://codereview.chromium.org/1003433002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:237: return JSON.parse(this.text_);
On 2015/03/17 22:06:20, kelvinp wrote:
> This may throw, should we wrap in try catch and return null on failure?

IMHO returning null in the case of an error is an anti-pattern because the error
may go totally unreported, and even in the best case, it throws away useful
information about the root cause of the failure.

I'm OK with using a null return when failure is an expected, non-error
condition, but missing or invalid JSON data is always an error in the context of
this method.

[Jamie, 2015-03-18 22:16:17]

My remaining comments are all trivial, so I'll remove myself from the review
process. It looks like Kelvin has some outstanding remarks, so I'll defer to him
for the final LG-to-M.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/app_re...
File remoting/webapp/app_remoting/js/feedback_consent.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/app_re...
remoting/webapp/app_remoting/js/feedback_consent.js:130: /** @param
{!remoting.Xhr.Response} response */
Nit: Use the shorter, in-line "/** <type> */" syntax.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/error_unittest.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/error_unittest.js:14: });
This looks like a rebase error.

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

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:16: * Executes an arbitrary HTTP method
asynchronously.
This comment would be better moved to either the @fileoverview (which is now
wrong) or the start() method, since the ctor no longer executes anything.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:91: /** Request a plain text response. */
These don't need to be javadoc-style comments, and could probably be on the same
line as the enum to be more concise (two-space separation, please).

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:125: *     Not compatible with the oauthToken
field.
You're not using this.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:154: return this.nativeXhr_.abort();
No return needed here.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:162: * resolveOnErrorStatus to true in the
constructor parameters allows
Remove this reference to resolveOnErrorStatus as well.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:290: * TODO(jrw): Stop using this.
Can you be more explicit about why you think this helper is a bad idea?

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/js_pro...
File remoting/webapp/js_proto/sinon_proto.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/js_pro...
remoting/webapp/js_proto/sinon_proto.js:99: sinon.fakeServer = {};
Namespaces are usually declared immediately before their contents.

Although, it doesn't look like you're using this, and it's not clear to me what
it's supposed to do. Can you remove it?

[John Williams, 2015-03-19 20:19:43]

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/app_re...
File remoting/webapp/app_remoting/js/feedback_consent.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/app_re...
remoting/webapp/app_remoting/js/feedback_consent.js:130: /** @param
{!remoting.Xhr.Response} response */
On 2015/03/18 22:16:16, Jamie wrote:
> Nit: Use the shorter, in-line "/** <type> */" syntax.

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/error_unittest.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/error_unittest.js:14: });
On 2015/03/18 22:16:16, Jamie wrote:
> This looks like a rebase error.

Done.

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

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:16: * Executes an arbitrary HTTP method
asynchronously.
On 2015/03/18 22:16:16, Jamie wrote:
> This comment would be better moved to either the @fileoverview (which is now
> wrong) or the start() method, since the ctor no longer executes anything.

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:91: /** Request a plain text response. */
On 2015/03/18 22:16:17, Jamie wrote:
> These don't need to be javadoc-style comments, and could probably be on the
same
> line as the enum to be more concise (two-space separation, please).

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:125: *     Not compatible with the oauthToken
field.
On 2015/03/18 22:16:17, Jamie wrote:
> You're not using this.

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:154: return this.nativeXhr_.abort();
On 2015/03/18 22:16:16, Jamie wrote:
> No return needed here.

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:162: * resolveOnErrorStatus to true in the
constructor parameters allows
On 2015/03/18 22:16:16, Jamie wrote:
> Remove this reference to resolveOnErrorStatus as well.

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/xhr.js:290: * TODO(jrw): Stop using this.
On 2015/03/18 22:16:17, Jamie wrote:
> Can you be more explicit about why you think this helper is a bad idea?

Done.

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/js_pro...
File remoting/webapp/js_proto/sinon_proto.js (right):

https://codereview.chromium.org/1003433002/diff/140001/remoting/webapp/js_pro...
remoting/webapp/js_proto/sinon_proto.js:99: sinon.fakeServer = {};
On 2015/03/18 22:16:17, Jamie wrote:
> Namespaces are usually declared immediately before their contents.
> 
> Although, it doesn't look like you're using this, and it's not clear to me
what
> it's supposed to do. Can you remove it?

Done.

[kelvinp, 2015-03-19 20:47:20]

lgtm

[John Williams, 2015-03-19 23:05:06]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-03-19 23:05:18]

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

[commit-bot: I haz the power, 2015-03-19 23:29:27]

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

[commit-bot: I haz the power, 2015-03-19 23:29:27]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[John Williams, 2015-03-19 23:49:36]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-19 23:49:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1003433002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 23:49:51]

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

[John Williams, 2015-03-19 23:57:54]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-19 23:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1003433002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 23:58:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2015-03-19 23:58:21]

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

[commit-bot: I haz the power, 2015-03-20 00:07:19]

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

[commit-bot: I haz the power, 2015-03-20 00:07:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[John Williams, 2015-03-20 18:01:41]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-20 18:01:41]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1003433002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-03-20 18:02:18]

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

[commit-bot: I haz the power, 2015-03-20 19:01:28]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-20 19:02:19]

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