It2MeHelperChannel error reporting cleanup

remoting/webapp/background/it2me_helper_channel.js

Index: remoting/webapp/background/it2me_helper_channel.js
diff --git a/remoting/webapp/background/it2me_helper_channel.js b/remoting/webapp/background/it2me_helper_channel.js
index e039783273b6c462e57bc24fde01b1cc0f73b481..6caeaf1ca4b66904e90c8176adf63c6d5afeb352 100644
--- a/remoting/webapp/background/it2me_helper_channel.js
+++ b/remoting/webapp/background/it2me_helper_channel.js
@@ -119,7 +119,8 @@ remoting.It2MeHelperChannel.HangoutMessageTypes = {
   HELLO: 'hello',
   HELLO_RESPONSE: 'helloResponse',
   CONNECT: 'connect',
-  DISCONNECT: 'disconnect'
+  DISCONNECT: 'disconnect',
+  ERROR: 'error'
 };
 
 /** @enum {string} */
@@ -167,11 +168,7 @@ remoting.It2MeHelperChannel.prototype.onHangoutMessage_ = function(message) {
     throw new Error('Unknown message method=' + message.method);
   } catch(e) {
     var error = /** @type {Error} */ e;
-    console.error(error);
-    this.hangoutPort_.postMessage({
-      method: message.method + 'Response',
-      error: error.message
-    });
+    this.sendErrorResponse_(this.hangoutPort_, message, error);
   }
   return false;
 };
@@ -280,11 +277,7 @@ remoting.It2MeHelperChannel.prototype.onWebappMessage_ = function(message) {
     throw new Error('Unknown message method=' + message.method);
   } catch(e) {
     var error = /** @type {Error} */ e;
-    console.error(error);
-    this.webappPort_.postMessage({
-      method: message.method + 'Response',
-      error: error.message
-    });
+    this.sendErrorResponse_(this.webappPort_, message, error);
   }
   return false;
 };
@@ -309,3 +302,25 @@ remoting.It2MeHelperChannel.prototype.unhookPorts_ = function() {
     this.onDisconnectCallback_  = null;
   }
 };
+
+/**
+ * @param {chrome.runtime.Port} port
+ * @param {?{method:string, data:Object.<string,*>}} incomingMessage

[Jamie, 2014/09/05 22:01:21]

incomingMessage is never null in this code; do you anticipate that it might be
in the future? If so, consider making it an optional parameter instead; assuming
the caller always knows whether or the error is a response to a message, I think
an optional parameter would be more readable here.

+ * @param {string|Error} error
+ * @private
+ */
+remoting.It2MeHelperChannel.prototype.sendErrorResponse_ =
+    function(port, incomingMessage, error) {
+  if (error instanceof Error) {
+    error = error.message;
+  }

[Jamie, 2014/09/05 22:01:21]

I think it would be cleaner to require the caller to pass a string (or, better,
an Error, since that's what the callers are doing right now, and it would allow
you to log it with a call stack here). I'm not a fan of changing the type of a
variable, even within such a limited scope.

+
+  console.error('Error responding to message method:' +
+                (incomingMessage ? incomingMessage.method : 'null') +
+                ' error:' + error);
+  port.postMessage({
+    method: remoting.It2MeHelperChannel.HangoutMessageTypes.ERROR,
+    message: error,
+    request: incomingMessage
+  });
+};