Improve web-app type safety.

remoting/webapp/me2mom/client_session.js

Index: remoting/webapp/me2mom/client_session.js
diff --git a/remoting/webapp/me2mom/client_session.js b/remoting/webapp/me2mom/client_session.js
index 505e41f087d1616cf1904add0f1f0e8fd9a0d049..f451e5d829a77593760e16c1937718260e01cf60 100644
--- a/remoting/webapp/me2mom/client_session.js
+++ b/remoting/webapp/me2mom/client_session.js
@@ -16,7 +16,6 @@
 /** @suppress {duplicate} */
 var remoting = remoting || {};
 
-(function() {

[Jamie, 2011/10/18 00:19:27]

JSCompiler doesn't recognise types inside anonymous functions so I've removed
most of them. They weren't buying us much anyway.

[simonmorris, 2011/10/18 00:55:36]

OK.

 /**
  * @param {string} hostJid The jid of the host to connect to.
  * @param {string} hostPublicKey The base64 encoded version of the host's
@@ -38,6 +37,9 @@ remoting.ClientSession = function(hostJid, hostPublicKey, accessCode, email,
   this.accessCode = accessCode;
   this.email = email;
   this.clientJid = '';
+  this.sessionId = '';
+  /** @type {remoting.ViewerPlugin} */ this.plugin = null;
+
   this.onStateChange = onStateChange;
 };
 
@@ -117,7 +119,7 @@ remoting.ClientSession.prototype.PLUGIN_ID = 'session-client-plugin';
 /**
  * Callback to invoke when the state is changed.
  *
- * @type {function(remoting.ClientSession.State):void}
+ * @param {remoting.ClientSession.State} state The previous state.
  */
 remoting.ClientSession.prototype.onStateChange = function(state) { };
 
@@ -146,8 +148,10 @@ remoting.ClientSession.prototype.createPluginAndConnect =
     return;
   }
 
-  var that = this;
+  /** @type {remoting.ClientSession} */ var that = this;

[Jamie, 2011/10/18 00:19:27]

I don't know why the compiler can't infer the type of |that|, but it seems that
it can't. This annotation crops up a few times.

[simonmorris, 2011/10/18 00:55:36]

Probably because |this| is a keyword, not a variable.
If this function is called from as a callback, I think
|this| needn't be a remoting.ClientSession.

[simonmorris, 2011/10/18 00:55:36]

No newline between "...*/" and "var ..."?

[Jamie, 2011/10/18 01:42:01]

Good point about the callback behaviour. I'd forgotten what an unholy hack
|this| is in Javascript.

Regarding the new-line, I've tended to put type annotations on the same line, as
they won't be in a typed languages. Where I'm annotating function parameters,
I've tended to put them above the function as there are often more than one.
That said, I suspect I haven't been very consistent about it.

[simonmorris, 2011/10/18 15:26:02]

OK.

+  /** @param {string} msg The IQ stanza to send. */
   this.plugin.sendIq = function(msg) { that.sendIq_(msg); };
+  /** @param {string} msg The message to log. */
   this.plugin.debugInfo = function(msg) {
     remoting.debug.log('plugin: ' + msg);
   };
@@ -180,13 +184,12 @@ remoting.ClientSession.prototype.createPluginAndConnect =
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.removePlugin = function() {
-  var plugin = document.getElementById(this.PLUGIN_ID);
-  if (plugin) {
+  if (this.plugin) {

[Jamie, 2011/10/18 00:19:27]

We've stored the plugin already--not sure why we were looking it up again.

[simonmorris, 2011/10/18 00:55:36]

Are you certain |this| is always what you think it is here?
I think it depends how removePlugin() is invoked. That might
be the reason |plugin| used to be looked up - or it might
just be inadvertent, as you say.

[Jamie, 2011/10/18 01:42:01]

Previously the function referred to both this.plugin and the version it had
looked up, so I'm pretty sure it's a case of different parts of the function
being written at different times.

[simonmorris, 2011/10/18 15:26:02]

Right - there's a |this.plugin| in the previous version, so
the function must be invoked in the way that makes |this| do
the right thing. grep confirms.

     var parentNode = this.plugin.parentNode;
-    parentNode.removeChild(plugin);
-    plugin = null;
+    parentNode.removeChild(this.plugin);
+    this.plugin = null;
   }
-}
+};
 
 /**
  * Deletes the <embed> element from the container and disconnects.
@@ -266,11 +269,13 @@ remoting.ClientSession.prototype.connectPluginToWcs_ =
   if (this.clientJid == '') {
     remoting.debug.log('Tried to connect without a full JID.');
   }
-  var that = this;
-  remoting.wcs.setOnIq(function(stanza) {
-      remoting.debug.log('Receiving Iq: ' + stanza);
-      that.plugin.onIq(stanza);
-  });
+  /** @type {remoting.ClientSession} */ var that = this;
+  /** @param {string} stanza The IQ stanza received. */
+  var onIq = function(stanza) {

[Jamie, 2011/10/18 00:19:27]

It's fiddly to add type information to anonymous functions, so I've done this
simple transformation a few times.

[simonmorris, 2011/10/18 00:55:36]

OK.

+    remoting.debug.log('Receiving Iq: ' + stanza);
+    that.plugin.onIq(stanza);
+  }
+  remoting.wcs.setOnIq(onIq);
   that.plugin.connect(this.hostJid, this.hostPublicKey, this.clientJid,
                       this.accessCode);
 };
@@ -302,7 +307,7 @@ remoting.ClientSession.prototype.connectionInfoUpdateCallback = function() {
       error = remoting.ClientSession.ConnectionError.SESSION_REJECTED;
     } else if (error == this.plugin.ERROR_INCOMPATIBLE_PROTOCOL) {
       error = remoting.ClientSession.ConnectionError.INCOMPATIBLE_PROTOCOL;
-    } else if (error == this.plugin.NETWORK_FAILURE) {
+    } else if (error == this.plugin.ERROR_NETWORK_FAILURE) {

[Jamie, 2011/10/18 00:19:27]

A genuine error! My efforts were not wasted after all.

       error = remoting.ClientSession.ConnectionError.NETWORK_FAILURE;
     } else {
       error = remoting.ClientSession.ConnectionError.OTHER;
@@ -327,7 +332,6 @@ remoting.ClientSession.prototype.setState_ = function(state) {
 /**
  * This is a callback that gets called when the window is resized.
  *
- * @private

[Jamie, 2011/10/18 00:19:27]

This is called externally.

  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.onWindowSizeChanged = function() {
@@ -382,11 +386,11 @@ remoting.ClientSession.prototype.updateDimensions = function() {
   if (this.plugin.width < windowWidth)
     parentNode.style.left = (windowWidth - this.plugin.width) / 2 + 'px';
   else
-    parentNode.style.left = 0;
+    parentNode.style.left = '0';

[Jamie, 2011/10/18 00:19:27]

Setting this to 0 works fine, but the property should actually a string.

   if (this.plugin.height < windowHeight)
     parentNode.style.top = (windowHeight - this.plugin.height) / 2 + 'px';
   else
-    parentNode.style.top = 0;
+    parentNode.style.top = '0';
 
   remoting.debug.log('plugin dimensions: ' +
                      parentNode.style.left + ',' +
@@ -398,7 +402,7 @@ remoting.ClientSession.prototype.updateDimensions = function() {
 /**
  * Returns an associative array with a set of stats for this connection.
  *
- * @return {Object} The connection statistics.
+ * @return {Object.<string, number>} The connection statistics.
  */
 remoting.ClientSession.prototype.stats = function() {
   return {
@@ -410,5 +414,3 @@ remoting.ClientSession.prototype.stats = function() {
     'roundtrip_latency': this.plugin.roundTripLatency
   };
 };
-
-}());