Improve web-app type safety.

remoting/webapp/me2mom/client_session.js

 // Copyright (c) 2011 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
  * Session class that handles creation and teardown of a remoting session.
  *
  * This abstracts a <embed> element and controls the plugin which does the
  * actual remoting work.  There should be no UI code inside this class.  It
  * should be purely thought of as a controller of sorts.
  */
 
 'use strict';
 
 /** @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
  *     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.
  * @constructor
  */
 remoting.ClientSession = function(hostJid, hostPublicKey, accessCode, email,
                                   onStateChange) {
   this.state = remoting.ClientSession.State.CREATED;
 
   this.hostJid = hostJid;
   this.hostPublicKey = hostPublicKey;
   this.accessCode = accessCode;
   this.email = email;
   this.clientJid = '';
+  this.sessionId = '';
+  /** @type {remoting.ViewerPlugin} */ this.plugin = null;
+
   this.onStateChange = onStateChange;
 };
 
 /** @enum {number} */
 remoting.ClientSession.State = {
   UNKNOWN: 0,
   CREATED: 1,
   BAD_PLUGIN_VERSION: 2,
   UNKNOWN_PLUGIN_ERROR: 3,
   CONNECTING: 4,
@@ skipping 59 matching lines @@
 /**
  * The id of the client plugin
  *
  * @const
  */
 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) { };
 
 /**
  * Adds <embed> element to |container| and readies the sesion object.
  *
  * @param {Element} container The element to add the plugin to.
  * @param {string} oauth2AccessToken A valid OAuth2 access token.
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.createPluginAndConnect =
     function(container, oauth2AccessToken) {
   this.plugin = /** @type {remoting.ViewerPlugin} */
       document.createElement('embed');
   this.plugin.id = this.PLUGIN_ID;
   this.plugin.src = 'about://none';
   this.plugin.type = 'pepper-application/x-chromoting';
   this.plugin.width = 0;
   this.plugin.height = 0;
   container.appendChild(this.plugin);
 
   if (!this.isPluginVersionSupported_(this.plugin)) {
     // TODO(ajwong): Remove from parent.
     delete this.plugin;
     this.setState_(remoting.ClientSession.State.BAD_PLUGIN_VERSION);
     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);
   };
 
   // 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();
   };
@@ skipping 12 matching lines @@
 };
 
 /**
  * Deletes the <embed> element from the container, without sending a
  * session_terminate request.  This is to be called when the session was
  * disconnected by the Host.
  *
  * @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.
  *
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.disconnect = function() {
   if (remoting.wcs) {
     remoting.wcs.setOnIq(function(stanza) {});
   }
@@ skipping 59 matching lines @@
  * @private
  * @param {string} oauth2AccessToken A valid OAuth2 access token.
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.connectPluginToWcs_ =
     function(oauth2AccessToken) {
   this.clientJid = remoting.wcs.getJid();
   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);
 };
 
 /**
  * Callback that the plugin invokes to indicate that the connection
  * status has changed.
  */
 remoting.ClientSession.prototype.connectionInfoUpdateCallback = function() {
   var state = this.plugin.status;
@@ skipping 11 matching lines @@
   } 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) {
       error = remoting.ClientSession.ConnectionError.HOST_IS_OFFLINE;
     } else if (error == this.plugin.ERROR_SESSION_REJECTED) {
       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;
     }
     this.setState_(remoting.ClientSession.State.CONNECTION_FAILED);
   }
 };
 
 /**
  * @private
  * @param {remoting.ClientSession.State} state The new state for the session.
  * @return {void} Nothing.
  */
 remoting.ClientSession.prototype.setState_ = function(state) {
   var oldState = this.state;
   this.state = state;
   if (this.onStateChange) {
     this.onStateChange(oldState);
   }
 };
 
 /**
  * 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() {
   remoting.debug.log('window size changed: ' +
                      window.innerWidth + 'x' +
                      window.innerHeight);
   this.updateDimensions();
 };
 
 /**
@@ skipping 34 matching lines @@
   // Resize the plugin if necessary.
   this.plugin.width = this.plugin.desktopWidth * scale;
   this.plugin.height = this.plugin.desktopHeight * scale;
 
   // Position the container.
   // TODO(wez): We should take into account scrollbars when positioning.
   var parentNode = this.plugin.parentNode;
   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 + ',' +
                      parentNode.style.top + '-' +
                      this.plugin.width + 'x' + this.plugin.height + '.');
   this.plugin.setScaleToFit(remoting.scaleToFit);
 };
 
 /**
  * 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 {
     'video_bandwidth': this.plugin.videoBandwidth,
     'capture_latency': this.plugin.videoCaptureLatency,
     'encode_latency': this.plugin.videoEncodeLatency,
     'decode_latency': this.plugin.videoDecodeLatency,
     'render_latency': this.plugin.videoRenderLatency,
     'roundtrip_latency': this.plugin.roundTripLatency
   };
 };
-
-}());