Hangout remote desktop part II - background.html and AppLauncher

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..a3868647e9c6ef830f3766bd5bb0d5ca7ff66d20
--- /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 relay messages between the hangout web page (hangout) and

[Jamie, 2014/08/12 02:24:57]

s/hangout/Hangouts/

[Jamie, 2014/08/12 02:24:58]

s/relay/relays/

+ * the chrome remote desktop (webapp) for the helper (The person who is giving

[Jamie, 2014/08/12 02:24:58]

s/The/the/

[Jamie, 2014/08/12 02:24:58]

Chrome Remote Desktop (capitalization).

[kelvinp, 2014/08/12 21:42:39]

Done.

[kelvinp, 2014/08/12 21:42:40]

Done.

+ * remoting assistance).

[Jamie, 2014/08/12 02:24:58]

s/remoting/remote/

[kelvinp, 2014/08/12 21:42:40]

Done.

+ *
+ * It contains two port object, with each representing a two-way connection
+ * between the background script and the webapp/hangout.

[Jamie, 2014/08/12 02:24:58]

I think this whole paragraph can be more succinctly summarised by:

"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 Hangout.

[Jamie, 2014/08/12 02:24:58]

s/Hangout/the hangout/

[kelvinp, 2014/08/12 21:42:39]

Done.

+ *
+ * Hangout                  It2MeHelperChannel
+ *    |-----runtime.connect() ----->|
+ *    |------connect message------->|---------launch----> Chrome remote desktop

[Jamie, 2014/08/12 02:24:58]

Even though CRD is not running until this step, I think it would be clearer to
move it up to the same header line as the hangout and helper channel.

[kelvinp, 2014/08/12 21:42:41]

Done.

+ *    |                             |<------runtime.connect()-------|
+ *    |<----sessionStateChanged-----|<-----sessionStateChanged------|
+ *    |                             |                               |
+ *
+ * Disconnection can be initiated from either side:
+ * 1. In the normal flow initiated from hangout
+ *    a) hangout ---- ------ disconnect message ---------> It2MeHelperChannel
+ *    b) hangout <---------- sessionStateChanged(CLOSED) - It2MeHelperChannel
+ *    c) It2MeHelperChannel- window.close() -----------> Webapp

[Jamie, 2014/08/12 02:24:58]

Can these diagrams be made to fit the layout above? In other words, hangout on
the left, channel in the middle and CRD on the right?

[kelvinp, 2014/08/12 21:42:40]

Done.

+ *
+ * 2. In the normal flow initiated from webapp
+ *    a) webapp ------------ sessionStateChanged(CLOSED)-> It2MeHelperChannel
+ *    b) webapp ------------ port.disconnect() ----------> It2MeHelperChannel
+ *    c) It2MeHelperChannel- port.disconnect() ----------> hangout
+ *
+ * 2. If hangout crashes
+ *    a) hangout ------------> port.disconnect()-----> It2MeHelperChannel
+ *    b) It2MeHelperChannel -> port.disconnect()-----> webapp
+ *    b) It2MeHelperChannel -> window.close()--------> webapp
+ *
+ * 3. If webapp crashes
+ *    a) webapp -------------> port.disconnect()-----------> It2MeHelperChannel
+ *    b) It2MeHelperChannel -> sessionStateChanged(FAILED)-> hangout
+ *    b) It2MeHelperChannel -> port.disconnect()-----------> hangout
+ *

[Jamie, 2014/08/12 02:24:57]

Nit: Unnecessary blank line.

[kelvinp, 2014/08/12 21:42:39]

Done.

+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @param {remoting.AppLauncher} appLauncher
+ * @param {chrome.runtime.Port} hangoutPort

[Jamie, 2014/08/12 02:24:57]

Please add a description for this parameter.

[kelvinp, 2014/08/12 21:42:40]

Done.

+ * @param {function(*)} onDisconnectCallback
+ *     Callback to notify when the

[Jamie, 2014/08/12 02:24:58]

Formatting.

[kelvinp, 2014/08/12 21:42:39]

Done.

+ *     connection is torn down.  IT2MeServices uses this callback to dispose the

[Jamie, 2014/08/12 02:24:57]

s/Services/Service/

[Jamie, 2014/08/12 02:24:58]

s/dispose/dispose of/

[kelvinp, 2014/08/12 21:42:40]

Done.

+ *     channel object.
+ * @constructor
+ */
+remoting.It2MeHelperChannel =
+    function(appLauncher, hangoutPort, onDisconnectCallback) {
+
+  /**
+   * @private

[Jamie, 2014/08/12 02:24:57]

Nit: @private should come after the type (for consistency with the rest of the
code-base).

[kelvinp, 2014/08/12 21:42:39]

Done.

+   * @type {remoting.AppLauncher}
+   */
+  this.appLauncher_ = appLauncher;
+
+  /**
+   * @private
+   * @type {chrome.runtime.Port}
+   */
+  this.hangoutPort_ = hangoutPort;
+
+  /**
+   * @private
+   * @type {chrome.runtime.Port}
+   */
+  this.webappPort_ = null;
+
+  /**
+   * @private
+   * @type {string}
+   */
+  this.instanceId_ = "";

[Jamie, 2014/08/12 02:24:57]

Nit: Single-quotes for strings.

+
+  /**
+   * @private
+   * @type {remoting.ClientSession.State}
+   */
+  this.sessionState_ = remoting.ClientSession.State.CONNECTING;
+
+  this.onWebappMessageRef_ = this.onWebappMessage_.bind(this);
+  this.onWebappDisconnectRef_ = this.onWebappDisconnect_.bind(this);
+  this.onHangoutMessageRef_ = this.onHangoutMessage_.bind(this);
+  this.onHangoutDisconnectRef_ = this.onHangoutDisconnect_.bind(this);
+  this.onDisconnectCallback_ = onDisconnectCallback;

[Jamie, 2014/08/12 02:24:57]

I think it's clearer to save all the ctor parameters together at the top of the
ctor, then initialize any other members.

[kelvinp, 2014/08/12 21:42:39]

Done.

+};
+
+remoting.It2MeHelperChannel.HangoutMessageTypes = {

[Jamie, 2014/08/12 02:24:58]

Add @enum annotation?

[kelvinp, 2014/08/12 21:42:40]

Done.

+  CONNECT: 'connect',
+  DISCONNECT: 'disconnect'
+};
+
+remoting.It2MeHelperChannel.WebappMessageTypes = {
+  SESSION_STATE_CHANGED: 'sessionStateChanged'
+};
+
+remoting.It2MeHelperChannel.prototype.init = function() {

[Jamie, 2014/08/12 02:24:57]

Please add some doc comments to this and other methods.

+  this.hangoutPort_.onMessage.addListener(this.onHangoutMessageRef_);
+  this.hangoutPort_.onDisconnect.addListener(this.onHangoutDisconnectRef_);
+};
+
+/** @return {string} */
+remoting.It2MeHelperChannel.prototype.instanceId = function() {
+  return this.instanceId_;
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ * @param {Object=} opt_value
+ * @param {string=} opt_error
+ */
+remoting.It2MeHelperChannel.prototype.sendResponse_ =
+    function(port, opt_value, opt_error) {
+  var errorMessage = (opt_error) ? opt_error : chrome.runtime.lastError.message;
+  port.postMessage({'value': opt_value, 'error': errorMessage});
+};
+
+/**
+ * @param {{method:string, data:Object.<string,*>}} message
+ * @return {boolean} whether the message is handled or not.
+ */
+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.sendResponse_(this.hangoutPort_, null, 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
+  // session crbug.com/402137.

[Jamie, 2014/08/12 02:24:58]

It's not clear what this comment is trying to say. I suggest something like:

TODO(kelvinp): Closing the v2 app current doesn't disconnect the IT2me session
(crbug.com/402137), so send an explicit notification to the hangout.

[kelvinp, 2014/08/12 21:42:40]

Done.

+  this.sessionState_ = remoting.ClientSession.State.CLOSED;
+  this.hangoutPort_.postMessage({
+    method: 'sessionStateChanged',
+    state: this.sessionState_
+  });
+  this.appLauncher_.close(this.instanceId_);
+};
+
+/**
+ * @param {{method:string, data:Object.<string,*>}} message
+ */
+remoting.It2MeHelperChannel.prototype.launchWebapp_ =
+    function(message) {
+  var accessCode = getStringAttr(message, 'accessCode');
+  if (!accessCode) {
+    throw new Error('AccessCode is missing');

[Jamie, 2014/08/12 02:24:58]

s/Access/access/

[kelvinp, 2014/08/12 21:42:39]

Done.

+  }
+
+  // Launch the webapp.
+  this.appLauncher_.launch({
+    mode: 'hangout',
+    accessCode: accessCode
+  }).then(
+    /**
+     * @this {remoting.It2MeHelperChannel}
+     * @param {string} instanceId
+     */
+    function(instanceId){
+      this.instanceId_ = instanceId;
+  }.bind(this));
+};
+
+remoting.It2MeHelperChannel.prototype.onHangoutDisconnect_ =
+    function() {
+  // Runtime port do not fire disconnect events synchronously so

[Jamie, 2014/08/12 02:24:57]

s/Runtime port do/chrome.runtime.Port does/

[kelvinp, 2014/08/12 21:42:41]

Done.

+  // appLauncher_.close won't invoke onWebappDiscconect

[Jamie, 2014/08/12 02:24:57]

s/Discconect/Disconnect/

[Jamie, 2014/08/12 02:24:58]

I don't understand what this comment is trying to tell me. Will
onWebappDisconnect never be called? Do we need the call to unhookPorts_ as a
consequence?

[kelvinp, 2014/08/12 21:42:39]

Done.

[kelvinp, 2014/08/12 21:42:40]

Comments removed

+  this.appLauncher_.close(this.instanceId_);
+  this.unhookPorts_();
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ * @param {string} id
+ */
+remoting.It2MeHelperChannel.prototype.onWebappConnect = function(port, id) {
+  base.debug.assert(id === this.instanceId_);
+  base.debug.assert(Boolean(this.hangoutPort_));

[Jamie, 2014/08/12 02:24:57]

I think "this.hangoutPort_ != null" would be clearer.

[kelvinp, 2014/08/12 21:42:39]

Done.

+
+  // 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,
+  // 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_ +
+                ' incoming message method:=' + message.method);

[Jamie, 2014/08/12 02:24:58]

Can this be removed? Console spam makes it harder to diagnose problems.

[kelvinp, 2014/08/12 21:42:40]

This will only log to the console of the background page.  I think logging
inter-component messages actually helps debugging. 

+    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 messages.');

[Jamie, 2014/08/12 02:24:57]

s/messages/message/

Also, include message.method in the error.

[kelvinp, 2014/08/12 21:42:38]

Done.

+  } catch(e) {
+    var error = /** @type {Error} */ e;
+    console.error(error);
+    this.sendResponse_(this.hangoutPort_, null, error.message);
+  }
+  return false;
+};
+
+

[Jamie, 2014/08/12 02:24:57]

Nit: Unnecessary blank line.

[kelvinp, 2014/08/12 21:42:39]

Done.

+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;
+  }
+};