[Chromoting] Change Application.Delegate to proper subclass of Application.

remoting/webapp/base/js/application.js

Index: remoting/webapp/base/js/application.js
diff --git a/remoting/webapp/base/js/application.js b/remoting/webapp/base/js/application.js
index bcd6cf122f2d5a5471fd16926e2e740863c23869..cb9f3ec02343b7db0441b5f2363407aa7aeebcf5 100644
--- a/remoting/webapp/base/js/application.js
+++ b/remoting/webapp/base/js/application.js
@@ -13,12 +13,58 @@
 var remoting = remoting || {};
 
 /**
+ * The interface specifies the methods that a subclass of remoting.Application
+ * is required implement to override the default behavior.
+ *
+ * @interface
+ */
+remoting.ApplicationInterface = function() {};
+
+/**
+ * @return {string} Application product name to be used in UI.
+ */
+remoting.ApplicationInterface.prototype.getApplicationName = function() {};
+
+/**
+ * Report an authentication error to the user. This is called in lieu of
+ * startApplication() if the user cannot be authenticated or if they decline
+ * the app permissions.
+ *
+ * @param {!remoting.Error} error The failure reason.
+ */
+remoting.ApplicationInterface.prototype.signInFailed = function(error) {};
+
+/**
+ * Initialize the application. This is called before an OAuth token is requested
+ * and should be used for tasks such as initializing the DOM, registering event
+ * handlers, etc. After this is called, the app is running and waiting for
+ * user events.
+ *
+ * @return {void} Nothing.
+ */
+remoting.ApplicationInterface.prototype.initApplication = function() {};
+
+/**
+ * Start the application. Once startApplication() is called, we can assume that
+ * the user has consented to all permissions specified in the manifest.
+ *
+ * @param {string} token An OAuth access token. The app should not cache
+ *     this token, but can assume that it will remain valid during application
+ *     start-up.
+ */
+remoting.ApplicationInterface.prototype.startApplication = function(token) {};
+
+
+/**
  * @param {Array<string>} appCapabilities Array of application capabilities.
  * @constructor
+ * @implements {remoting.ApplicationInterface}
  */
 remoting.Application = function(appCapabilities) {
-  /** @private {remoting.Application.Delegate} */
-  this.delegate_ = null;
+  // Create global objects.
+  remoting.ClientPlugin.factory = new remoting.DefaultClientPluginFactory();
+  remoting.SessionConnector.factory =
+      new remoting.DefaultSessionConnectorFactory();

[garykac, 2015/03/23 18:44:22]

Moved here from start() since this is required to create the sessionconnector.

[Jamie, 2015/03/25 20:00:39]

The purpose of these factories is to allow test code to substitute mocks for the
client plugin and session connector. Since the Application is constructed from a
load handler, moving them here makes that more difficult.

[garykac, 2015/03/26 01:41:57]

Previously, they were setup in the start() function, which was also called in
the load handler (just after the app was created). So moving them here does not
change the testability.

Fixing the problem you point out (tests being able to override) should be in a
followup cl.

 
   /** @private {Array<string>} */
   this.appCapabilities_ = [
@@ -29,26 +75,23 @@ remoting.Application = function(appCapabilities) {
   // Append the app-specific capabilities.
   this.appCapabilities_.push.apply(this.appCapabilities_, appCapabilities);
 
-  /** @private {remoting.SessionConnector} */
-  this.sessionConnector_ = null;
+  /** @protected {remoting.SessionConnector} */
+  this.sessionConnector_ = remoting.SessionConnector.factory.createConnector(

[garykac, 2015/03/23 18:44:22]

Moved here from getter.

+      document.getElementById('client-container'),
+      this.onConnected.bind(this),
+      this.onError.bind(this),
+      this.onConnectionFailed.bind(this),
+      this.appCapabilities_);
 
   /** @private {base.Disposable} */
   this.sessionConnectedHooks_ = null;
 };
 
 /**
- * @param {remoting.Application.Delegate} appDelegate The delegate that
- *    contains the app-specific functionality.
+ * @return {remoting.SessionConnector} The session connector.
  */
-remoting.Application.prototype.setDelegate = function(appDelegate) {
-  this.delegate_ = appDelegate;
-};
-
-/**
- * @return {string} Application product name to be used in UI.
- */
-remoting.Application.prototype.getApplicationName = function() {
-  return this.delegate_.getApplicationName();
+remoting.Application.prototype.getSessionConnector = function() {
+  return this.sessionConnector_;
 };
 
 /**
@@ -60,6 +103,27 @@ remoting.Application.prototype.hasCapability = function(capability) {
   return capabilities.indexOf(capability) != -1;
 };
 
+
+/** @return {string} */

[Jamie, 2015/03/24 21:57:05]

AFAICT, these functions are essentially boilerplate to provide something
equivalent to pure virtual functions in Javascript. As such, I would move them
to the end of the file so they don't get in the way of understanding the rest of
it.

[garykac, 2015/03/26 01:41:57]

Done.

+remoting.Application.prototype.getApplicationName = function() {
+  throw "Subclass must override";

[Jamie, 2015/03/24 21:57:05]

base.assert would be more appropriate here.

[garykac, 2015/03/26 01:41:57]

Done.

+};
+
+/** @param {!remoting.Error} error */
+remoting.Application.prototype.signInFailed = function(error) {
+  throw "Subclass must override";
+};
+
+remoting.Application.prototype.initApplication = function() {
+  throw "Subclass must override";
+};
+
+/** @param {string} token */
+remoting.Application.prototype.startApplication = function(token) {
+  throw "Subclass must override";
+};
+
+
 /**
  * Initialize the application and register all event handlers. After this
  * is called, the app is running and waiting for user events.
@@ -67,11 +131,6 @@ remoting.Application.prototype.hasCapability = function(capability) {
  * @return {void} Nothing.
  */
 remoting.Application.prototype.start = function() {
-  // Create global objects.
-  remoting.ClientPlugin.factory = new remoting.DefaultClientPluginFactory();
-  remoting.SessionConnector.factory =
-      new remoting.DefaultSessionConnectorFactory();
-
   // TODO(garykac): This should be owned properly rather than living in the
   // global 'remoting' namespace.
   remoting.settings = new remoting.Settings();
@@ -79,17 +138,17 @@ remoting.Application.prototype.start = function() {
   remoting.initGlobalObjects();
   remoting.initIdentity();
 
-  this.delegate_.init();
+  this.initApplication();
 
   var that = this;
-  remoting.identity.getToken().then(
-      this.delegate_.start.bind(this.delegate_, this.getSessionConnector())
-  ).catch(remoting.Error.handler(
+  remoting.identity.getToken().
+    then(this.startApplication.bind(this)).
+    catch(remoting.Error.handler(
       function(/** !remoting.Error */ error) {
         if (error.hasTag(remoting.Error.Tag.CANCELLED)) {
           that.exit();
         } else {
-          that.delegate_.signInFailed(error);
+          that.signInFailed(error);
         }
       }
     )
@@ -100,7 +159,6 @@ remoting.Application.prototype.start = function() {
  * Quit the application.
  */
 remoting.Application.prototype.exit = function() {
-  this.delegate_.handleExit();

[Jamie, 2015/03/24 21:57:05]

I'm concerned about a loss of readability here. With the old code, when I see a
call to remoting.application.exit(), it's immediately obvious where I need to
look for that method (ie, here) and when I get here, it's immediately obvious
that the delegate might be doing additional work. With the new design, it's not
obvious that the delegate-as-was is involved at all, which makes it much harder
to reason about the run-time behaviour of the application. This is one reason
why I think a delegate model, for all its failings, is a better design in this
case.

[Jamie, 2015/03/25 20:00:39]

As discussed off-line, please refactor so that all methods are either pure
virtual or final (at least notionally--Javascript doesn't have a concept of
either).

[garykac, 2015/03/26 01:41:57]

Done.

   chrome.app.window.current().close();
 };
 
@@ -112,22 +170,6 @@ remoting.Application.prototype.disconnect = function() {
   }
 };
 
-/**
- * Called when a new session has been connected.
- *
- * @param {remoting.ConnectionInfo} connectionInfo
- * @return {void} Nothing.
- */
-remoting.Application.prototype.onConnected = function(connectionInfo) {

[garykac, 2015/03/23 18:44:22]

This has been moved a bit further down so that it is next to onSessionFinished_
and updateStatistics_.

-  this.sessionConnectedHooks_ = new base.Disposables(
-    new base.EventHook(connectionInfo.session(), 'stateChanged',
-                       this.onSessionFinished_.bind(this)),
-    new base.RepeatingTimer(this.updateStatistics_.bind(this), 1000)
-  );
-  remoting.clipboard.startSession();
-
-  this.delegate_.handleConnected(connectionInfo);
-};
 
 /**
  * Called when the current session has been disconnected.
@@ -135,7 +177,6 @@ remoting.Application.prototype.onConnected = function(connectionInfo) {
  * @return {void} Nothing.
  */
 remoting.Application.prototype.onDisconnected = function() {

[Jamie, 2015/03/25 20:00:39]

Under the new "pure-virtual or final" model, I think most of these onX methods
should be part of the interface, correct? Also, there's a discrepancy between
public and private visibility among them. In general, I think we tend to make
onX methods private because they are registered by the class itself as
callbacks. I don't know if that approach makes sense here, because I'm not sure
where they are called from (and in any case, changing this is probably not part
of this CL).

[garykac, 2015/03/26 01:41:57]

All of them except getApplicationName are now @protected.

-  this.delegate_.handleDisconnected();

[garykac, 2015/03/23 18:44:22]

These redirects are no longer needed. Subclass will override if needed.

 };
 
 /**
@@ -145,7 +186,6 @@ remoting.Application.prototype.onDisconnected = function() {
  * @return {void} Nothing.
  */
 remoting.Application.prototype.onConnectionFailed = function(error) {
-  this.delegate_.handleConnectionFailed(this.sessionConnector_, error);
 };
 
 /**
@@ -155,24 +195,21 @@ remoting.Application.prototype.onConnectionFailed = function(error) {
  * @return {void} Nothing.
  */
 remoting.Application.prototype.onError = function(errorTag) {
-  this.delegate_.handleError(errorTag);
 };
 
 /**
- * @return {remoting.SessionConnector} A session connector, creating a new one
- *     if necessary.
+ * Called when a new session has been connected.
+ *
+ * @param {remoting.ConnectionInfo} connectionInfo
+ * @return {void} Nothing.
  */
-remoting.Application.prototype.getSessionConnector = function() {
-  // TODO(garykac): Check if this can be initialized in the ctor.

[garykac, 2015/03/23 18:44:22]

Recent changes make this easy to move into the constructor now.

-  if (!this.sessionConnector_) {
-    this.sessionConnector_ = remoting.SessionConnector.factory.createConnector(
-        document.getElementById('client-container'),
-        this.onConnected.bind(this),
-        this.onError.bind(this),
-        this.onConnectionFailed.bind(this),
-        this.appCapabilities_);
-  }
-  return this.sessionConnector_;
+remoting.Application.prototype.onConnected = function(connectionInfo) {
+  this.sessionConnectedHooks_ = new base.Disposables(
+    new base.EventHook(connectionInfo.session(), 'stateChanged',
+                       this.onSessionFinished_.bind(this)),
+    new base.RepeatingTimer(this.updateStatistics_.bind(this), 1000)
+  );
+  remoting.clipboard.startSession();
 };
 
 /**
@@ -219,84 +256,5 @@ remoting.Application.prototype.updateStatistics_ = function() {
 };
 
 
-/**
- * @interface
- */
-remoting.Application.Delegate = function() {};
-
-/**
- * Initialize the application. This is called before an OAuth token is requested
- * and should be used for tasks such as initializing the DOM, registering event
- * handlers, etc.
- */
-remoting.Application.Delegate.prototype.init = function() {};
-
-/**
- * Start the application. Once start() is called, the delegate can assume that
- * the user has consented to all permissions specified in the manifest.
- *
- * @param {remoting.SessionConnector} connector
- * @param {string} token An OAuth access token. The delegate should not cache
- *     this token, but can assume that it will remain valid during application
- *     start-up.
- */
-remoting.Application.Delegate.prototype.start = function(connector, token) {};
-
-/**
- * Report an authentication error to the user. This is called in lieu of start()
- * if the user cannot be authenticated.
- *
- * @param {!remoting.Error} error The failure reason.
- */
-remoting.Application.Delegate.prototype.signInFailed = function(error) {};
-
-/**
- * @return {string} Application product name to be used in UI.
- */
-remoting.Application.Delegate.prototype.getApplicationName = function() {};
-
-/**
- * Called when a new session has been connected.
- *
- * @param {remoting.ConnectionInfo} connectionInfo
- * @return {void} Nothing.
- */
-remoting.Application.Delegate.prototype.handleConnected = function(
-    connectionInfo) {};
-
-/**
- * Called when the current session has been disconnected.
- *
- * @return {void} Nothing.
- */
-remoting.Application.Delegate.prototype.handleDisconnected = function() {};
-
-/**
- * Called when the current session's connection has failed.
- *
- * @param {remoting.SessionConnector} connector
- * @param {!remoting.Error} error
- * @return {void} Nothing.
- */
-remoting.Application.Delegate.prototype.handleConnectionFailed =
-    function(connector, error) {};
-
-/**
- * Called when an error needs to be displayed to the user.
- *
- * @param {!remoting.Error} errorTag The error to be localized and displayed.
- * @return {void} Nothing.
- */
-remoting.Application.Delegate.prototype.handleError = function(errorTag) {};
-
-/**
- * Perform any application-specific cleanup before exiting. This is called in
- * lieu of start() if the user declines the app permissions, and will usually
- * be called immediately prior to exiting, although delegates should not rely
- * on this.
- */
-remoting.Application.Delegate.prototype.handleExit = function() {};
-
-
 /** @type {remoting.Application} */
 remoting.app = null;