Hangout remote desktop part II - background.html and AppLauncher

remoting/webapp/background/it2me_service.js

Index: remoting/webapp/background/it2me_service.js
diff --git a/remoting/webapp/background/it2me_service.js b/remoting/webapp/background/it2me_service.js
new file mode 100644
index 0000000000000000000000000000000000000000..185e9898066fddb8e926544c480fe4062720aacd
--- /dev/null
+++ b/remoting/webapp/background/it2me_service.js
@@ -0,0 +1,171 @@
+// 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
+ * It2MeService listens for incoming connections request from the hangout

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

s/connections request/connection requests/

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

Done.

+ * and webapp and creates a It2MeHelperChannel between them.
+ * It supports multiple concurrent helper sessions.
+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @param {remoting.AppLauncher} appLauncher
+ * @constructor
+ */
+remoting.It2MeService = function(appLauncher) {
+  /**
+   * @private
+   * @type {remoting.AppLauncher}
+   */
+  this.appLauncher_ = appLauncher;
+
+  /**
+   * @private
+   * @type {Array.<remoting.It2MeHelperChannel>}
+   */
+  this.helpers_ = [];
+
+  /** @private */
+  this.helpee_ = null;
+
+  this.onWebappConnectRef_ = this.onWebappConnect_.bind(this);
+  this.onMessageExternalRef_ = this.onMessageExternal_.bind(this);
+  this.onConnectExternalRef_ = this.onConnectExternal_.bind(this);
+};
+
+remoting.It2MeService.ConnectionTypes = {
+  HELPER_HANGOUT: 'it2me.helper.hangout',
+  HELPEE_HANGOUT: 'it2me.helpee.hangout',
+  HELPER_WEBAPP: 'it2me.helper.webapp'

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

Are these names global? If so, then they should probably something com.google.*
or org.chromium.*

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

Those names are only valid within our extension.

+};
+
+remoting.It2MeService.prototype.init = function() {
+  chrome.runtime.onConnect.addListener(this.onWebappConnectRef_);
+  chrome.runtime.onMessageExternal.addListener(this.onMessageExternalRef_);
+  chrome.runtime.onConnectExternal.addListener(this.onConnectExternalRef_);
+};
+
+remoting.It2MeService.prototype.dispose = function() {
+  chrome.runtime.onConnect.removeListener(this.onWebappConnectRef_);
+  chrome.runtime.onMessageExternal.removeListener(
+      this.onMessageExternalRef_);
+  chrome.runtime.onConnectExternal.removeListener(
+      this.onConnectExternalRef_);
+};
+
+/**
+ * This function is called when a runtime message is received from an external
+ * web page (hangout) or extension.
+ * @param {{method:string, data:Object.<string,*>}} message

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

Nit: Blank line between function comment and type annotations.

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

Acknowledged.

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

Done.

+ * @param {chrome.runtime.MessageSender} sender
+ * @param {Function} sendResponse
+ */
+remoting.It2MeService.prototype.onMessageExternal_ =
+    function(message, sender, sendResponse) {
+  /**
+   * @param {Object} value
+   * @param {string=} opt_error
+   */
+  function doSendResponse(value, opt_error) {
+    var errorMessage = opt_error || chrome.runtime.lastError.message;
+    sendResponse({'value': value, 'error': errorMessage});
+  }

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

I don't think this helper actually helps much, since you only call it twice;
once with only a value and once with only an error. Also, do you need a separate
error field on every message, as opposed to a dedicated "error" message?

+
+  try {
+    var method = message.method;
+    var isHandled = false;
+
+    if (method == 'hello') {
+      // The hello message is used by hangouts to detect whether the app is
+      // installed and what features are supported.
+      doSendResponse({'supportedFeatures': ['it2me']});
+      return true;
+    }
+    throw new Error('Unknown method: ' + method);
+  } catch (e) {
+    var error = /** @type {Error} */ e;
+    doSendResponse(null, error.name + ': ' + error.message);
+  }
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ */
+remoting.It2MeService.prototype.onConnectExternal_ = function(port) {
+  var ConnectionTypes = remoting.It2MeService.ConnectionTypes;
+  try {
+    switch (port.name) {

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

Presumably port.name can be spoofed. Can/should we have a more robust check? For
example, the origin of the other end?

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

Yes, not every webpage can connect to our list.  Only the apps white-listed in
the manifest can connect.

+      case ConnectionTypes.HELPER_HANGOUT:
+        this.handleExternalHelperConnection_(port);
+        return true;
+      default:
+        throw new Error('Unsupported port - ' + port.name);
+    }
+  } catch (e) {
+    port.disconnect();
+  }
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ */
+remoting.It2MeService.prototype.onWebappConnect_ = function(port) {
+  try {
+    console.log('Incoming helper connection from webapp.');
+
+    // The senderId (tabId or windowId) of the webapp is embedded in the port
+    // name with the format port_name@senderId.
+    var parts = port.name.split('@');
+    var portName = parts[0];
+    var senderId = parts[1];
+    var ConnectionTypes = remoting.It2MeService.ConnectionTypes;
+    if (portName === ConnectionTypes.HELPER_WEBAPP && senderId !== undefined) {
+      for (var i = 0; i < this.helpers_.length; i++) {
+        var helper = this.helpers_[i];
+        if (helper.instanceId() === senderId) {
+          helper.onWebappConnect(port, senderId);
+          return;
+        }
+      }
+    }
+    throw new Error('No matching hangout connection found for ' + port.name);
+  } catch (e) {
+    var error = /** @type {Error} */ e;
+    console.error(error);
+    port.disconnect();
+  }
+};
+
+/**
+ * @param {remoting.It2MeHelperChannel} helper
+ */
+remoting.It2MeService.prototype.onHelperChannelDisconnected = function (helper){

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

Nit: Space before '{'

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

Is this a public method? If so, it should be moved above the private methods. If
not, it should end with an underscore.

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

Done.

+  for (var i = 0; i < this.helpers_.length; i++) {
+    if (helper === this.helpers_[i]) {
+      this.helpers_.splice(i, 1);
+    }
+  }
+};
+
+/**
+ * @param {chrome.runtime.Port} port
+ */
+remoting.It2MeService.prototype.handleExternalHelperConnection_ =
+    function(port) {
+  if (this.helpee_) {
+    console.error('An existing helpee connection is in process.');

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

This error should probably be more prominent. Is anything displayed to the user
in this case?

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

The hangout code should guarantee this should never happens.  

+    port.disconnect();
+  }
+
+  console.log('Incoming helper connection from Hangout');
+  var helper = new remoting.It2MeHelperChannel(
+      this.appLauncher_, port, this.onHelperChannelDisconnected.bind(this));
+  helper.init();
+  this.helpers_.push(helper);
+};