Clean up client state callback.

BUG=None
TEST=Manual

Review URL: http://codereview.chromium.org/8573024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111036 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 77 insertions, 72 deletions

diff --git a/remoting/client/plugin/chromoting_scriptable_object.cc b/remoting/client/plugin/chromoting_scriptable_object.cc
index 91a82b1..a9a60d5 100644
--- a/remoting/client/plugin/chromoting_scriptable_object.cc
+++ b/remoting/client/plugin/chromoting_scriptable_object.cc
@@ -246,7 +246,7 @@ void ChromotingScriptableObject::SetConnectionStatus(
   }
 
   if (signal)
-    SignalConnectionInfoChange();
+    SignalConnectionInfoChange(status, error);
 }
 
 void ChromotingScriptableObject::LogDebugInfo(const std::string& info) {
@@ -299,10 +299,12 @@ void ChromotingScriptableObject::AddMethod(const std::string& name,
   properties_.push_back(PropertyDescriptor(name, handler));
 }
 
-void ChromotingScriptableObject::SignalConnectionInfoChange() {
+void ChromotingScriptableObject::SignalConnectionInfoChange(int status,
+                                                            int error) {
   plugin_message_loop_->PostTask(
       FROM_HERE, task_factory_.NewRunnableMethod(
-          &ChromotingScriptableObject::DoSignalConnectionInfoChange));
+          &ChromotingScriptableObject::DoSignalConnectionInfoChange,
+          status, error));
 }
 
 void ChromotingScriptableObject::SignalDesktopSizeChange() {
@@ -311,12 +313,13 @@ void ChromotingScriptableObject::SignalDesktopSizeChange() {
           &ChromotingScriptableObject::DoSignalDesktopSizeChange));
 }
 
-void ChromotingScriptableObject::DoSignalConnectionInfoChange() {
+void ChromotingScriptableObject::DoSignalConnectionInfoChange(int status,
+                                                              int error) {
   Var exception;
   VarPrivate cb = GetProperty(Var(kConnectionInfoUpdate), &exception);
 
   // |this| must not be touched after Call() returns.
-  cb.Call(Var(), &exception);
+  cb.Call(Var(), Var(status), Var(error), &exception);
 
   if (!exception.is_undefined())
     LOG(ERROR) << "Exception when invoking connectionInfoUpdate JS callback.";
diff --git a/remoting/client/plugin/chromoting_scriptable_object.h b/remoting/client/plugin/chromoting_scriptable_object.h
index 1ca45fd..547b349 100644
--- a/remoting/client/plugin/chromoting_scriptable_object.h
+++ b/remoting/client/plugin/chromoting_scriptable_object.h
@@ -120,7 +120,11 @@ class ChromotingScriptableObject
     : public pp::deprecated::ScriptableObject,
       public base::SupportsWeakPtr<ChromotingScriptableObject> {
  public:
+  // These state values are duplicated in the JS code. Remember to update both
+  // copies when making changes.
   enum ConnectionStatus {
+    // TODO(jamiewalch): Remove STATUS_UNKNOWN once all web-apps that might try
+    // to access it have been upgraded.
     STATUS_UNKNOWN = 0,
     STATUS_CONNECTING,
     STATUS_INITIALIZING,
@@ -129,6 +133,8 @@ class ChromotingScriptableObject
     STATUS_FAILED,
   };
 
+  // These state values are duplicated in the JS code. Remember to update both
+  // copies when making changes.
   enum ConnectionError {
     ERROR_NONE = 0,
     ERROR_HOST_IS_OFFLINE,
@@ -197,13 +203,13 @@ class ChromotingScriptableObject
   void AddAttribute(const std::string& name, pp::Var attribute);
   void AddMethod(const std::string& name, MethodHandler handler);
 
-  void SignalConnectionInfoChange();
+  void SignalConnectionInfoChange(int status, int error);
   void SignalDesktopSizeChange();
 
   // Calls to these methods are posted to the plugin thread so that we
   // call JavaScript with clean stack. This is necessary because
   // JavaScript event handlers may destroy the plugin.
-  void DoSignalConnectionInfoChange();
+  void DoSignalConnectionInfoChange(int status, int error);
   void DoSignalDesktopSizeChange();
   void DoSendIq(const std::string& message_xml);
 
diff --git a/remoting/webapp/me2mom/client_screen.js b/remoting/webapp/me2mom/client_screen.js
index ad69024..85cc20b 100644
--- a/remoting/webapp/me2mom/client_screen.js
+++ b/remoting/webapp/me2mom/client_screen.js
@@ -179,29 +179,29 @@ function tryConnectWithWcs_(success) {
  * current state is available via the |state| member variable.
  *
  * @param {number} oldState The previous state of the plugin.
+ * @param {number} newState The current state of the plugin.
  */
 // TODO(jamiewalch): Make this pass both the current and old states to avoid
 // race conditions.
-function onClientStateChange_(oldState) {
+function onClientStateChange_(oldState, newState) {
   if (!remoting.clientSession) {
     // If the connection has been cancelled, then we no longer have a reference
     // to the session object and should ignore any state changes.
     return;
   }
-  var state = remoting.clientSession.state;
-  if (state == remoting.ClientSession.State.CREATED) {
+  if (newState == remoting.ClientSession.State.CREATED) {
     remoting.debug.log('Created plugin');
 
-  } else if (state == remoting.ClientSession.State.BAD_PLUGIN_VERSION) {
+  } else if (newState == remoting.ClientSession.State.BAD_PLUGIN_VERSION) {
     showConnectError_(remoting.Error.BAD_PLUGIN_VERSION);
 
-  } else if (state == remoting.ClientSession.State.CONNECTING) {
+  } else if (newState == remoting.ClientSession.State.CONNECTING) {
     remoting.debug.log('Connecting as ' + remoting.oauth2.getCachedEmail());
 
-  } else if (state == remoting.ClientSession.State.INITIALIZING) {
+  } else if (newState == remoting.ClientSession.State.INITIALIZING) {
     remoting.debug.log('Initializing connection');
 
-  } else if (state == remoting.ClientSession.State.CONNECTED) {
+  } else if (newState == remoting.ClientSession.State.CONNECTED) {
     if (remoting.clientSession) {
       remoting.setMode(remoting.AppMode.IN_SESSION);
       recenterToolbar_();
@@ -209,7 +209,7 @@ function onClientStateChange_(oldState) {
       updateStatistics_();
     }
 
-  } else if (state == remoting.ClientSession.State.CLOSED) {
+  } else if (newState == remoting.ClientSession.State.CLOSED) {
     if (oldState == remoting.ClientSession.State.CONNECTED) {
       remoting.clientSession.removePlugin();
       remoting.clientSession = null;
@@ -222,7 +222,7 @@ function onClientStateChange_(oldState) {
       showConnectError_(remoting.Error.INVALID_ACCESS_CODE);
     }
 
-  } else if (state == remoting.ClientSession.State.CONNECTION_FAILED) {
+  } else if (newState == remoting.ClientSession.State.CONNECTION_FAILED) {
     remoting.debug.log('Client plugin reported connection failed: ' +
                        remoting.clientSession.error);
     if (remoting.clientSession.error ==
@@ -242,7 +242,7 @@ function onClientStateChange_(oldState) {
     }
 
   } else {
-    remoting.debug.log('Unexpected client plugin state: ' + state);
+    remoting.debug.log('Unexpected client plugin state: ' + newState);
     // This should only happen if the web-app and client plugin get out of
     // sync, and even then the version check should allow compatibility.
     showConnectError_(remoting.Error.MISSING_PLUGIN);
diff --git a/remoting/webapp/me2mom/client_session.js b/remoting/webapp/me2mom/client_session.js
index 2b566e3..e862afa 100644
--- a/remoting/webapp/me2mom/client_session.js
+++ b/remoting/webapp/me2mom/client_session.js
@@ -22,10 +22,9 @@ var remoting = remoting || {};
  *     public key.
  * @param {string} accessCode The access code for the IT2Me connection.
  * @param {string} email The username for the talk network.
- * @param {function(remoting.ClientSession.State):void} onStateChange
- *     The callback to invoke when the session changes state. This callback
- *     occurs after the state changes and is passed the previous state; the
- *     new state is accessible via ClientSession's |state| property.
+ * @param {function(remoting.ClientSession.State,
+                    remoting.ClientSession.State):void} onStateChange
+ *     The callback to invoke when the session changes state.
  * @constructor
  */
 remoting.ClientSession = function(hostJid, hostPublicKey, accessCode, email,
@@ -43,17 +42,21 @@ remoting.ClientSession = function(hostJid, hostPublicKey, accessCode, email,
   this.onStateChange = onStateChange;
 };
 
+// Note that the positive values in both of these enums are copied directly
+// from chromoting_scriptable_object.h and must be kept in sync. The negative
+// values represent states transitions that occur within the web-app that have
+// no corresponding plugin state transition.
 /** @enum {number} */
 remoting.ClientSession.State = {
+  CREATED: -3,
+  BAD_PLUGIN_VERSION: -2,
+  UNKNOWN_PLUGIN_ERROR: -1,
   UNKNOWN: 0,
-  CREATED: 1,
-  BAD_PLUGIN_VERSION: 2,
-  UNKNOWN_PLUGIN_ERROR: 3,
-  CONNECTING: 4,
-  INITIALIZING: 5,
-  CONNECTED: 6,
-  CLOSED: 7,
-  CONNECTION_FAILED: 8
+  CONNECTING: 1,
+  INITIALIZING: 2,
+  CONNECTED: 3,
+  CLOSED: 4,
+  CONNECTION_FAILED: 5
 };
 
 /** @enum {number} */
@@ -62,8 +65,7 @@ remoting.ClientSession.ConnectionError = {
   HOST_IS_OFFLINE: 1,
   SESSION_REJECTED: 2,
   INCOMPATIBLE_PROTOCOL: 3,
-  NETWORK_FAILURE: 4,
-  OTHER: 5
+  NETWORK_FAILURE: 4
 };
 
 /**
@@ -119,9 +121,11 @@ remoting.ClientSession.prototype.PLUGIN_ID = 'session-client-plugin';
 /**
  * Callback to invoke when the state is changed.
  *
- * @param {remoting.ClientSession.State} state The previous state.
+ * @param {remoting.ClientSession.State} oldState The previous state.
+ * @param {remoting.ClientSession.State} newState The current state.
  */
-remoting.ClientSession.prototype.onStateChange = function(state) { };
+remoting.ClientSession.prototype.onStateChange =
+    function(oldState, newState) { };
 
 /**
  * Adds <embed> element to |container| and readies the sesion object.
@@ -159,8 +163,12 @@ remoting.ClientSession.prototype.createPluginAndConnect =
   // TODO(ajwong): Is it even worth having this class handle these events?
   // Or would it be better to just allow users to pass in their own handlers
   // and leave these blank by default?
-  this.plugin.connectionInfoUpdate = function() {
-    that.connectionInfoUpdateCallback();
+  /**
+   * @param {number} status The plugin status.
+   * @param {number} error The plugin error status, if any.
+   */
+  this.plugin.connectionInfoUpdate = function(status, error) {
+    that.connectionInfoUpdateCallback(status, error);
   };
   this.plugin.desktopSizeUpdate = function() { that.onDesktopSizeChanged_(); };
 
@@ -290,49 +298,39 @@ remoting.ClientSession.prototype.connectPluginToWcs_ =
 /**
  * Callback that the plugin invokes to indicate that the connection
  * status has changed.
+ *
+ * @param {number} status The plugin's status.
+ * @param {number} error The plugin's error state, if any.
  */
-remoting.ClientSession.prototype.connectionInfoUpdateCallback = function() {
-  var state = this.plugin.status;
-
-  // TODO(ajwong): We're doing silly type translation here. Any way to avoid?
-  if (state == this.plugin.STATUS_UNKNOWN) {
-    this.setState_(remoting.ClientSession.State.UNKNOWN);
-  } else if (state == this.plugin.STATUS_CONNECTING) {
-    this.setState_(remoting.ClientSession.State.CONNECTING);
-  } else if (state == this.plugin.STATUS_INITIALIZING) {
-    this.setState_(remoting.ClientSession.State.INITIALIZING);
-  } else if (state == this.plugin.STATUS_CONNECTED) {
+remoting.ClientSession.prototype.connectionInfoUpdateCallback =
+    function(status, error) {
+  // Old plugins didn't pass the status and error values, so get them directly.
+  // Note that there is a race condition inherent in this approach.
+  if (typeof(status) == 'undefined') {
+    status = this.plugin.status;
+  }
+  if (typeof(error) == 'undefined') {
+    error = this.plugin.error;
+  }
+
+  if (status == this.plugin.STATUS_CONNECTED) {
     this.onDesktopSizeChanged_();
-    this.setState_(remoting.ClientSession.State.CONNECTED);
-  } else if (state == this.plugin.STATUS_CLOSED) {
-    this.setState_(remoting.ClientSession.State.CLOSED);
-  } else if (state == this.plugin.STATUS_FAILED) {
-    var error = this.plugin.error;
-    if (error == this.plugin.ERROR_HOST_IS_OFFLINE) {
-      this.error = remoting.ClientSession.ConnectionError.HOST_IS_OFFLINE;
-    } else if (error == this.plugin.ERROR_SESSION_REJECTED) {
-      this.error = remoting.ClientSession.ConnectionError.SESSION_REJECTED;
-    } else if (error == this.plugin.ERROR_INCOMPATIBLE_PROTOCOL) {
-      this.error = remoting.ClientSession.ConnectionError.INCOMPATIBLE_PROTOCOL;
-    } else if (error == this.plugin.ERROR_NETWORK_FAILURE) {
-      this.error = remoting.ClientSession.ConnectionError.NETWORK_FAILURE;
-    } else {
-      this.error = remoting.ClientSession.ConnectionError.OTHER;
-    }
-    this.setState_(remoting.ClientSession.State.CONNECTION_FAILED);
+  } else if (status == this.plugin.STATUS_FAILED) {
+    this.error = /** @type {remoting.ClientSession.ConnectionError} */ (error);
   }
+  this.setState_(/** @type {remoting.ClientSession.State} */ (status));
 };
 
 /**
  * @private
- * @param {remoting.ClientSession.State} state The new state for the session.
+ * @param {remoting.ClientSession.State} newState The new state for the session.
  * @return {void} Nothing.
  */
-remoting.ClientSession.prototype.setState_ = function(state) {
+remoting.ClientSession.prototype.setState_ = function(newState) {
   var oldState = this.state;
-  this.state = state;
+  this.state = newState;
   if (this.onStateChange) {
-    this.onStateChange(oldState);
+    this.onStateChange(oldState, newState);
   }
   this.logToServer.logClientSessionStateChange(this.state, this.error);
 };
@@ -343,9 +341,6 @@ remoting.ClientSession.prototype.setState_ = function(state) {
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.onWindowSizeChanged = function() {
-  remoting.debug.log('window size changed: ' +
-                     window.innerWidth + 'x' +
-                     window.innerHeight);
   this.updateDimensions();
 };
 
diff --git a/remoting/webapp/me2mom/server_log_entry.js b/remoting/webapp/me2mom/server_log_entry.js
index 18c5784..0b1f23f 100644
--- a/remoting/webapp/me2mom/server_log_entry.js
+++ b/remoting/webapp/me2mom/server_log_entry.js
@@ -85,8 +85,6 @@ remoting.ServerLogEntry.prototype.getValueForConnectionError =
       return 'incompatible-protocol';
     case remoting.ClientSession.ConnectionError.NETWORK_FAILURE:
       return 'network-failure';
-    case remoting.ClientSession.ConnectionError.OTHER:
-      return 'other';
     default:
       return 'unknown-' + connectionError;
   }
diff --git a/remoting/webapp/me2mom/viewer_plugin_proto.js b/remoting/webapp/me2mom/viewer_plugin_proto.js
index 3954994..d480d17 100644
--- a/remoting/webapp/me2mom/viewer_plugin_proto.js
+++ b/remoting/webapp/me2mom/viewer_plugin_proto.js
@@ -32,6 +32,9 @@ remoting.ViewerPlugin.prototype.releaseAllKeys = function() {};
 remoting.ViewerPlugin.prototype.connect =
     function(hostJid, hostPublicKey, clientJid, accessCode) {};
 
+/** @type {function(number, number): void} State change callback function. */
+remoting.ViewerPlugin.prototype.connectionInfoUpdate;
+
 /** @type {number} */ remoting.ViewerPlugin.prototype.apiMinVersion;
 /** @type {number} */ remoting.ViewerPlugin.prototype.apiVersion;