Updated XHR API so call sites are more descriptive.

Updated XHR API so call sites are more descriptive.

This CL mainly converts from using positional parameters to a single
struct, but there are various other changes as well.

The functions corresponding to different HTTP methods have been
eliminated, and the "doMethod" function has been renamed "start".
Saving one parameter at each call site seemed like a poor tradeoff for
having four functions that differ only in the HTTP method they use.

The core logic of creating an XHR and arranging for the onDone
function to be called has been factored out into a separate function,
startInternal_.

The "parameters" argument has been replaced by separate urlParams and
content arguments.  This eliminates the need to heuristically decide
what to do with the parmameters based on the HTTP method, and it
allows URL parameters and body content to be included in the same
call, a case which occurs in practice in host_controller.js.

There are now three parameters for sepecifying different types of body
content: textContent, formContent, and jsonContent.  They have
different data types, and the formContent and jsonContent parameters
implicitly set the Content-type header to the correct value.

A new parameter, oauthToken, is a convenience for adding a
correctly-formatted Authorization header.  This formatting was
previously duplicated at five seperate call sites.

The string "OAuth" in Authorization headers has been replaced with
"Bearer". The OAuth 2 spec supports the use of "Bearer" but not
"OAuth".

There are no longer any instances where headers are specified
explicitly, but the ability to do so still exists in the API.

Finally, headers and urlParameters with the value null are accepted
but not included in the HTTP request.  This simplifies the logic of
the doRegisterHost function in host_controller.js.

BUG=

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

[John Williams, 2015-02-21 00:22:44]

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

[John Williams, 2015-02-21 00:22:45]

This is a rewrite of the XHR changes I had in my GCD support CL.  It doesn't add
the features the GCD code uses, but it provides a logical place to put them.

[Jamie, 2015-02-21 01:35:48]

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/host_...
File remoting/webapp/crd/js/host_controller.js (left):

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/host_...
remoting/webapp/crd/js/host_controller.js:290: if (hostClientId) {
You've deleted this conditional. Was that intentional?

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

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:39: * Parameters for the 'start' function.
I'm not a big fan of mixing optional and mandatory parameters in this way. I
think a cleaner design would be something like:

method: leave it as the name of the function--that also means less churn at the
callsites.
url: make it the first (and only required) parameter.
onDone: ideally, we'd update this API to use Promises, but that's a much bigger
change and I'm not going to insist you do it for this CL (though I wouldn't
object :))

That said, I think the design you have is much cleaner than what we had before,
so I don't feel too strongly about any of the above.

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:52: * fromContent: (optional) Data to be
URL-encoded and sent as the
s/from/form/

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:133: if ((0 +
An explicit cast using Number would be more readable than '0+', but this still
feels a bit hacky. An alternative might be to change the "else if" clauses below
into simple "if" statements and assert in the formContent and jsonContent
clauses that content is still null. WDYT?

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:137: throw Error(
s/Error/new Error/ for consistency with (most of) the rest of the codebase.

[John Williams, 2015-02-23 23:00:55]

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/host_...
File remoting/webapp/crd/js/host_controller.js (left):

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/host_...
remoting/webapp/crd/js/host_controller.js:290: if (hostClientId) {
On 2015/02/21 01:35:48, Jamie wrote:
> You've deleted this conditional. Was that intentional?

Yes. The new version automagically omits URL parameters and headers whose value
is null.  I suppose line 289 in the new code should say "hostClientId || null"
to ensure the semantics haven't changed, but the type annotations led me to
believe hostClientId will always be truthy or null.

Omitting null fields feels somewhat magical to me, but it's a pattern I've used
a lot in the past, and I've always found the result easier to read than having
conditional logic at the call site.

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

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:39: * Parameters for the 'start' function.
On 2015/02/21 01:35:48, Jamie wrote:
> I'm not a big fan of mixing optional and mandatory parameters in this way. I
> think a cleaner design would be something like:
> 
> method: leave it as the name of the function--that also means less churn at
the
> callsites.
> url: make it the first (and only required) parameter.
> onDone: ideally, we'd update this API to use Promises, but that's a much
bigger
> change and I'm not going to insist you do it for this CL (though I wouldn't
> object :))
> 
> That said, I think the design you have is much cleaner than what we had
before,
> so I don't feel too strongly about any of the above.

I prefer to keep it the way it is, because when there are lots of arguments, I
prefer to have them all labeled at the call site :)

Closure's type system is good enough to complain about missing mandatory
arguments, but there are other things that bother me enough that I might just
try to fix them in jscompiler. One is that "optional" arguments aren't really
optional in all use cases (e.g. if you declare a variable of type
remoting.XhrParams), and the other is that it's not an error to include extra
arguments that aren't declared.

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:52: * fromContent: (optional) Data to be
URL-encoded and sent as the
On 2015/02/21 01:35:48, Jamie wrote:
> s/from/form/

Done.

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:133: if ((0 +
On 2015/02/21 01:35:48, Jamie wrote:
> An explicit cast using Number would be more readable than '0+', but this still
> feels a bit hacky. An alternative might be to change the "else if" clauses
below
> into simple "if" statements and assert in the formContent and jsonContent
> clauses that content is still null. WDYT?

Changed to an explicit cast. I don't want to use separate "if" statements
because I want a descriptive error message, but I don't want to write the
message three times or write three almost identical messages.

Generally speaking, I think three-way mutual exclusivity is just hard to express
elegantly.

https://codereview.chromium.org/945033002/diff/1/remoting/webapp/crd/js/xhr.j...
remoting/webapp/crd/js/xhr.js:137: throw Error(
On 2015/02/21 01:35:48, Jamie wrote:
> s/Error/new Error/ for consistency with (most of) the rest of the codebase.

Done.

[Jamie, 2015-02-23 23:15:54]

lgtm

[John Williams, 2015-02-24 01:32:13]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-02-24 01:34:16]

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

[commit-bot: I haz the power, 2015-02-24 01:39:52]

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

[commit-bot: I haz the power, 2015-02-24 01:39:53]

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

[John Williams, 2015-02-24 03:35:32]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-02-24 03:35:43]

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

[commit-bot: I haz the power, 2015-02-24 03:40:50]

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

[commit-bot: I haz the power, 2015-02-24 03:40:51]

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

[John Williams, 2015-02-24 03:43:21]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-02-24 18:47:01]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-02-24 18:47:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/945033002/#ps40001 (title:
"Reparented branch.")

[commit-bot: I haz the power, 2015-02-24 18:48:43]

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

[commit-bot: I haz the power, 2015-02-24 22:32:12]

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

[commit-bot: I haz the power, 2015-02-24 22:32:13]

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

[John Williams, 2015-02-25 23:06:08]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-02-25 23:06:09]

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

[commit-bot: I haz the power, 2015-02-25 23:07:58]

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

[commit-bot: I haz the power, 2015-02-26 00:04:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-26 00:05:17]

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