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

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 0551bdca3be439f0e620eddae9d9a05144d6f200..5b68c860338da70720dea98ed98806385a1fba53 100644
--- a/remoting/webapp/crd/js/xhr.js
+++ b/remoting/webapp/crd/js/xhr.js
@@ -12,26 +12,88 @@
 /** @suppress {duplicate} */
 var remoting = remoting || {};
 
-/** Namespace for XHR functions */
-/** @type {Object} */
-remoting.xhr = remoting.xhr || {};
-
 /**
- * Takes an associative array of parameters and urlencodes it.
+ * Executes an arbitrary HTTP method asynchronously.
  *
- * @param {Object<string,string>} paramHash The parameter key/value pairs.
- * @return {string} URLEncoded version of paramHash.
+ * @constructor
+ * @param {remoting.Xhr.Params} params
  */
-remoting.xhr.urlencodeParamHash = function(paramHash) {
-  var paramArray = [];
-  for (var key in paramHash) {
-    paramArray.push(encodeURIComponent(key) +
-                     '=' + encodeURIComponent(paramHash[key]));
+remoting.Xhr = function(params) {
+  /** @private @const {!XMLHttpRequest} */
+  this.nativeXhr_ = new XMLHttpRequest();
+  this.nativeXhr_.onreadystatechange = this.onReadyStateChange_.bind(this);
+  this.nativeXhr_.withCredentials = params.withCredentials || false;
+
+  /** @private @const */
+  this.responseType_ = params.responseType || remoting.Xhr.ResponseType.TEXT;
+
+  // Apply URL parameters.
+  var url = params.url;
+  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 (paramArray.length > 0) {
-    return paramArray.join('&');
+  if (parameterString) {
+    base.debug.assert(url.indexOf('?') == -1);
+    url += '?' + parameterString;
   }
-  return '';
+
+  // Check that the content spec is consistent.
+  if ((Number(params.textContent !== undefined) +
+       Number(params.formContent !== undefined) +
+       Number(params.jsonContent !== undefined)) > 1) {
+    throw new Error(
+        'may only specify one of textContent, formContent, and jsonContent');
+  }
+
+  // Prepare the build modified headers.
+  var headers = remoting.Xhr.removeNullFields_(params.headers);
+
+  // Convert the content fields to a single text content variable.
+  /** @private {?string} */
+  this.content_ = null;
+  if (params.textContent !== undefined) {
+    this.content_ = params.textContent;
+  } else if (params.formContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/x-www-form-urlencoded';
+    }
+    this.content_ = remoting.Xhr.urlencodeParamHash(params.formContent);
+  } else if (params.jsonContent !== undefined) {
+    if (!('Content-type' in headers)) {
+      headers['Content-type'] = 'application/json; charset=UTF-8';
+    }
+    this.content_ = JSON.stringify(params.jsonContent);
+  }
+
+  // Apply the oauthToken field.
+  if (params.oauthToken !== undefined) {
+    base.debug.assert(!('Authorization' in headers));
+    headers['Authorization'] = 'Bearer ' + params.oauthToken;
+  }
+
+  this.nativeXhr_.open(params.method, url, true);
+  for (var key in headers) {
+    this.nativeXhr_.setRequestHeader(key, headers[key]);
+  }
+
+  /** @private {base.Deferred<!remoting.Xhr.Response>} */
+  this.deferred_ = null;
+};
+
+/**
+ * @enum {string}
+ */
+remoting.Xhr.ResponseType = {
+  /** Request a plain text response. */
+  TEXT: 'TEXT',
+  /** Request a JSON response. */
+  JSON: 'JSON',
+  /** Ignore any response body sent by the remote host. */
+  NONE: 'NONE'
 };
 
 /**
@@ -41,8 +103,6 @@ remoting.xhr.urlencodeParamHash = function(paramHash) {
  *
  * url: The URL to request.
  *
- * onDone: Function to call when the XHR finishes.
-
  * urlParams: (optional) Parameters to be appended to the URL.
  *     Null-valued parameters are omitted.
  *
@@ -61,23 +121,121 @@ remoting.xhr.urlencodeParamHash = function(paramHash) {
  *
  * withCredentials: (optional) Value of the XHR's withCredentials field.
  *
+ * useOauth: (optional) If true, use OAuth2 to authenticate the request.
+ *     Not compatible with the oauthToken field.
+ *
  * oauthToken: (optional) An OAuth2 token used to construct an
- *     Authentication header.
+ *     Authentication header.  Not compatible with the useOauth field.
+ *
+ * responseType: (optional) Request a response of a specific
+ *    type. Default: TEXT.
  *
  * @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)
+ *   useOauth:(boolean|undefined),
+ *   oauthToken:(string|undefined),
+ *   responseType:(remoting.Xhr.ResponseType|undefined)
  * }}
  */
-remoting.XhrParams;
+remoting.Xhr.Params;
+
+/**
+ * Aborts the HTTP request.  Does nothing is the request has finished
+ * already.
+ */
+remoting.Xhr.prototype.abort = function() {
+  return this.nativeXhr_.abort();
+};
+
+/**
+ * Gets a promise that is resolved when the request completes.
+ *
+ * If an HTTP status code outside the 200..299 range is returned, the
+ * promise is rejected with a remoting.Error by default.  Setting
+ * resolveOnErrorStatus to true in the constructor parameters allows
+ * the promise to resolve for any HTTP status code.
+ *
+ * Any error that prevents the HTTP request from succeeding causes
+ * this promise to be rejected.
+ *
+ * NOTE: Calling this method more than once will return the same
+ * promise and not start a new request, despite what the name
+ * suggests.
+ *
+ * @return {!Promise<!remoting.Xhr.Response>}
+ */
+remoting.Xhr.prototype.start = function() {
+  if (this.deferred_ == null) {
+    var xhr = this.nativeXhr_;
+    xhr.send(this.content_);
+    this.content_ = null;  // for gc
+    this.deferred_ = new base.Deferred();
+  }
+  return this.deferred_.promise();
+};
+
+/**
+ * @private
+ */
+remoting.Xhr.prototype.onReadyStateChange_ = function() {

[kelvinp, 2015/03/17 22:06:20]

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.

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

Done.

+  var xhr = this.nativeXhr_;
+  if (xhr.readyState == 4) {
+    this.deferred_.resolve(new remoting.Xhr.Response(xhr, this.responseType_));
+  }
+};
+
+/**
+ * @constructor
+ * @param {!XMLHttpRequest} xhr
+ * @param {remoting.Xhr.ResponseType} type
+ */
+remoting.Xhr.Response = function(xhr, type) {
+  /** @private @const */
+  this.type_ = type;
+
+  /**
+   * The HTTP status code.
+   * @const {number}
+   */
+  this.status = xhr.status;
+
+  /**
+   * The HTTP status description.
+   * @const {string}
+   */
+  this.statusText = xhr.statusText;
+
+  /**
+   * The response URL, if any.
+   * @const {?string}
+   */
+  this.url = xhr.responseURL;
+
+  /** @private {string} */
+  this.text_ = xhr.responseText || '';
+};
+
+/**
+ * @return {string} The text content of the response.
+ */
+remoting.Xhr.Response.prototype.getText = function() {
+  return this.text_;
+};
+
+/**
+ * @return {*} The parsed JSON content of the response.
+ */
+remoting.Xhr.Response.prototype.getJson = function() {
+  base.debug.assert(this.type_ == remoting.Xhr.ResponseType.JSON);
+  return JSON.parse(this.text_);

[kelvinp, 2015/03/17 22:06:20]

This may throw, should we wrap in try catch and return null on failure?

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

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.

+};
 
 /**
  * Returns a copy of the input object with all null or undefined
@@ -87,7 +245,7 @@ remoting.XhrParams;
  * @return {!Object<string,string>}
  * @private
  */
-remoting.xhr.removeNullFields_ = function(input) {
+remoting.Xhr.removeNullFields_ = function(input) {
   /** @type {!Object<string,string>} */
   var result = {};
   if (input) {
@@ -102,118 +260,42 @@ remoting.xhr.removeNullFields_ = function(input) {
 };
 
 /**
- * Executes an arbitrary HTTP method asynchronously.
+ * Takes an associative array of parameters and urlencodes it.
  *
- * @param {remoting.XhrParams} params
- * @return {XMLHttpRequest} The XMLHttpRequest object.
+ * @param {Object<string,string>} paramHash The parameter key/value pairs.
+ * @return {string} URLEncoded version of paramHash.
  */
-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 ((Number(params.textContent !== undefined) +
-       Number(params.formContent !== undefined) +
-       Number(params.jsonContent !== undefined)) > 1) {
-    throw new Error(
-        '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;
+remoting.Xhr.urlencodeParamHash = function(paramHash) {
+  var paramArray = [];
+  for (var key in paramHash) {
+    paramArray.push(encodeURIComponent(key) +
+                     '=' + encodeURIComponent(paramHash[key]));
   }
-
-  return remoting.xhr.startInternal_(
-      method, url, onDone, content, headers, withCredentials);
-};
-
-/**
- * Executes an arbitrary HTTP method asynchronously.
- *
- * @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.startInternal_ = function(
-    method, url, onDone, content, headers, withCredentials) {
-  /** @type {XMLHttpRequest} */
-  var xhr = new XMLHttpRequest();
-  xhr.onreadystatechange = function() {
-    if (xhr.readyState != 4) {
-      return;
-    }
-    onDone(xhr);
-  };
-
-  xhr.open(method, url, true);
-  for (var key in headers) {
-    xhr.setRequestHeader(key, headers[key]);
+  if (paramArray.length > 0) {
+    return paramArray.join('&');
   }
-  xhr.withCredentials = withCredentials;
-  xhr.send(content);
-  return xhr;
+  return '';
 };
 
 /**
  * Generic success/failure response proxy.
+ * TODO(jrw): Stop using this.
  *
  * @param {function():void} onDone
  * @param {function(!remoting.Error):void} onError
  * @param {Array<remoting.Error.Tag>=} opt_ignoreErrors
- * @return {function(XMLHttpRequest):void}
+ * @return {function(!remoting.Xhr.Response):void}
  */
-remoting.xhr.defaultResponse = function(onDone, onError, opt_ignoreErrors) {
-  /** @param {XMLHttpRequest} xhr */
-  var result = function(xhr) {
-    var error =
-        remoting.Error.fromHttpStatus(/** @type {number} */ (xhr.status));
-    if (!error.isError()) {
+remoting.Xhr.defaultResponse = function(onDone, onError, opt_ignoreErrors) {
+  /** @param {!remoting.Xhr.Response} response */
+  var result = function(response) {
+    var error = remoting.Error.fromHttpStatus(response.status);
+    if (error.isNone()) {
       onDone();
       return;
     }
 
-    if (opt_ignoreErrors && opt_ignoreErrors.indexOf(error.tag) !== -1) {
+    if (opt_ignoreErrors && error.hasTag.apply(error, opt_ignoreErrors)) {
       onDone();
       return;
     }