Eliminated named constants for instances of remoting.Error.

remoting/webapp/crd/js/error.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /** @suppress {duplicate} */
 var remoting = remoting || {};
 
 /**
  * A wrapper for remoting.Error.Tag.  Having a wrapper makes it
  * possible to use instanceof checks on caught exceptions.  It also
  * allows adding more detailed error information if desired.
  *
  * @constructor
  * @param {remoting.Error.Tag} tag
  * @param {string=} opt_message
  */
 remoting.Error = function(tag, opt_message) {
-  /** @const {remoting.Error.Tag} */
-  this.tag = tag;
+  /** @private @const {remoting.Error.Tag} */
+  this.tag_ = tag;
 
   /** @const {?string} */
   this.message = opt_message || null;
 };
 
 /**
- * @return {boolean} True if this object represents an error
- *     condition.
+ * @return {string}
  */
-remoting.Error.prototype.isError = function() {
-  return this.tag != remoting.Error.Tag.NONE;
+remoting.Error.prototype.getTagForLocalization = function() {

[Jamie, 2015/03/13 17:56:17]

I think getTag is a better name, but then I'm not convinced that hasTag(x) is
any clearer or safer than getTag() == x. Is it just the confusion between == and
===?

[John Williams, 2015/03/13 19:30:29]

No, == vs. === is not a major issue AFAICT.  What matters is that if someone
writes an expression like one of these, the comparison will always return false:

  error == remoting.Error.Tag.NONE
  error == remoting.Error.none()
  error.tag == remoting.Error.none()

This seems like a pretty easy mistake to make.

...

After some further travel down the JS semantics rathole, I've discovered that
comparing an error object to a string will actually work if the object's
toString method returns the tag, so maybe I should rename getTag to toString,
get rid of the toString method I wrote, and just log error objects as objects so
the detail field is logged.   Personally I'd rather not do that, since it's
obscure, and it conflicts with my Java-based intuition that the value returned
by a toString method is ONLY for producing human-readable output.

(BTW, nothing uses the detail field yet, but I plan on using it in new code, and
the existing remoting.XmppConnection.prototype.onError_ method looks like a good
place to use it as well.)

[Jamie, 2015/03/13 20:26:00]

I'm inclined to agree with you, and the new form of hasTag that can take
multiple arguments adds enough value for me to withdraw my objection.

+  return this.tag_;
 };
 
 /**
+ * @return {string}
+ */
+remoting.Error.prototype.getTagForServerLog = function() {

[Jamie, 2015/03/13 17:56:17]

Another point in favour of calling the previous method getTag. I think this
mapping is better where it was.

[John Williams, 2015/03/13 19:30:29]

Done.

+  // Directory service should be updated if a new string is added here as
+  // otherwise the error code will be ignored (i.e. recorded as 0 instead).
+  switch (this.tag_) {
+    case remoting.Error.Tag.NONE:
+      return 'none';
+    case remoting.Error.Tag.INVALID_ACCESS_CODE:
+      return 'invalid-access-code';
+    case remoting.Error.Tag.MISSING_PLUGIN:
+      return 'missing_plugin';
+    case remoting.Error.Tag.AUTHENTICATION_FAILED:
+      return 'authentication-failed';
+    case remoting.Error.Tag.HOST_IS_OFFLINE:
+      return 'host-is-offline';
+    case remoting.Error.Tag.INCOMPATIBLE_PROTOCOL:
+      return 'incompatible-protocol';
+    case remoting.Error.Tag.BAD_PLUGIN_VERSION:
+      return 'bad-plugin-version';
+    case remoting.Error.Tag.NETWORK_FAILURE:
+      return 'network-failure';
+    case remoting.Error.Tag.HOST_OVERLOAD:
+      return 'host-overload';
+    case remoting.Error.Tag.P2P_FAILURE:
+      return 'p2p-failure';
+    case remoting.Error.Tag.UNEXPECTED:
+      return 'unexpected';
+    default:
+      return 'unknown-' + this.tag_;
+  }
+};
+
+/**
+ * Checks the type of an error.
+ * @param {remoting.Error.Tag} tag
+ * @return {boolean} True if this object has the specified tag.
+ */
+remoting.Error.prototype.hasTag = function(tag) {
+  return this.tag_ == tag;
+};
+
+/**
+ * Checks the type of an error.
+ * @param {!Array<remoting.Error.Tag>} tags
+ * @return {boolean} True if this object has one of the specified tags.
+ */
+remoting.Error.prototype.hasAnyTag = function(tags) {

[Jamie, 2015/03/13 17:56:17]

I think you can omit this method and just use varargs style for hasTag (which
would address my concern about it not being particularly useful in the presence
of getTag).

[John Williams, 2015/03/13 19:30:29]

Done.

+  var thisTag = this.tag_;
+  return tags.some(function(tag) {
+    return thisTag == tag;
+  });
+};
+
+/**
+ * @return {boolean} True if this object's tag is NONE, meaning this
+ *     object represents the lack of an error.
+ */
+remoting.Error.prototype.isNone = function() {
+  return this.hasTag(remoting.Error.Tag.NONE);
+};
+
+/**
+ * Convenience method for creating the second most common error type.
+ * @return {!remoting.Error}
+ */
+remoting.Error.none = function() {
+  return new remoting.Error(remoting.Error.Tag.NONE);
+};
+
+/**
+ * Convenience method for creating the most common error type.
+ * @return {!remoting.Error}
+ */
+remoting.Error.unexpected = function() {
+  return new remoting.Error(remoting.Error.Tag.UNEXPECTED);
+};
+
+/**
  * @enum {string} All error messages from messages.json
  */
 remoting.Error.Tag = {
   NONE: '',
 
   // Used to signify that an operation was cancelled by the user. This should
   // not normally cause the error text to be shown to the user, so the
   // i18n-content prefix is not needed in this case.
   CANCELLED: '__CANCELLED__',
 
@@ skipping 17 matching lines @@
   // TODO(garykac): Move app-specific errors into separate location.
   APP_NOT_AUTHORIZED: /*i18n-content*/'ERROR_APP_NOT_AUTHORIZED'
 };
 
 // A whole bunch of semi-redundant constants, mostly to reduce to size
 // of the diff that introduced the remoting.Error class.
 //
 // Please don't add any more constants here; just call the
 // remoting.Error constructor directly
 
-/** @const */
-remoting.Error.NONE = new remoting.Error(remoting.Error.Tag.NONE);
-
-/** @const */
-remoting.Error.CANCELLED =
-  new remoting.Error(remoting.Error.Tag.CANCELLED);
-
-/** @const */
-remoting.Error.INVALID_ACCESS_CODE =
-  new remoting.Error(remoting.Error.Tag.INVALID_ACCESS_CODE);
-
-/** @const */
-remoting.Error.MISSING_PLUGIN =
-  new remoting.Error(remoting.Error.Tag.MISSING_PLUGIN);
-
-/** @const */
-remoting.Error.AUTHENTICATION_FAILED =
-  new remoting.Error(remoting.Error.Tag.AUTHENTICATION_FAILED);
-
-/** @const */
-remoting.Error.HOST_IS_OFFLINE =
-  new remoting.Error(remoting.Error.Tag.HOST_IS_OFFLINE);
-
-/** @const */
-remoting.Error.INCOMPATIBLE_PROTOCOL =
-  new remoting.Error(remoting.Error.Tag.INCOMPATIBLE_PROTOCOL);
-
-/** @const */
-remoting.Error.BAD_PLUGIN_VERSION =
-  new remoting.Error(remoting.Error.Tag.BAD_PLUGIN_VERSION);
-
-/** @const */
-remoting.Error.NETWORK_FAILURE =
-  new remoting.Error(remoting.Error.Tag.NETWORK_FAILURE);
-
-/** @const */
-remoting.Error.HOST_OVERLOAD =
-  new remoting.Error(remoting.Error.Tag.HOST_OVERLOAD);
-
-/** @const */
-remoting.Error.UNEXPECTED =
-  new remoting.Error(remoting.Error.Tag.UNEXPECTED);
-
-/** @const */
-remoting.Error.SERVICE_UNAVAILABLE =
-  new remoting.Error(remoting.Error.Tag.SERVICE_UNAVAILABLE);
-
-/** @const */
-remoting.Error.NOT_AUTHENTICATED =
-  new remoting.Error(remoting.Error.Tag.NOT_AUTHENTICATED);
-
-/** @const */
-remoting.Error.NOT_FOUND =
-  new remoting.Error(remoting.Error.Tag.NOT_FOUND);
-
-/** @const */
-remoting.Error.INVALID_HOST_DOMAIN =
-  new remoting.Error(remoting.Error.Tag.INVALID_HOST_DOMAIN);
-
-/** @const */
-remoting.Error.P2P_FAILURE =
-  new remoting.Error(remoting.Error.Tag.P2P_FAILURE);
-
-/** @const */
-remoting.Error.REGISTRATION_FAILED =
-  new remoting.Error(remoting.Error.Tag.REGISTRATION_FAILED);
-
-/** @const */
-remoting.Error.NOT_AUTHORIZED =
-  new remoting.Error(remoting.Error.Tag.NOT_AUTHORIZED);
-
-/** @const */
-remoting.Error.APP_NOT_AUTHORIZED =
-  new remoting.Error(remoting.Error.Tag.APP_NOT_AUTHORIZED);
-
 /**
  * @param {number} httpStatus An HTTP status code.
  * @return {!remoting.Error} The remoting.Error enum corresponding to the
  *     specified HTTP status code.
  */
 remoting.Error.fromHttpStatus = function(httpStatus) {
   if (httpStatus == 0) {
-    return remoting.Error.NETWORK_FAILURE;
+    return new remoting.Error(remoting.Error.Tag.NETWORK_FAILURE);
   } else if (httpStatus >= 200 && httpStatus < 300) {
-    return remoting.Error.NONE;
+    return remoting.Error.none();
   } else if (httpStatus == 400 || httpStatus == 401) {
-    return remoting.Error.AUTHENTICATION_FAILED;
+    return new remoting.Error(remoting.Error.Tag.AUTHENTICATION_FAILED);
   } else if (httpStatus == 403) {
-    return remoting.Error.NOT_AUTHORIZED;
+    return new remoting.Error(remoting.Error.Tag.NOT_AUTHORIZED);
   } else if (httpStatus == 404) {
-    return remoting.Error.NOT_FOUND;
+    return new remoting.Error(remoting.Error.Tag.NOT_FOUND);
   } else if (httpStatus >= 500 && httpStatus < 600) {
-    return remoting.Error.SERVICE_UNAVAILABLE;
+    return new remoting.Error(remoting.Error.Tag.SERVICE_UNAVAILABLE);
   } else {
     console.warn('Unexpected HTTP error code: ' + httpStatus);
-    return remoting.Error.UNEXPECTED;
+    return remoting.Error.unexpected();
   }
 };
 
 /**
  * Create an error-handling function suitable for passing to a
  * Promise's "catch" method.
  *
  * @param {function(!remoting.Error):void} onError
  * @return {function(*):void}
  */
 remoting.Error.handler = function(onError) {
   return function(/** * */ error) {
     if (error instanceof remoting.Error) {
       onError(/** @type {!remoting.Error} */ (error));
     } else {
       console.error('Unexpected error: %o', error);
-      onError(remoting.Error.UNEXPECTED);
+      onError(remoting.Error.unexpected());
     }
   };
 };