Added mock_xhr module w/ unit tests.

remoting/webapp/crd/js/mock_xhr.js

+// Copyright (c) 2015 The Chromium Authors. All rights reserved.

[John Williams, 2015/04/02 23:57:06]

The main file for this CL.  Needs more docs, I know.

[Jamie, 2015/04/03 01:45:57]

No (c) needed

[Jamie, 2015/04/03 01:45:57]

Doesn't sinon already support mocking out XMLHttpRequest? Some comment
explaining why we need our own implementation would seem to be in order.

[John Williams, 2015/04/04 01:40:26]

Because it's a test file, or because we're phasing it out for all files?

[John Williams, 2015/04/04 01:40:26]

Done.

[Jamie, 2015/04/06 19:24:03]

It's not needed for any new files (for the past year, at least), but there's no
requirement to remove it from old files.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * A mock version of remoting.Xhr.
+ */
+
+(function() {
+'use strict';
+
+/**
+ * @constructor
+ * @param {remoting.Xhr.Params} params

[Jamie, 2015/04/03 01:45:57]

Would it make sense to declare this as @extends {XMLHttpRequest}? That way you
could explicitly annotate the mock methods with @overrides.

[John Williams, 2015/04/04 01:40:26]

Doing that causes the compiler to check for private variables in Xhr that are
also defined in MockXhr.  Since there is one (deferred_), I would rather not
rename it to satisfy the compiler, especially since an @extends declaration is
basically a lie anyway.

[Jamie, 2015/04/06 19:24:03]

Acknowledged.

+ */
+remoting.MockXhr = function(params) {
+  origXhr['checkParams_'](params);
+
+  /** @const {remoting.Xhr.Params} */
+  this.params = normalizeParams(params);
+
+

[Jamie, 2015/04/03 01:45:57]

Duplicate blank line.

[John Williams, 2015/04/04 01:40:26]

Done.

+  /** @private {base.Deferred<!remoting.Xhr.Response>} */
+  this.deferred_ = null;
+
+  /** @type {remoting.Xhr.Response} */
+  this.response_ = null;
+
+  /** @type {boolean} */
+  this.aborted_ = false;
+};
+
+/**
+ * @param {remoting.Xhr.Params} params
+ * @return {remoting.Xhr.Params} The argument with all missing fields
+ *     filled in with default values.
+ */
+var normalizeParams = function(params) {
+  return {
+    method: params.method,
+    url: params.url,
+    urlParams: typeof params.urlParams == 'object' ?
+        base.copyWithoutNullFields(params.urlParams) :
+        params.urlParams,
+    textContent: params.textContent,
+    jsonContent: params.jsonContent,
+    formContent: params.formContent ||
+        base.copyWithoutNullFields(params.formContent),

[Jamie, 2015/04/03 01:45:57]

You're only calling copyWithoutNullFields if its parameter is null, in which
case you know that the result will be {}. Why not use that here, or maybe call
it unconditionally?

[John Williams, 2015/04/04 01:40:26]

Having formContent === undefined is a special case that needs to be preserved
because it's forbidden for more than one of {textContent, jsonContent,
formContent} to be defined.

[Jamie, 2015/04/06 19:24:03]

That's not what this does, I don't think. If params.formContent is undefined,
then formContent will be passed as copyWithoutNF(undefined), which is {}. So
you're not preserving undefined here.

[John Williams, 2015/04/06 23:46:13]

I changed it to be more explicit about undefined.

+    headers: base.copyWithoutNullFields(params.headers),
+    withCredentials: !!params.withCredentials,

[Jamie, 2015/04/03 01:45:57]

Prefer Boolean(x) to !!x for clarity.

[John Williams, 2015/04/04 01:40:26]

Done.

+    oauthToken: params.oauthToken == null : null params.oauthToken,

[Jamie, 2015/04/03 01:45:57]

This doesn't look right.

[John Williams, 2015/04/04 01:40:26]

Yeah, I was in a hurry to get this CL out before I had to leave :(

+    useIdentity: !!params.useIdentity,
+    acceptJson: !!params.acceptJson
+  };
+};
+
+//////////////////////////////////////////////////////////////////////////////
+//
+//  Mock implementation.
+//

[Jamie, 2015/04/03 01:45:57]

I don't think we have a big block comment like this anywhere else. A more
natural ordering within the file would be to declare the private and helper
functions at the end (that's what we've done elsewhere) so that the public
methods are all together at the top.

+
+/**
+ * @return {void}
+ */
+remoting.MockXhr.prototype.abort = function() {
+  this.aborted_ = true;
+};
+
+/**
+ * @return {!Promise<!remoting.Xhr.Response>}
+ */
+remoting.MockXhr.prototype.start = function() {
+  runMatchingHandler(this);
+  if (!this.deferred_) {
+    this.deferred_ = new base.Deferred();
+    this.maybeRespond_();
+  }
+  return this.deferred_.promise();
+};
+
+//////////////////////////////////////////////////////////////////////////////
+//
+//  Mock-specific methods.
+//
+
+/**
+ * @param {number} status

[Jamie, 2015/04/03 01:45:57]

I think this (and subsequent) methods should be @private.

[John Williams, 2015/04/04 01:40:26]

They need to be called by user code (see the unit tests for examples).

+ */
+remoting.MockXhr.prototype.setEmptyResponse = function(status) {
+  this.setResponse_(new remoting.Xhr.Response(
+      status,
+      'mock status text from setEmptyResponse',
+      null,
+      '',
+      false));
+};
+
+/**
+ * @param {number} status
+ * @param {string} body
+ */
+remoting.MockXhr.prototype.setTextResponse = function(status, body) {
+  this.setResponse_(new remoting.Xhr.Response(
+      status,
+      'mock status text from setTextResponse',
+      null,
+      body || '',
+      false));
+};
+
+/**
+ * @param {number} status
+ * @param {*} body
+ */
+remoting.MockXhr.prototype.setJsonResponse = function(status, body) {
+  if (!this.params.acceptJson) {
+    throw new Error('client does not want JSON response');
+  }
+  this.setResponse_(new remoting.Xhr.Response(
+      status,
+      'mock status text from setJsonResponse',
+      null,
+      JSON.stringify(body),
+      true));
+};
+
+/**
+ * @param {!remoting.Xhr.Response} response
+ */
+remoting.MockXhr.prototype.setResponse_ = function(response) {

[Jamie, 2015/04/03 01:45:57]

I think this (and some other) methods should be @private.

[John Williams, 2015/04/04 01:40:26]

Done.

+  base.debug.assert(this.response_ == null);
+  this.response_ = response;
+  this.maybeRespond_();
+};
+
+remoting.MockXhr.prototype.maybeRespond_ = function() {
+  if (this.deferred_ && this.response_ && !this.aborted_) {
+    this.deferred_.resolve(this.response_);
+  }
+};
+
+/** @type {*} */

[Jamie, 2015/04/03 01:45:57]

Can you be more specific about the type of this?

[John Williams, 2015/04/04 01:40:26]

It's a pretty ugly type, but OK.

[Jamie, 2015/04/06 19:24:03]

Yuck; that is horrible. I was thinking it would be as simple as {remoting.Xhr},
but of course you need the type of the ctor. Can you add a comment to that
effect, since (as you point out) this is not very readable.

[John Williams, 2015/04/06 23:46:13]

Done.

+var origXhr = null;
+
+/**
+ * @type {!Array<remoting.MockXhr.UrlHandler>}
+ */
+var handlers = [];
+
+/**
+ * Registers a handler for a given method and URL.  The |urlPattern|
+ * argument may either be a string, which must equal a URL to match
+ * it, or a RegExp, which is matched against URLs with the |test|

[Jamie, 2015/04/03 01:45:57]

I don't think "with the test method" adds any value to this comment, since it's
implicit in the idea of matching a RegEx.

[John Williams, 2015/04/04 01:40:26]

Hmm. You're right. I'm used to there being two methods like |match| and |search|
in Python.

+ * method.
+ *
+ * Matching handlers are run when a FakeXhr's |start| method is called.
+ *
+ * @param {?string} method The HTTP method to respond to, or null to
+ *     response to any method.

[Jamie, 2015/04/03 01:45:57]

s/response/respond/

[John Williams, 2015/04/04 01:40:26]

Done.

+ * @param {?string|!RegExp} urlPattern The URL or pattern to respond
+ *     to, or null to match any URL.
+ * @param {function(!remoting.MockXhr):void} proc The function to call

[Jamie, 2015/04/03 01:45:57]

|callback| would be a more conventional name for this.

[John Williams, 2015/04/04 01:40:26]

Done.

+ *     when a matching XHR is started.
+ * @param {boolean=} opt_reuse If true, the response can be used for
+ *     multiple requests.
+ */
+remoting.MockXhr.setResponseFor = function(
+    method, urlPattern, proc, opt_reuse) {
+  handlers.push({
+    method: method,
+    urlPattern: urlPattern,
+    proc: proc,
+    reuse: !!opt_reuse
+  });
+};
+
+/**
+ * Calls |setResponseFor| to add a 200 response with text content.

[Jamie, 2015/04/03 01:45:57]

s/200/204/
s/with text content/(no content)/

I would also probably omit "Calls |setResponseFor| to" from these descriptions,
since it's not important to the caller how it adds the response.

[John Williams, 2015/04/04 01:40:26]

Done.

I said "calls |setResponseFor|" because |setResponseFor| is a public method with
more extensive documentation.  Is there another way I should express this, or
should I just copy the full documentation from |setResponseFor|?

[Jamie, 2015/04/06 19:24:03]

Either copy them, or add a "see setResponseFor" comment.

[John Williams, 2015/04/06 23:46:13]

Done.

+ *
+ * @param {?string} method
+ * @param {?string|!RegExp} urlPattern
+ * @param {boolean=} opt_reuse
+ */
+remoting.MockXhr.setEmptyResponseFor = function(method, urlPattern, opt_reuse) {
+  remoting.MockXhr.setResponseFor(
+      method, urlPattern, function(/** remoting.MockXhr */ xhr) {
+        xhr.setEmptyResponse(204);
+      }, opt_reuse);
+};
+
+/**
+ * Calls |setResponseFor| to add a 200 response with text content.
+ *
+ * @param {?string} method
+ * @param {?string|!RegExp} urlPattern
+ * @param {string} content
+ * @param {boolean=} opt_reuse
+ */
+remoting.MockXhr.setTextResponseFor = function(
+    method, urlPattern, content, opt_reuse) {
+  remoting.MockXhr.setResponseFor(
+      method, urlPattern, function(/** remoting.MockXhr */ xhr) {
+        xhr.setTextResponse(200, content);
+      }, opt_reuse);
+};
+
+/**
+ * Calls |setResponseFor| to add a 200 response with JSON content.
+ *
+ * @param {?string} method
+ * @param {?string|!RegExp} urlPattern
+ * @param {*} content
+ * @param {boolean=} opt_reuse
+ */
+remoting.MockXhr.setJsonResponseFor = function(
+    method, urlPattern, content, opt_reuse) {
+  remoting.MockXhr.setResponseFor(
+      method, urlPattern, function(/** remoting.MockXhr */ xhr) {
+        xhr.setJsonResponse(200, content);
+      }, opt_reuse);
+};
+
+/**
+ * Runs the most first handler for a given method and URL.
+ * @param {!remoting.MockXhr} xhr
+ */
+var runMatchingHandler = function(xhr) {
+  for (var i = 0; i < handlers.length; i++) {
+    var handler = handlers[i];
+    if (handler.method == null || handler.method != xhr.params.method) {
+      continue;
+    }
+    if (handler.urlPattern == null) {
+      // Let the handler run.
+    } else if (typeof handler.urlPattern == 'string') {
+      if (xhr.params.url != handler.urlPattern) {
+        continue;
+      }
+    } else {
+      var regexp = /** @type {RegExp} */ (handler.urlPattern);
+      if (!regexp.test(xhr.params.url)) {
+        continue;
+      }
+    }
+    if (!handler.reuse) {
+      handlers.splice(i, 1);
+    }
+    handler.proc(xhr);
+    return;
+  };
+  throw new Error(
+      'No handler registered for ' + xhr.params.method +
+      ' to '+ xhr.params.url);
+};
+
+remoting.MockXhr.activate = function() {
+  base.debug.assert(
+      origXhr == null,
+      'Xhr mocking already active');
+  origXhr = remoting.Xhr;
+  remoting.MockXhr.Response = remoting.Xhr.Response;
+  remoting['Xhr'] = remoting.MockXhr;
+};
+
+remoting.MockXhr.restore = function() {
+  base.debug.assert(
+      origXhr != null,
+      'Xhr mocking not active');
+  remoting['Xhr'] = origXhr;
+  origXhr = null;
+  handlers = [];
+};
+
+})();
+
+// Can't put put typedefs inside a function :-(
+/**
+ * @typedef {{
+ *   method:?string,
+ *   urlPattern:(?string|RegExp),
+ *   proc:function(!remoting.MockXhr):void,
+ *   reuse:boolean
+ * }}
+ */
+remoting.MockXhr.UrlHandler;