It2MeHelperChannel error reporting cleanup

remoting/webapp/background/it2me_helper_channel.js

 // Copyright 2014 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.
 
 /**
  * @fileoverview
  *
  * It2MeHelperChannel relays messages between Hangouts and Chrome Remote Desktop
  * (webapp) for the helper (the Hangouts participant who is giving remote
  * assistance).
@@ skipping 101 matching lines @@
   this.onWebappDisconnectRef_ = this.onWebappDisconnect_.bind(this);
   this.onHangoutMessageRef_ = this.onHangoutMessage_.bind(this);
   this.onHangoutDisconnectRef_ = this.onHangoutDisconnect_.bind(this);
 };
 
 /** @enum {string} */
 remoting.It2MeHelperChannel.HangoutMessageTypes = {
   HELLO: 'hello',
   HELLO_RESPONSE: 'helloResponse',
   CONNECT: 'connect',
-  DISCONNECT: 'disconnect'
+  DISCONNECT: 'disconnect',
+  ERROR: 'error'
 };
 
 /** @enum {string} */
 remoting.It2MeHelperChannel.Features = {
   REMOTE_ASSISTANCE: 'remoteAssistance'
 };
 
 /** @enum {string} */
 remoting.It2MeHelperChannel.WebappMessageTypes = {
   SESSION_STATE_CHANGED: 'sessionStateChanged'
@@ skipping 27 matching lines @@
       case MessageTypes.HELLO:
         this.hangoutPort_.postMessage({
           method: MessageTypes.HELLO_RESPONSE,
           supportedFeatures: base.values(remoting.It2MeHelperChannel.Features)
         });
         return true;
     }
     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;
 };
 
 /**
  * Disconnect the existing connection to the helpee.
  *
  * @param {{method:string, data:Object.<string,*>}} message
  * @private
  */
@@ skipping 88 matching lines @@
       case MessageTypes.SESSION_STATE_CHANGED:
         var state = getNumberAttr(message, 'state');
         this.sessionState_ =
             /** @type {remoting.ClientSession.State} */ state;
         this.hangoutPort_.postMessage(message);
         return true;
     }
     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;
 };
 
 remoting.It2MeHelperChannel.prototype.unhookPorts_ = function() {
   if (this.webappPort_) {
     this.webappPort_.onMessage.removeListener(this.onWebappMessageRef_);
     this.webappPort_.onDisconnect.removeListener(this.onWebappDisconnectRef_);
     this.webappPort_.disconnect();
     this.webappPort_ = null;
   }
 
   if (this.hangoutPort_) {
     this.hangoutPort_.onMessage.removeListener(this.onHangoutMessageRef_);
     this.hangoutPort_.onDisconnect.removeListener(this.onHangoutDisconnectRef_);
     this.hangoutPort_.disconnect();
     this.hangoutPort_ = null;
   }
 
   if (this.onDisconnectCallback_) {
     this.onDisconnectCallback_(this);
     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
+  });
+};