Hangouts remote desktop part III - It2MeService

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
new file mode 100644
index 0000000000000000000000000000000000000000..7192e82729118cc01b9f15a94c3a1d9dbcef7b96
--- /dev/null
+++ b/remoting/webapp/background/it2me_helper_channel.js
@@ -0,0 +1,277 @@
+// 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).
+ *
+ * It runs in the background page and contains two chrome.runtime.Port objects,
+ * respresenting connections to the webapp and hangout, respectively.
+ *
+ * Connection is always initiated from Hangouts.
+ *
+ *   Hangout                     It2MeHelperChannel        Chrome Remote Desktop
+ *      |-----runtime.connect() ------>|                                |
+ *      |------connect message-------->|                                |
+ *      |                              |-------appLauncher.launch()---->|
+ *      |                              |<------runtime.connect()------- |
+ *      |                              |<-----sessionStateChanged------ |
+ *      |<----sessionStateChanged------|                                |
+ *
+ * Disconnection can be initiated from either side:
+ * 1. In the normal flow initiated from hangout
+ *    Hangout                    It2MeHelperChannel        Chrome Remote Desktop
+ *       |-----disconnect message------>|                               |
+ *       |<-sessionStateChanged(CLOSED)-|                               |
+ *       |                              |-----appLauncher.close()------>|
+ *
+ * 2. In the normal flow initiated from webapp
+ *    Hangout                    It2MeHelperChannel        Chrome Remote Desktop
+ *       |                              |<-sessionStateChanged(CLOSED)--|
+ *       |                              |<--------port.disconnect()-----|
+ *       |<--------port.disconnect()----|                               |
+ *
+ * 2. If hangout crashes
+ *    Hangout                    It2MeHelperChannel        Chrome Remote Desktop
+ *       |---------port.disconnect()--->|                               |
+ *       |                              |--------port.disconnect()----->|
+ *       |                              |------appLauncher.close()----->|
+ *
+ * 3. If webapp crashes
+ *    Hangout                    It2MeHelperChannel        Chrome Remote Desktop
+ *       |                              |<-------port.disconnect()------|
+ *       |<-sessionStateChanged(FAILED)-|                               |
+ *       |<--------port.disconnect()----|                               |
+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @param {remoting.AppLauncher} appLauncher
+ * @param {chrome.runtime.Port} hangoutPort
+ *     Represents an active connection to Hangouts.

[Jamie, 2014/08/13 01:44:52]

For consistency, param descriptions should start on the same line as the @param,
if possible. The indentation for subsequent lines is correct, however.

[kelvinp, 2014/08/13 23:44:10]

Done.

+ * @param {function(*)} onDisconnectCallback
+ *     Callback to notify when the connection is torn down.  IT2MeService uses
+ *     this callback to dispose of the channel object.
+ * @constructor
+ */
+remoting.It2MeHelperChannel =
+    function(appLauncher, hangoutPort, onDisconnectCallback) {
+
+  /**
+   * @type {remoting.AppLauncher}
+   * @private
+   */
+  this.appLauncher_ = appLauncher;
+
+  /**
+   * @type {chrome.runtime.Port}
+   * @private
+   */
+  this.hangoutPort_ = hangoutPort;
+
+  /**
+   * @type {chrome.runtime.Port}
+   * @private
+   */
+  this.webappPort_ = null;
+
+  /**
+   * @type {string}
+   * @private
+   */
+  this.instanceId_ = '';
+
+  /**
+   * @type {remoting.ClientSession.State}
+   * @private
+   */
+  this.sessionState_ = remoting.ClientSession.State.CONNECTING;
+  this.onDisconnectCallback_ = onDisconnectCallback;

[Jamie, 2014/08/13 01:44:52]

Unless it fits more logically here, I think it would be better to group it with
the other ctor parameters. It should also have a @private tag (if you can get
away without specifying its type, then that's fine).

[kelvinp, 2014/08/13 23:44:10]

Done.

[Jamie, 2014/08/14 00:25:06]

You haven't moved it to be with the other parameters? Was that an oversight, or
do you think it makes more sense to put it here? I generally copy the ctor
parameters to member variables at the top of the ctor, in the order they're
declared.

+
+  this.onWebappMessageRef_ = this.onWebappMessage_.bind(this);
+  this.onWebappDisconnectRef_ = this.onWebappDisconnect_.bind(this);
+  this.onHangoutMessageRef_ = this.onHangoutMessage_.bind(this);
+  this.onHangoutDisconnectRef_ = this.onHangoutDisconnect_.bind(this);
+};
+
+/** @enum {string} */
+remoting.It2MeHelperChannel.HangoutMessageTypes = {
+  CONNECT: 'connect',
+  DISCONNECT: 'disconnect'
+};
+
+/** @enum {string} */
+remoting.It2MeHelperChannel.WebappMessageTypes = {
+  SESSION_STATE_CHANGED: 'sessionStateChanged'
+};
+
+remoting.It2MeHelperChannel.prototype.init = function() {
+  this.hangoutPort_.onMessage.addListener(this.onHangoutMessageRef_);
+  this.hangoutPort_.onDisconnect.addListener(this.onHangoutDisconnectRef_);
+};
+
+/** @return {string} */
+remoting.It2MeHelperChannel.prototype.instanceId = function() {
+  return this.instanceId_;
+};
+
+/**
+ * @param {{method:string, data:Object.<string,*>}} message
+ * @return {boolean} whether the message is handled or not.

[Jamie, 2014/08/13 01:44:52]

@private, or no underscore, here and elsewhere.

[kelvinp, 2014/08/13 23:44:10]

Done.

+ */
+remoting.It2MeHelperChannel.prototype.onHangoutMessage_ = function(message) {
+  try {
+    var MessageTypes = remoting.It2MeHelperChannel.HangoutMessageTypes;
+    switch (message.method) {
+      case MessageTypes.CONNECT:
+        this.launchWebapp_(message);
+        return true;
+      case MessageTypes.DISCONNECT:
+        this.closeWebapp_(message);
+        return true;
+    }
+  } catch(e) {
+    var error = /** @type {Error} */ e;
+    console.error(error);
+    this.hangoutPort_.postMessage({
+      method: message.method + 'Response',
+      error: error.message
+    });
+  }
+  return false;
+};
+
+/**
+ * Disconnect the existing connection to the helpee.
+ *
+ * @param {{method:string, data:Object.<string,*>}} message
+ */
+remoting.It2MeHelperChannel.prototype.closeWebapp_ =
+    function(message) {
+  // TODO(kepoon): Closing the v2 app current doesn't disconnect the IT2me

[Jamie, 2014/08/13 01:44:52]

s/current/currently/

[Jamie, 2014/08/13 01:44:52]

Wrong user id :)

[kelvinp, 2014/08/13 23:44:09]

Done.

[kelvinp, 2014/08/13 23:44:10]

Done.

+  // session crbug.com/402137, so send an explicit notification to the hangout.

[Jamie, 2014/08/13 01:44:53]

Parentheses around the bug.

[kelvinp, 2014/08/13 23:44:09]

Done.

+  this.sessionState_ = remoting.ClientSession.State.CLOSED;
+  this.hangoutPort_.postMessage({
+    method: 'sessionStateChanged',
+    state: this.sessionState_
+  });
+  this.appLauncher_.close(this.instanceId_);

[Jamie, 2014/08/13 01:44:52]

I'm not sure it makes much difference, but if the explicit notification is to
work around the lack of an onUnload handler, shouldn't it be deferred until the
close() promise fulfills?

[kelvinp, 2014/08/13 23:44:09]

this.appLauncher_.close will disconnect the port and cause the disconnect
callback to fire.  The background page will thought the webapp has crashed.

+};
+
+/**
+ * Launches the web app.
+ * @param {{method:string, data:Object.<string,*>}} message

[Jamie, 2014/08/13 01:44:52]

Blank line between description and annotations.

[kelvinp, 2014/08/13 23:44:10]

Done.

+ */
+remoting.It2MeHelperChannel.prototype.launchWebapp_ =
+    function(message) {
+  var accessCode = getStringAttr(message, 'accessCode');
+  if (!accessCode) {
+    throw new Error('Access code is missing');
+  }
+
+  // Launch the webapp.
+  this.appLauncher_.launch({
+    mode: 'hangout',
+    accessCode: accessCode
+  }).then(
+    /**
+     * @this {remoting.It2MeHelperChannel}
+     * @param {string} instanceId
+     */
+    function(instanceId){
+      this.instanceId_ = String(instanceId);

[Jamie, 2014/08/13 01:44:52]

Do you need String here?

[kelvinp, 2014/08/13 23:44:09]

Done.

+  }.bind(this));

[Jamie, 2014/08/13 01:44:52]

Closing brace should be aligned with 'function'.

[kelvinp, 2014/08/13 23:44:09]

Done.

+};
+
+remoting.It2MeHelperChannel.prototype.onHangoutDisconnect_ =
+    function() {

[Jamie, 2014/08/13 01:44:52]

No need for line break I don't think.

[kelvinp, 2014/08/13 23:44:09]

Done.

+  this.appLauncher_.close(this.instanceId_);

[Jamie, 2014/08/13 01:44:52]

In the non-error case, this is going to result in two calls to close(). Since
the interface doesn't mandate that it be idempotent, that could be a problem.

It's probably sufficient to add a comment explaining that this is benign, but I
wonder if there's a cleaner solution?

[kelvinp, 2014/08/13 23:44:10]

This won't need to two calls to close.  The disconnected event will only fire if
the port is disconnected on the remote side.

+  this.unhookPorts_();
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ * @param {string} id

[Jamie, 2014/08/13 01:44:53]

I think these parameters would benefit from documentation: what's on the other
end of the port, and what does id identify?

[kelvinp, 2014/08/13 23:44:09]

Done.

+ */
+remoting.It2MeHelperChannel.prototype.onWebappConnect = function(port, id) {
+  base.debug.assert(id === this.instanceId_);
+  base.debug.assert(this.hangoutPort_ !== null);
+
+  // Hook listeners.
+  port.onMessage.addListener(this.onWebappMessageRef_);
+  port.onDisconnect.addListener(this.onWebappDisconnectRef_);
+  this.webappPort_ = port;
+};
+
+/** @param {chrome.runtime.Port} port */
+remoting.It2MeHelperChannel.prototype.onWebappDisconnect_ = function(port) {
+  // If the port got disconnected while the session is still connected,

[Jamie, 2014/08/13 01:44:52]

Please specify which port.

[kelvinp, 2014/08/13 23:44:09]

Done.

+  // treat it as an error.
+  var States = remoting.ClientSession.State;
+  if (this.sessionState_ === States.CONNECTING ||
+      this.sessionState_ === States.CONNECTED) {
+    this.sessionState_ = States.FAILED;
+    this.hangoutPort_.postMessage({
+      method: 'sessionStateChanged',
+      state: this.sessionState_
+    });
+  }
+  this.unhookPorts_();
+};
+
+/**
+ * @param {{method:string, data:Object.<string,*>}} message
+ */
+remoting.It2MeHelperChannel.prototype.onWebappMessage_ = function(message) {
+  try {
+    console.log('It2MeHelperChannel id:=' + this.instanceId_ +

[kelvinp, 2014/08/13 00:08:48]

This statement will only log to the console of background.html.  IMHO, I think
logging inter-component messages makes it easier to debug protocol errors.

[Jamie, 2014/08/13 01:44:52]

As long as the protocol isn't too chatty, that's fine, although I'd go with
either a colon or an equals sign, not both.

+                ' incoming message method:=' + message.method);
+    var MessageTypes = remoting.It2MeHelperChannel.WebappMessageTypes;
+    switch (message.method) {
+      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);

[Jamie, 2014/08/13 01:44:52]

s/:=/: /

[kelvinp, 2014/08/13 23:44:09]

Done.

+  } catch(e) {
+    var error = /** @type {Error} */ e;
+    console.error(error);
+    this.webappPort_.postMessage({
+      method: message.method + 'Response',
+      error: error.message
+    });
+  }
+  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;
+  }
+};