Updated XHR API so call sites are more descriptive.

remoting/webapp/crd/js/xhr.js

Index: remoting/webapp/crd/js/xhr.js
diff --git a/remoting/webapp/crd/js/xhr.js b/remoting/webapp/crd/js/xhr.js
index 4a3d5740e9eba76850a3c5b2496bffaec7f37aea..444a7387f20fe165678e2c32bab3c76a9f8fe15f 100644
--- a/remoting/webapp/crd/js/xhr.js
+++ b/remoting/webapp/crd/js/xhr.js
@@ -19,7 +19,7 @@ remoting.xhr = remoting.xhr || {};
 /**
  * Takes an associative array of parameters and urlencodes it.
  *
- * @param {Object.<string>} paramHash The parameter key/value pairs.
+ * @param {Object.<string,string>} paramHash The parameter key/value pairs.
  * @return {string} URLEncoded version of paramHash.
  */
 remoting.xhr.urlencodeParamHash = function(paramHash) {
@@ -36,108 +36,149 @@ remoting.xhr.urlencodeParamHash = function(paramHash) {
 };
 
 /**
- * Execute an XHR GET asynchronously.
+ * Parameters for the 'start' function.

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

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.

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

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.

  *
- * @param {string} url The base URL to GET, excluding parameters.
- * @param {function(XMLHttpRequest):void} onDone The function to call on
- *     completion.
- * @param {(string|Object.<string>)=} opt_parameters The request parameters,
- *     either as an associative array, or a string.  If it is a string, do
- *     not include the ? and be sure it is correctly URLEncoded.
- * @param {Object.<string>=} opt_headers Additional headers to include on the
- *     request.
- * @param {boolean=} opt_withCredentials Set the withCredentials flags in the
- *     XHR.
- * @return {XMLHttpRequest} The request object.
- */
-remoting.xhr.get = function(url, onDone, opt_parameters, opt_headers,
-                            opt_withCredentials) {
-  return remoting.xhr.doMethod('GET', url, onDone, opt_parameters,
-                               opt_headers, opt_withCredentials);
-};
+ * method: The HTTP method to use.
+ *
+ * url: The URL to request.
+ *
+ * onDone: Function to call when the XHR finishes.
 
-/**
- * Execute an XHR POST asynchronously.
+ * urlParams: (optional) Parameters to be appended to the URL.
+ *     Null-valued parameters are omitted.
+ *
+ * textContent: (optional) Text to be sent as the request body.
+ *
+ * fromContent: (optional) Data to be URL-encoded and sent as the

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

s/from/form/

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

Done.

+ *     request body.  Causes Content-type header to be set
+ *     appropriately.
+ *
+ * jsonContent: (optional) Data to be JSON-encoded and sent as the
+ *     request body.  Causes Content-type header to be set
+ *     appropriately.
+ *
+ * headers: (optional) Additional request headers to be sent.
+ *     Null-valued headers are omitted.
+ *
+ * withCredentials: (optional) Value of the XHR's withCredentials field.
  *
- * @param {string} url The base URL to POST, excluding parameters.
- * @param {function(XMLHttpRequest):void} onDone The function to call on
- *     completion.
- * @param {(string|Object.<string>)=} opt_parameters The request parameters,
- *     either as an associative array, or a string.  If it is a string, be
- *     sure it is correctly URLEncoded.
- * @param {Object.<string>=} opt_headers Additional headers to include on the
- *     request.
- * @param {boolean=} opt_withCredentials Set the withCredentials flags in the
- *     XHR.
- * @return {XMLHttpRequest} The request object.
+ * oauthToken: (optional) An OAuth2 token used to construct an
+ *     Authentication header.
+ *
+ * @typedef {{
+ *   method: string,
+ *   url:string,
+ *   onDone:(function(XMLHttpRequest):void),
+ *   urlParams:(string|Object<string,?string>|undefined),
+ *   textContent:(string|undefined),
+ *   formContent:(Object|undefined),
+ *   jsonContent:(*|undefined),
+ *   headers:(Object<string,?string>|undefined),
+ *   withCredentials:(boolean|undefined),
+ *   oauthToken:(string|undefined)
+ * }}
  */
-remoting.xhr.post = function(url, onDone, opt_parameters, opt_headers,
-                             opt_withCredentials) {
-  return remoting.xhr.doMethod('POST', url, onDone, opt_parameters,
-                               opt_headers, opt_withCredentials);
-};
+remoting.XhrParams;
 
 /**
- * Execute an XHR DELETE asynchronously.
+ * Returns a copy of the input object with all null or undefined
+ * fields removed.
  *
- * @param {string} url The base URL to DELETE, excluding parameters.
- * @param {function(XMLHttpRequest):void} onDone The function to call on
- *     completion.
- * @param {(string|Object.<string>)=} opt_parameters The request parameters,
- *     either as an associative array, or a string.  If it is a string, be
- *     sure it is correctly URLEncoded.
- * @param {Object.<string>=} opt_headers Additional headers to include on the
- *     request.
- * @param {boolean=} opt_withCredentials Set the withCredentials flags in the
- *     XHR.
- * @return {XMLHttpRequest} The request object.
+ * @param {Object<string,?string>|undefined} input
+ * @return {!Object<string,string>}
+ * @private
  */
-remoting.xhr.remove = function(url, onDone, opt_parameters, opt_headers,
-                             opt_withCredentials) {
-  return remoting.xhr.doMethod('DELETE', url, onDone, opt_parameters,
-                               opt_headers, opt_withCredentials);
+remoting.xhr.removeNullFields_ = function(input) {
+  /** @type {!Object<string,string>} */
+  var result = {};
+  if (input) {
+    for (var field in input) {
+      var value = input[field];
+      if (value != null) {
+        result[field] = value;
+      }
+    }
+  }
+  return result;
 };
 
 /**
- * Execute an XHR PUT asynchronously.
+ * Executes an arbitrary HTTP method asynchronously.
  *
- * @param {string} url The base URL to PUT, excluding parameters.
- * @param {function(XMLHttpRequest):void} onDone The function to call on
- *     completion.
- * @param {(string|Object.<string>)=} opt_parameters The request parameters,
- *     either as an associative array, or a string.  If it is a string, be
- *     sure it is correctly URLEncoded.
- * @param {Object.<string>=} opt_headers Additional headers to include on the
- *     request.
- * @param {boolean=} opt_withCredentials Set the withCredentials flags in the
- *     XHR.
- * @return {XMLHttpRequest} The request object.
+ * @param {remoting.XhrParams} params
+ * @return {XMLHttpRequest} The XMLHttpRequest object.
  */
-remoting.xhr.put = function(url, onDone, opt_parameters, opt_headers,
-                             opt_withCredentials) {
-  return remoting.xhr.doMethod('PUT', url, onDone, opt_parameters,
-                               opt_headers, opt_withCredentials);
+remoting.xhr.start = function(params) {
+  // Extract fields that can be used more or less as-is.
+  var method = params.method;
+  var url = params.url;
+  var onDone = params.onDone;
+  var headers = remoting.xhr.removeNullFields_(params.headers);
+  var withCredentials = params.withCredentials || false;
+
+  // Apply URL parameters.
+  var parameterString = '';
+  if (typeof(params.urlParams) === 'string') {
+    parameterString = params.urlParams;
+  } else if (typeof(params.urlParams) === 'object') {
+    parameterString = remoting.xhr.urlencodeParamHash(
+        remoting.xhr.removeNullFields_(params.urlParams));
+  }
+  if (parameterString) {
+    base.debug.assert(url.indexOf('?') == -1);
+    url += '?' + parameterString;
+  }
+
+  // Check that the content spec is consistent.
+  if ((0 +

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

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?

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

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.

+       (params.textContent !== undefined) +
+       (params.formContent !== undefined) +
+       (params.jsonContent !== undefined)) > 1) {
+    throw Error(

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

s/Error/new Error/ for consistency with (most of) the rest of the codebase.

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

Done.

+        'may only specify one of textContent, formContent, and jsonContent');
+  }
+
+  // Convert the content fields to a single text content variable.
+  /** @type {?string} */
+  var content = null;
+  if (params.textContent !== undefined) {
+    content = params.textContent;
+  } else if (params.formContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/x-www-form-urlencoded';
+    }
+    content = remoting.xhr.urlencodeParamHash(params.formContent);
+  } else if (params.jsonContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/json; charset=UTF-8';
+    }
+    content = JSON.stringify(params.jsonContent);
+  }
+
+  // Apply the oauthToken field.
+  if (params.oauthToken !== undefined) {
+    base.debug.assert(!('Authorization' in headers));
+    headers['Authorization'] = 'Bearer ' + params.oauthToken;
+  }
+
+  return remoting.xhr.startInternal_(
+      method, url, onDone, content, headers, withCredentials);
 };
 
 /**
- * Execute an arbitrary HTTP method asynchronously.
+ * Executes an arbitrary HTTP method asynchronously.
  *
- * @param {string} methodName The HTTP method name, e.g. "GET", "POST" etc.
- * @param {string} url The base URL, excluding parameters.
- * @param {function(XMLHttpRequest):void} onDone The function to call on
- *     completion.
- * @param {(string|Object.<string>)=} opt_parameters The request parameters,
- *     either as an associative array, or a string.  If it is a string, be
- *     sure it is correctly URLEncoded.
- * @param {Object.<string>=} opt_headers Additional headers to include on the
- *     request.
- * @param {boolean=} opt_withCredentials Set the withCredentials flags in the
- *     XHR.
+ * @param {string} method
+ * @param {string} url
+ * @param {function(XMLHttpRequest):void} onDone
+ * @param {?string} content
+ * @param {!Object<string,string>} headers
+ * @param {boolean} withCredentials
  * @return {XMLHttpRequest} The XMLHttpRequest object.
+ * @private
  */
-remoting.xhr.doMethod = function(methodName, url, onDone,
-                                 opt_parameters, opt_headers,
-                                 opt_withCredentials) {
+remoting.xhr.startInternal_ = function(
+    method, url, onDone, content, headers, withCredentials) {
   /** @type {XMLHttpRequest} */
   var xhr = new XMLHttpRequest();
   xhr.onreadystatechange = function() {
@@ -147,45 +188,12 @@ remoting.xhr.doMethod = function(methodName, url, onDone,
     onDone(xhr);
   };
 
-  var parameterString = '';
-  if (typeof(opt_parameters) === 'string') {
-    parameterString = opt_parameters;
-  } else if (typeof(opt_parameters) === 'object') {
-    parameterString = remoting.xhr.urlencodeParamHash(opt_parameters);
-  } else if (opt_parameters === undefined) {
-    // No problem here. Do nothing.
-  } else {
-    throw 'opt_parameters must be string or associated array.';
+  xhr.open(method, url, true);
+  for (var key in headers) {
+    xhr.setRequestHeader(key, headers[key]);
   }
-
-  var useBody = (methodName == 'POST') || (methodName == 'PUT');
-
-  if (!useBody && parameterString != '') {
-    url = url + '?' + parameterString;
-  }
-
-  xhr.open(methodName, url, true);
-  if (methodName == 'POST' &&
-      (typeof opt_headers !== 'object' ||
-       typeof opt_headers['Content-type'] !== 'string')) {
-    xhr.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
-  }
-  // Add in request headers.
-  if (typeof(opt_headers) === 'object') {
-    for (var key in opt_headers) {
-      xhr.setRequestHeader(key, opt_headers[key]);
-    }
-  } else if (opt_headers === undefined) {
-    // No problem here. Do nothing.
-  } else {
-    throw 'opt_headers must be associative array.';
-  }
-
-  if (opt_withCredentials) {
-    xhr.withCredentials = true;
-  }
-
-  xhr.send(useBody ? parameterString : null);
+  xhr.withCredentials = withCredentials;
+  xhr.send(content);
   return xhr;
 };