Move ProtocolExtensionManager from SessionConnector into its own class

This CL moves protocolExtensionManager from SessionConnector into its own class.
The ProtcolExtensionManager will be exposed via the ClientPlugin.

The higher level goal is to remove functionality from the SessionConnecctor
so that it can be removed altogether in a future CL.

BUG=474766

Review URL: https://codereview.chromium.org/1067133002

Cr-Commit-Position: refs/heads/master@{#324167}

remoting/remoting_webapp_files.gypi

@@ -69,6 +69,7 @@
       'webapp/base/js/base_event_hook_unittest.js',
       'webapp/base/js/base_inherits_unittest.js',
       'webapp/base/js/ipc_unittest.js',
+      'webapp/base/js/protocol_extension_manager_unittest.js',
       'webapp/crd/js/apps_v2_migration_unittest.js',
       'webapp/crd/js/desktop_viewport_unittest.js',
       'webapp/crd/js/dns_blackhole_checker_unittest.js',
@@ -170,6 +171,7 @@
       'webapp/base/js/input_dialog.js',
       'webapp/base/js/ipc.js',
       'webapp/base/js/platform.js',
+      'webapp/base/js/protocol_extension_manager.js',
       'webapp/base/js/protocol_extension.js',
       'webapp/crd/js/apps_v2_migration.js',
       'webapp/crd/js/error.js',

remoting/webapp/app_remoting/js/app_remoting.js

@@ -214,7 +214,7 @@ remoting.AppRemoting.prototype.onConnected_ = function(connectionInfo) {
             JSON.stringify({fullName: userInfo.name}));
       });
 
-  this.sessionConnector_.registerProtocolExtension(this);
+  connectionInfo.plugin().extensions().register(this);
 
   this.connectedView_ = new remoting.AppConnectedView(
       document.getElementById('client-container'), connectionInfo);

remoting/webapp/base/js/protocol_extension_manager.js

+// Copyright 2015 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.
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+(function() {
+
+'use strict';
+
+/**
+ * @param {function(string, string)} sendExtensionMessage Callback used to
+ *     send an extension message to the plugin.
+ * @constructor
+ * @implements {base.Disposable}
+ */
+remoting.ProtocolExtensionManager = function(sendExtensionMessage) {
+  /** @private */
+  this.sendExtensionMessage_ = sendExtensionMessage;
+  /** @private {Object<string,remoting.ProtocolExtension>} */
+  this.protocolExtensions_ = {};
+
+  /**
+   * True once a session has been created and we've started the extensions.
+   * This is used to immediately start any extensions that are registered
+   * after the CONNECTED state change.
+   * @private
+   */
+  this.protocolExtensionsStarted_ = false;
+};
+
+remoting.ProtocolExtensionManager.prototype.dispose = function() {
+  this.sendExtensionMessage_ = base.doNothing;
+  this.protocolExtensions_ = {};
+};
+
+/** Called by the plugin when the session is connected */
+remoting.ProtocolExtensionManager.prototype.start = function() {
+  base.debug.assert(!this.protocolExtensionsStarted_);
+  for (var type in this.protocolExtensions_) {
+    this.startExtension_(type);
+  }
+  this.protocolExtensionsStarted_ = true;
+};
+
+/**
+ * @param {remoting.ProtocolExtension} extension
+ * @return {boolean} true if the extension is successfully registered.
+ */
+remoting.ProtocolExtensionManager.prototype.register =
+    function(extension) {
+  var types = extension.getExtensionTypes();
+
+  // Make sure we don't have an extension of that type already registered.
+  for (var i=0, len=types.length; i < len; i++) {
+    if (types[i] in this.protocolExtensions_) {
+      console.error(
+          'Attempt to register multiple extensions of the same type: ', type);
+      return false;
+    }
+  }
+
+  for (var i=0, len=types.length; i < len; i++) {
+    var type = types[i];
+    this.protocolExtensions_[type] = extension;
+    if (this.protocolExtensionsStarted_) {
+      this.startExtension_(type);
+    }
+  }
+  return true;
+};
+
+/**
+ * @param {string} type
+ * @private
+ */
+remoting.ProtocolExtensionManager.prototype.startExtension_ =
+    function(type) {
+  var extension = this.protocolExtensions_[type];
+  extension.startExtension(this.sendExtensionMessage_);
+};
+
+/**
+ * Called when an extension message needs to be handled.
+ *
+ * @param {string} type The type of the extension message.
+ * @param {string} data The payload of the extension message.
+ * @return {boolean} Return true if the extension message was recognized.
+ */
+remoting.ProtocolExtensionManager.prototype.onProtocolExtensionMessage =
+    function(type, data) {
+  if (type == 'test-echo-reply') {
+    console.log('Got echo reply: ' + data);
+    return true;
+  }
+
+  var message = base.jsonParseSafe(data);
+  if (typeof message != 'object') {
+    console.error('Error parsing extension json data: ' + data);
+    return false;
+  }
+
+  if (type in this.protocolExtensions_) {
+    /** @type {remoting.ProtocolExtension} */
+    var extension = this.protocolExtensions_[type];
+    var handled = false;
+    try {
+      handled = extension.onExtensionMessage(type, message);
+    } catch (/** @type {*} */ err) {
+      console.error('Failed to process protocol extension ' + type +
+                    ' message: ' + err);
+    }
+    if (handled) {
+      return true;
+    }
+  }
+
+  return false;
+};
+
+})();

remoting/webapp/base/js/protocol_extension_manager_unittest.js

+// Copyright 2015 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
+ */
+
+(function() {
+
+'use strict';
+
+/** @type {remoting.ProtocolExtensionManager} */
+var extensionManager;
+
+/** @type {(sinon.Spy|function(string, string))} */
+var sendClientMessage;
+
+/**
+ * @constructor
+ * @param {Array<string>} types
+ * @implements {remoting.ProtocolExtension}
+ */
+var DummyExtension = function(types) {
+  /** @private {?function(string, string)} */
+  this.sendMessageToHost_ = null;
+  /** @private */
+  this.types_ = types;
+};
+
+DummyExtension.prototype.getExtensionTypes = function() {
+  return this.types_.slice(0);
+}
+
+/**
+ * @param {function(string,string)} sendMessageToHost Callback to send a message
+ *     to the host.
+ */
+DummyExtension.prototype.startExtension = function(sendMessageToHost) {
+  this.sendMessageToHost_ = sendMessageToHost;
+};
+
+/**
+ * Called when an extension message of a matching type is received.
+ *
+ * @param {string} type The message type.
+ * @param {Object} message The parsed extension message data.
+ * @return {boolean} True if the extension message was handled.
+ */
+DummyExtension.prototype.onExtensionMessage = function(type, message){
+  return this.types_.indexOf(type) !== 1;
+};
+
+
+QUnit.module('ProtocolExtensionManager', {
+  beforeEach: function() {
+    sendClientMessage = /** @type {function(string, string)} */ (sinon.spy());
+    extensionManager = new remoting.ProtocolExtensionManager(sendClientMessage);
+  },
+  afterEach: function() {
+  }
+});
+
+QUnit.test('should route message to extension by type', function(assert) {
+  var extension = new DummyExtension(['type1', 'type2']);
+  var onExtensionMessage = /** @type {(sinon.Spy|function(string, string))} */ (
+      sinon.spy(extension, 'onExtensionMessage'));
+  assert.ok(extensionManager.register(extension));
+  extensionManager.start();
+
+  extensionManager.onProtocolExtensionMessage('type1', '{"message": "hello"}');
+  assert.ok(onExtensionMessage.called);
+  onExtensionMessage.reset();
+
+  extensionManager.onProtocolExtensionMessage('type2', '{"message": "hello"}');
+  assert.ok(onExtensionMessage.called);
+  onExtensionMessage.reset();
+
+  extensionManager.onProtocolExtensionMessage('type3', '{"message": "hello"}');
+  assert.ok(!onExtensionMessage.called);
+  onExtensionMessage.reset();
+});
+
+
+QUnit.test('should not register extensions of the same type', function(assert) {
+  var extension1 = new DummyExtension(['type1']);
+  var extension2 = new DummyExtension(['type1']);
+
+  var onExtensionMessage1 = /** @type {(sinon.Spy|function(string, string))} */(
+      sinon.spy(extension1, 'onExtensionMessage'));
+  var onExtensionMessage2 = /** @type {(sinon.Spy|function(string, string))} */(
+      sinon.spy(extension2, 'onExtensionMessage'));
+
+  assert.ok(extensionManager.register(extension1));
+  assert.ok(!extensionManager.register(extension2));
+  extensionManager.start();
+
+  extensionManager.onProtocolExtensionMessage('type1', '{"message": "hello"}');
+  assert.ok(onExtensionMessage1.called);
+  assert.ok(!onExtensionMessage2.called);
+});
+
+QUnit.test('should handle extensions registration after it is started',
+    function(assert) {
+  var extension = new DummyExtension(['type']);
+
+  var onExtensionMessage = /** @type {(sinon.Spy|function(string, string))} */(
+      sinon.spy(extension, 'onExtensionMessage'));
+
+  extensionManager.start();
+  assert.ok(extensionManager.register(extension));
+
+  extensionManager.onProtocolExtensionMessage('type', '{"message": "hello"}');
+  assert.ok(onExtensionMessage.called);
+});
+
+})();

remoting/webapp/crd/js/client_plugin.js

@@ -23,6 +23,11 @@ remoting.ClientPlugin = function() {};
  */
 remoting.ClientPlugin.prototype.hostDesktop = function() {};
 
+/**
+ * @return {remoting.ProtocolExtensionManager}
+ */
+remoting.ClientPlugin.prototype.extensions = function() {};
+
 /**
  * @return {HTMLElement} The DOM element representing the remote session.
  */
@@ -230,14 +235,6 @@ remoting.ClientPlugin.ConnectionEventHandler.prototype.onConnectionReady =
 remoting.ClientPlugin.ConnectionEventHandler.prototype.onSetCapabilities =
     function(capabilities) {};
 
-/**
- * @param {string} type
- * @param {string} data
- */
-remoting.ClientPlugin.ConnectionEventHandler.prototype.onExtensionMessage =
-    function(type, data) {};
-
-
 /**
  * @interface
  */

remoting/webapp/crd/js/client_plugin_impl.js

@@ -93,9 +93,14 @@ remoting.ClientPluginImpl = function(container,
     window.setTimeout(this.showPluginForClickToPlay_.bind(this), 500);
   }
 
+  /** @private */
   this.hostDesktop_ = new remoting.ClientPlugin.HostDesktopImpl(
       this, this.postMessage_.bind(this));
 
+  /** @private */
+  this.extensions_ = new remoting.ProtocolExtensionManager(
+      this.sendClientMessage.bind(this));
+
   /** @private {remoting.CredentialsProvider} */
   this.credentials_ = null;
 
@@ -226,7 +231,11 @@ remoting.ClientPluginImpl.prototype.handleMessageMethod_ = function(message) {
       var error = remoting.ClientSession.ConnectionError.fromString(
           base.getStringAttr(message.data, 'error'));
       handler.onConnectionStatusUpdate(state, error);
-
+      // TODO(kelvinp): Refactor the ClientSession.State into its own file as
+      // the plugin should not depend on ClientSession.
+      if (state === remoting.ClientSession.State.CONNECTED) {
+        this.extensions_.start();
+      }
     } else if (message.method == 'onRouteChanged') {
       var channel = base.getStringAttr(message.data, 'channel');
       var connectionType = base.getStringAttr(message.data, 'connectionType');
@@ -242,10 +251,6 @@ remoting.ClientPluginImpl.prototype.handleMessageMethod_ = function(message) {
           base.getStringAttr(message.data, 'capabilities'));
       handler.onSetCapabilities(capabilities);
 
-    } else if (message.method == 'extensionMessage') {
-      var extMsgType = base.getStringAttr(message.data, 'type');
-      var extMsgData = base.getStringAttr(message.data, 'data');
-      handler.onExtensionMessage(extMsgType, extMsgData);
     }
   }
 
@@ -372,6 +377,11 @@ remoting.ClientPluginImpl.prototype.handleMessageMethod_ = function(message) {
       this.debugRegionHandler_(
           /** @type {{rects: Array<(Array<number>)>}} **/(message.data));
     }
+  } else if (message.method == 'extensionMessage') {
+    var extMsgType = base.getStringAttr(message.data, 'type');
+    var extMsgData = base.getStringAttr(message.data, 'data');
+    this.extensions_.onProtocolExtensionMessage(extMsgType, extMsgData);
+
   }
 };
 
@@ -383,6 +393,9 @@ remoting.ClientPluginImpl.prototype.dispose = function() {
     this.plugin_.parentNode.removeChild(this.plugin_);
     this.plugin_ = null;
   }
+
+  base.dispose(this.extensions_);
+  this.extensions_ = null;
 };
 
 /**
@@ -791,6 +804,10 @@ remoting.ClientPluginImpl.prototype.hostDesktop = function() {
   return this.hostDesktop_;
 };
 
+remoting.ClientPluginImpl.prototype.extensions = function() {
+  return this.extensions_;
+};
+
 /**
  * If we haven't yet received a "hello" message from the plugin, change its
  * size so that the user can confirm it if click-to-play is enabled, or can

remoting/webapp/crd/js/client_session.js

@@ -35,17 +35,13 @@ remoting.ACCESS_TOKEN_RESEND_INTERVAL_MS = 15 * 60 * 1000;
  * @param {remoting.ClientPlugin} plugin
  * @param {remoting.Host} host The host to connect to.
  * @param {remoting.SignalStrategy} signalStrategy Signal strategy.
- * @param {function(string, string):boolean} onExtensionMessage The handler for
- *     protocol extension messages. Returns true if a message is recognized;
- *     false otherwise.
  *
  * @constructor
  * @extends {base.EventSourceImpl}
  * @implements {base.Disposable}
  * @implements {remoting.ClientPlugin.ConnectionEventHandler}
  */
-remoting.ClientSession = function(plugin, host, signalStrategy,
-                                  onExtensionMessage) {
+remoting.ClientSession = function(plugin, host, signalStrategy) {
   base.inherits(this, base.EventSourceImpl);
 
   /** @private */
@@ -80,9 +76,6 @@ remoting.ClientSession = function(plugin, host, signalStrategy,
    */
   this.logHostOfflineErrors_ = true;
 
-  /** @private {function(string, string):boolean} */
-  this.onExtensionMessageHandler_ = onExtensionMessage;
-
   /** @private {remoting.ClientPlugin} */
   this.plugin_ = plugin;
   plugin.setConnectionEventHandler(this);
@@ -511,14 +504,6 @@ remoting.ClientSession.prototype.onSetCapabilities = function(capabilities) {
   }
 };
 
-/**
- * @param {string} type
- * @param {string} data
- */
-remoting.ClientSession.prototype.onExtensionMessage = function(type, data) {
-  this.onExtensionMessageHandler_(type, data);
-};
-
 /**
  * @param {remoting.ClientSession.State} newState The new state for the session.
  * @return {void} Nothing.

remoting/webapp/crd/js/desktop_remoting.js

@@ -171,7 +171,7 @@ remoting.DesktopRemoting.prototype.onConnected_ = function(connectionInfo) {
   if (connectionInfo.session().hasCapability(
           remoting.ClientSession.Capability.VIDEO_RECORDER)) {
     var recorder = new remoting.VideoFrameRecorder();
-    this.sessionConnector_.registerProtocolExtension(recorder);
+    connectionInfo.plugin().extensions().register(recorder);
     this.connectedView_.setVideoFrameRecorder(recorder);
   }
 

remoting/webapp/crd/js/me2me_activity.js

@@ -120,11 +120,11 @@ remoting.Me2MeActivity.prototype.onConnected = function(connectionInfo) {
   // Reset the refresh flag so that the next connection will retry if needed.
   this.retryOnHostOffline_ = true;
 
+  var plugin = connectionInfo.plugin();
   if (remoting.app.hasCapability(remoting.ClientSession.Capability.CAST)) {
-    this.connector_.registerProtocolExtension(
-        new remoting.CastExtensionHandler());
+    plugin.extensions().register(new remoting.CastExtensionHandler());
   }
-  this.connector_.registerProtocolExtension(new remoting.GnubbyAuthHandler());
+  plugin.extensions().register(new remoting.GnubbyAuthHandler());
   this.pinDialog_.requestPairingIfNecessary(connectionInfo.plugin(),
                                             this.connector_);
 };

remoting/webapp/crd/js/session_connector.js

@@ -97,12 +97,6 @@ remoting.SessionConnector.prototype.cancel = function() {};
  */
 remoting.SessionConnector.prototype.getHostId = function() {};
 
-/**
- * @param {remoting.ProtocolExtension} extension
- */
-remoting.SessionConnector.prototype.registerProtocolExtension =
-    function(extension) {};
-
 /**
  * Closes the session and removes the plugin element.
  */

remoting/webapp/crd/js/session_connector_impl.js

@@ -89,17 +89,6 @@ remoting.SessionConnectorImpl.prototype.resetConnection_ = function() {
 
   /** @private {remoting.CredentialsProvider} */
   this.credentialsProvider_ = null;
-
-  /** @private {Object<string,remoting.ProtocolExtension>} */
-  this.protocolExtensions_ = {};
-
-  /**
-   * True once a session has been created and we've started the extensions.
-   * This is used to immediately start any extensions that are registered
-   * after the CONNECTED state change.
-   * @private {boolean}
-   */
-  this.protocolExtensionsStarted_ = false;
 };
 
 /**
@@ -327,8 +316,7 @@ remoting.SessionConnectorImpl.prototype.onPluginInitialized_ = function(
   }
 
   this.clientSession_ = new remoting.ClientSession(
-      this.plugin_, this.host_, this.signalStrategy_,
-      this.onProtocolExtensionMessage_.bind(this));
+      this.plugin_, this.host_, this.signalStrategy_);
   remoting.clientSession = this.clientSession_;
 
   this.clientSession_.logHostOfflineErrors(this.logHostOfflineErrors_);
@@ -359,89 +347,6 @@ remoting.SessionConnectorImpl.prototype.closeSession = function() {
   this.plugin_ = null;
 };
 
-/**
- * @param {remoting.ProtocolExtension} extension
- */
-remoting.SessionConnectorImpl.prototype.registerProtocolExtension =
-    function(extension) {
-  var types = extension.getExtensionTypes();
-
-  // Make sure we don't have an extension of that type already registered.
-  for (var i=0, len=types.length; i < len; i++) {
-    if (types[i] in this.protocolExtensions_) {
-      console.error(
-          'Attempt to register multiple extensions of the same type: ', type);
-      return;
-    }
-  }
-
-  for (var i=0, len=types.length; i < len; i++) {
-    var type = types[i];
-    this.protocolExtensions_[type] = extension;
-    if (this.protocolExtensionsStarted_) {
-      this.startProtocolExtension_(type);
-    }
-  }
-};
-
-/** @private */
-remoting.SessionConnectorImpl.prototype.initProtocolExtensions_ = function() {
-  base.debug.assert(!this.protocolExtensionsStarted_);
-  for (var type in this.protocolExtensions_) {
-    this.startProtocolExtension_(type);
-  }
-  this.protocolExtensionsStarted_ = true;
-};
-
-/**
- * @param {string} type
- * @private
- */
-remoting.SessionConnectorImpl.prototype.startProtocolExtension_ =
-    function(type) {
-  var extension = this.protocolExtensions_[type];
-  extension.startExtension(this.plugin_.sendClientMessage.bind(this.plugin_));
-};
-
-/**
- * Called when an extension message needs to be handled.
- *
- * @param {string} type The type of the extension message.
- * @param {string} data The payload of the extension message.
- * @return {boolean} Return true if the extension message was recognized.
- * @private
- */
-remoting.SessionConnectorImpl.prototype.onProtocolExtensionMessage_ =
-    function(type, data) {
-  if (type == 'test-echo-reply') {
-    console.log('Got echo reply: ' + data);
-    return true;
-  }
-
-  var message = base.jsonParseSafe(data);
-  if (typeof message != 'object') {
-    console.error('Error parsing extension json data: ' + data);
-    return false;
-  }
-
-  if (type in this.protocolExtensions_) {
-    /** @type {remoting.ProtocolExtension} */
-    var extension = this.protocolExtensions_[type];
-    var handled = false;
-    try {
-      handled = extension.onExtensionMessage(type, message);
-    } catch (/** @type {*} */ err) {
-      console.error('Failed to process protocol extension ' + type +
-                    ' message: ' + err);
-    }
-    if (handled) {
-      return true;
-    }
-  }
-
-  return false;
-};
-
 /**
  * Handle a change in the state of the client session prior to successful
  * connection (after connection, this class no longer handles state change
@@ -472,8 +377,6 @@ remoting.SessionConnectorImpl.prototype.onStateChange_ = function(event) {
           this.host_, this.credentialsProvider_, this.clientSession_,
           this.plugin_);
       this.onConnected_(connectionInfo);
-      // Initialize any protocol extensions that may have been added by the app.
-      this.initProtocolExtensions_();
       break;
 
     case remoting.ClientSession.State.CONNECTING: