CastExtensionHandler for Chromoting Webapp

remoting/webapp/cast_extension_handler.js

Index: remoting/webapp/cast_extension_handler.js
diff --git a/remoting/webapp/cast_extension_handler.js b/remoting/webapp/cast_extension_handler.js
new file mode 100644
index 0000000000000000000000000000000000000000..4073f2b21d46131231c3c27ad5e1dba0c1201c15
--- /dev/null
+++ b/remoting/webapp/cast_extension_handler.js
@@ -0,0 +1,333 @@
+// Copyright 2014 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 Description of this file.
+ * Class handling interaction with the cast extension session of the Chromoting
+ * host. It receives and sends extension messages from/to the host through
+ * the client session. It uses the Google Cast Chrome Sender API library to
+ * interact with nearby Cast receivers.
+ *
+ * Once it establishes connection with a Cast device (upon user choice), it
+ * creates a session loads our registered receiver application and then becomes

[Jamie, 2014/08/13 23:21:02]

Comma after "session"

[aiguha, 2014/08/15 07:09:55]

Done.

+ * a message proxy between the host and cast device, helping negotiate
+ * their peer connection.
+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @constructor
+ * @param {!remoting.ClientSession} clientSession The client session to send
+ * cast extension messages to.
+ */
+remoting.CastExtensionHandler = function(clientSession) {

[aiguha, 2014/08/13 01:29:34]

We discussed calling this CastExtensionMessageProxy earlier today, but it later
occurred to me that that doesn't reflect the fact that it is responsible for
initializing the Cast API and acting as a "Cast Sender App", which wasn't too
well reflected in the name. What do you think?

[Jamie, 2014/08/13 23:21:02]

Acknowledged.

+  this.clientSession_ = clientSession;

[Jamie, 2014/08/13 23:21:03]

Please add @private.

[aiguha, 2014/08/15 07:09:53]

Done.

+
+  /** @type {chrome.cast.Session}
+    * @private */

[Jamie, 2014/08/13 23:21:02]

Nit: For multi-line doc comments, the format you've used for the method
signature is correct (with the opening and closing comment characters on the own
line).

[aiguha, 2014/08/15 07:09:54]

Acknowledged.

+  this.session = null;

[Jamie, 2014/08/13 23:21:03]

Private members should have a trailing underscore.

[aiguha, 2014/08/15 07:09:54]

Acknowledged.

+
+  /** @type {string}
+    * @private */
+  this.chromotingNamespace = 'urn:x-cast:com.chromoting.cast.all';

[Jamie, 2014/08/13 23:21:03]

If this is a constant, we conventionally prefix the name with a "k". Also, we
shouldn't really use the name Chromoting externally; since it's implicit by
virtual of being a class member, I think you can call this kCastNamespace.

[aiguha, 2014/08/15 07:09:54]

Done.

+
+  /** @type {Array}

[Jamie, 2014/08/13 23:21:03]

Can you be more specific about the type? Is it Array.<string>, for example?

[aiguha, 2014/08/15 07:09:54]

Done.

+    * @private */
+  this.messageQueue = [];
+
+  this.start();

[Jamie, 2014/08/13 23:21:02]

Generally, doing anything non-trivial in the ctor is a bad idea. If you must
call start() from the ctor, then it should probably be a private method.

[aiguha, 2014/08/15 07:09:54]

I think the call is unavoidable, because we need to load the script before doing
anything else, and that happens in start(). It is now a private method though.

[Jamie, 2014/08/15 18:45:43]

You can just call it explicitly immediately after calling new.

+};
+
+/**
+ * The id of the script node.
+ * @type {string}
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.SCRIPT_NODE_ID_ = 'cast-script-node';
+
+/**
+ * Attempts to load the Google Cast Chrome Sender API libary.
+ */
+remoting.CastExtensionHandler.prototype.start = function() {
+  var node = document.getElementById(this.SCRIPT_NODE_ID_);
+  if (node) {
+    console.error(
+        '### Multiple calls to CastExtensionHandler.start not expected.');

[aiguha, 2014/08/13 01:29:34]

The Sender API turns on logging by default, and though useful, it is quite
verbose. I was using the "###" to easily distinguish my own logs. 
If you prefer, I'll get rid of the ### and/or try to find a way to disable the
logging.

[Jamie, 2014/08/13 23:21:02]

I think we should disable logs by default, especially if they can easily by
turned on at runtime. I don't think the ### is necessary, since you're using
console.error, which is red and easily filterable in the dev tools.

For your other console logs, I would get rid of anything that's not an error,
unless it will fire infrequently and you feel strongly that it aids
debuggability.

[aiguha, 2014/08/15 07:09:55]

Sounds good, I've left in the logs that represent unexpected events/error
situations.

Looks like the chrome sender api has no way to disable logging, which makes me
think that the logging is really coming from my Cast Canary Extension installed
on my browser (i.e., the stable version won't log anything by default). 

+    return;
+  }
+
+  // Create a script node to load the Cast Sender API.
+  node = document.createElement('script');
+  node.id = this.SCRIPT_NODE_ID_;
+  node.src = "https://www.gstatic.com/cv/js/sender/v1/cast_sender.js";
+  node.type = 'text/javascript';
+  document.body.insertBefore(node, document.body.firstChild);

[Jamie, 2014/08/13 23:21:02]

I think this should happen after adding the event listeners. JS is
single-threaded, so I don't think there's a race if you do it in this order, but
it feels more natural.

[aiguha, 2014/08/15 07:09:54]

I was following wcs_loader.js's start() exactly here. Should I still change it?

[Jamie, 2014/08/15 18:45:43]

No, I'm pretty sure there's no race, so it can stay as-is.

+
+  /** @type {remoting.CastExtensionHandler} */
+  var that = this;
+  var onLoad = function() {
+    window['__onGCastApiAvailable'] = that.onGCastApiAvailable.bind(that);
+
+  };
+  var onLoadError = function(event) {
+    console.error("### Failed to load cast_sender.js.");
+  }
+  node.addEventListener('load', onLoad, false);
+  node.addEventListener('error', onLoadError, false);
+
+};
+
+/**
+ * Process cast-host-extension messages.

[Jamie, 2014/08/13 23:21:03]

I'm not sure how to parse "cast-host-extension messages", especially since it
doesn't have hyphens on the next line. Do you mean "Cast->host extension
messages" (ie, messages from Cast to the host)? Being consistent about
capitalizing Cast can help with this sort of ambiguity.

[aiguha, 2014/08/15 07:09:55]

Acknowledged.

+ * @param {string} msgString The cast host extension message data.
+ */
+remoting.CastExtensionHandler.prototype.onMessage = function(msgString) {

[Jamie, 2014/08/13 23:21:03]

I think this and most of the subsequent methods are private. They should be
annotated as such and have a trailing underscore in the name.

[aiguha, 2014/08/15 07:09:56]

This one is actually called from ClientSession, but I will change rest, thanks!

+  var message = getJsonObjectFromString(msgString);

[Jamie, 2014/08/13 23:21:03]

This can throw an exception, which you're not catching. Normally, functions from
typecheck.js are intended to be used inside a try...catch to at least log an
error if a type violation occurs.

[aiguha, 2014/08/15 07:09:53]

Acknowledged. 

Caught above in client_session as is done with gnubby auth handler.

+  console.log("### Received Message from Host: ", message);
+  // Save messages to send after a session is established.
+  this.messageQueue.push(message);
+  // Trigger the sending of pending messages, followed by the one just
+  // received.
+  if(this.session) {
+    this.sendPendingMessages();
+  }
+};
+
+/**
+ * Send cast-extension messages through the client session.
+ * @param {Object} response The JSON response to be sent to the host. The
+ * response object must contain the appropriate keys.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.sendMessageToHost_ =
+    function(response) {
+  this.clientSession_.sendCastExtensionMessage(response);
+};
+
+/**
+ * Send pending messages from the host to the receiver app.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.sendPendingMessages = function() {
+  var len = this.messageQueue.length;
+  for(var i = 0; i<len; i++) {
+    this.session.sendMessage(this.chromotingNamespace,
+                        this.messageQueue[i],
+                        this.sendMessageSuccess.bind(this),
+                        this.sendMessageFailure.bind(this));

[Jamie, 2014/08/13 23:21:02]

Nit: subsequent parameters should be either indented 4 characters relative to
the previous line, or aligned with the first parameter.

[aiguha, 2014/08/15 07:09:54]

Acknowledged.

+  }
+  this.messageQueue = [];
+};
+
+/**
+ * Event handler for '__onGCastApiAvailable' window event.

[Jamie, 2014/08/13 23:21:04]

Can you provide a bit more context about this event? When is it fired, what does
it signify, etc.

[aiguha, 2014/08/15 07:09:53]

Done.

+ *
+ * @param {boolean} loaded Represents if the API loaded succesfully.
+ * @param {Object} errorInfo Info if the API load failed.
+ */
+remoting.CastExtensionHandler.prototype.onGCastApiAvailable =
+    function(loaded, errorInfo) {
+  if (loaded) {
+    this.initializeCastApi();
+  } else {
+    console.log(errorInfo);
+  }
+};
+
+/**
+ * Initialize the Cast API.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.initializeCastApi = function() {
+  var applicationID = "8A1211E3";

[aiguha, 2014/08/13 01:29:34]

This Application ID is registered against the custom cast receiver application
I've built on the Google Cast SDK Dev Console. Until published, the receiver app
cannot be started on any device except those registered against the ID in the
console. Given that, I figured it would be safe to put this ID in the open. What
do you think?

[Jamie, 2014/08/13 23:21:03]

I think that's fine. If that was the reason for restricting this code review, I
think you can unrestrict it.

It feels like another constant with a similar purpose to the namespace, which
you've declared in the ctor. I think it would be cleaner to declare this there
as well. I realize it's only used in this one function, but logically I think it
belongs with the namespace; WDYT?

[aiguha, 2014/08/15 07:09:54]

I agree completely! I actually wanted to do that to when I took another look at
it.

+  var sessionRequest = new chrome.cast.SessionRequest(applicationID);
+  var apiConfig =
+      new chrome.cast.ApiConfig(sessionRequest,
+                                this.sessionListener.bind(this),
+                                this.receiverListener.bind(this),
+                                chrome.cast.AutoJoinPolicy.PAGE_SCOPED,
+                                chrome.cast.DefaultActionPolicy.CREATE_SESSION);
+  chrome.cast.initialize(
+      apiConfig, this.onInitSuccess.bind(this), this.onInitError.bind(this));
+};
+
+/**
+ * Callback for successful initialization of the Cast API.
+ */
+remoting.CastExtensionHandler.prototype.onInitSuccess = function() {
+  console.log("### Initialization Successful.");
+};
+
+/**
+ * Callback for failed initialization of the Cast API.
+ */
+remoting.CastExtensionHandler.prototype.onInitError = function() {
+  console.log("### Initialization Failed.");

[Jamie, 2014/08/13 23:21:03]

console.error

[aiguha, 2014/08/15 07:09:54]

Done.

+};
+
+/**
+ * Listener invoked when a session is created or connected by the SDK.
+ * Note: The requestSession method would not cause this callback to be invoked
+ * since it is passed its own listener.
+ * @param {chrome.cast.Session} e The resulting session, non-nullable.

[Jamie, 2014/08/13 23:21:02]

I think we can probably come up with a more descriptive name for this parameter
;)

Also, rather than commenting that it's non-nullable, you can express it as part
of the type annotation. I haven't tended to bother with that elsewhere, but if
the non-nullability concept is important, that's the place to declare it. You
could also assert it at the start of the function.

[aiguha, 2014/08/15 07:09:53]

I think I'll follow suit and not bother with it, because its actually a
guarantee from the API.

+ */
+remoting.CastExtensionHandler.prototype.sessionListener = function(e) {
+  console.log('### New Session. ID: ' + /** @type {string} */ (e.sessionId));
+  this.session = e;
+  if(this.session.media.length != 0) {
+    console.log("### Found " + this.session.media.length + " sessions.");
+    this.onMediaDiscovered('sessionListener', this.session.media[0]);

[Jamie, 2014/08/13 23:21:02]

Why are we only interested in the first session? I think an explanatory comment
would be useful.

[aiguha, 2014/08/15 07:09:53]

Done.

We only bother with the first because it's enough indication that something is
wrong. We never directly load media on the receiver from the sender, so this
should always be empty.

+  }
+  this.session.addMediaListener(
+      this.onMediaDiscovered.bind(this, 'addMediaListener'));
+  this.session.addUpdateListener(this.sessionUpdateListener.bind(this));
+  this.session.addMessageListener(this.chromotingNamespace,
+                             this.chromotingMessageListener.bind(this));

[Jamie, 2014/08/13 23:21:03]

Nit: Indentation

[aiguha, 2014/08/15 07:09:53]

Done.

+  this.session.sendMessage(this.chromotingNamespace,
+                      {subject : 'test', chromoting_data : 'Hello, Cast.'},
+                      this.sendMessageSuccess.bind(this),
+                      this.sendMessageFailure.bind(this));

[Jamie, 2014/08/13 23:21:03]

Nit: Indentation

[aiguha, 2014/08/15 07:09:55]

Done.

+  this.sendPendingMessages();
+};
+
+/**
+ * Listener invoked when a media session is created by another sender.
+ * @param {string} how How this callback was triggered.
+ * @param {chrome.cast.media.Media} media The media item discovered.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.onMediaDiscovered =
+    function(how, media) {
+  console.log('### New Media Session ID: ' + media.mediaSessionId);
+};

[Jamie, 2014/08/13 23:21:04]

Do this function need to exist at all? I was assuming it was a callback from the
Cast API that we're not interested in, but it seems you're calling it
explicitly.

[aiguha, 2014/08/15 07:09:55]

It is a callback from the API, but it is also called explictly when we find an
existing session with media since the API doesn't callback in that case. It
should never be fired because we shouldn't find media on our sessions. If it is
fired,  we log an error.

+
+/**
+ * Listener invoked when a cast extension message was sent to the cast device
+ * successfully.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.sendMessageSuccess = function() {
+  console.log('### Sent Message Successfully.');
+};
+
+/**
+ * Listener invoked when a cast extension message failed to be sent to the cast
+ * device.
+ * @param {Object} error The error.
+ * @private
+ */
+remoting.CastExtensionHandler.prototype.sendMessageFailure = function(error) {
+  console.error('### Failed to Send Message.', error);
+};
+
+/**
+ * Listener invoked when a cast extension message was sent to the cast device
+ * successfully.

[Jamie, 2014/08/13 23:21:02]

This is the same comment as for sendMessageSuccess; is that intentional?

[aiguha, 2014/08/15 07:09:54]

Nope, I copied it over and forgot to s/sent to/received from. Thanks!

+ * @param {string} ns The namespace of the message received.

[Jamie, 2014/08/13 23:21:03]

You're not using the namespace. Shouldn't you be checking it against the
expected value?

[aiguha, 2014/08/15 07:09:54]

Done.

+ * @param {string} message The stringified JSON message received.
+ */
+remoting.CastExtensionHandler.prototype.chromotingMessageListener =
+    function(ns, message) {
+  var messageObj = getJsonObjectFromString(message);
+  console.log("### Received Message from Cast: " + ns + "-" + messageObj);
+  this.sendMessageToHost_(messageObj);
+};
+
+/**
+ * Listener invoked when there updates to the current session.
+ *
+ * @param {boolean} isAlive True if the session is still alive.
+ */
+remoting.CastExtensionHandler.prototype.sessionUpdateListener =
+    function(isAlive) {
+  var message = isAlive ? '### Session Updated' : '### Session Removed';
+  message += ': ' + this.session.sessionId +'.';
+  console.log(message);
+};
+
+/**
+ * Listener invoked when the availability of a Cast receiver that supports
+ * the application in sessionRequest is known or changes.
+ *
+ * @param {chrome.cast.ReceiverAvailability} e Describes receiver availability.

[Jamie, 2014/08/13 23:21:02]

Parameter name.

[aiguha, 2014/08/15 07:09:54]

Done.

+ */
+remoting.CastExtensionHandler.prototype.receiverListener = function(e) {
+  if( e === chrome.cast.ReceiverAvailability.AVAILABLE) {

[Jamie, 2014/08/13 23:21:03]

Space after 'if', not before 'e'.

[aiguha, 2014/08/15 07:09:54]

Done.

+    console.log("### Receiver(s) Found.");
+  }
+  else {

[Jamie, 2014/08/13 23:21:02]

No newline before 'else'.

[aiguha, 2014/08/15 07:09:55]

Done.

+    console.log("### No Receivers Available.");
+  }
+};
+
+/**
+ * Launches the associated receiver application by requesting that it be created
+ * or joined on the Cast device. By default, the SessionRequest passed during
+ * initialization is used.

[Jamie, 2014/08/13 23:21:02]

I don't really understand what this function does. You say that the
SessionRequest passed during initialization is used by default, which suggests
to me that this method should allow an alternative to be specified, but it
doesn't. I think this comment needs to be more explicit.

[aiguha, 2014/08/15 07:09:55]

Clarified the comment. It is supposed to use the SessionRequest specified in the
init phase. You COULD change the app being launched here, but we don't want to
that.

+ * Note: This method is intended to be used as a click listener for a custom
+ * cast button on the webpage. We currently use the default cast button in
+ * Chrome, so this method is unused.
+ */
+remoting.CastExtensionHandler.prototype.launchApp = function() {
+  console.log("### Launching Cast App.");
+  chrome.cast.requestSession(this.onRequestSessionSuccess.bind(this),
+                             this.onLaunchError.bind(this));
+};
+
+/**
+ * Listener invoked when the chrome.cast.requestSession is successful.

[Jamie, 2014/08/13 23:21:03]

s/the//
s/is successful/completes successfully/

[aiguha, 2014/08/15 07:09:55]

Done.

+ *
+ * @param {chrome.cast.Session} e The requested session.
+ */
+remoting.CastExtensionHandler.prototype.onRequestSessionSuccess = function (e) {
+  console.log("### Successfully created session: " + e.sessionId);
+  this.session = e;
+  this.session.addUpdateListener(this.sessionUpdateListener.bind(this));
+  if(this.session.media.length != 0) {
+    this.onMediaDiscovered('onRequestSession', this.session.media[0]);
+  }
+  this.session.addMediaListener(
+      this.onMediaDiscovered.bind(this, 'addMediaListener'));
+  this.session.addMessageListener(this.chromotingNamespace,
+                                  this.chromotingMessageListener.bind(this));
+};
+
+/**
+ * Listener invoked when the launchApp() fails.
+ * @param {chrome.cast.Error} error The error code.
+ */
+remoting.CastExtensionHandler.prototype.onLaunchError = function(error) {
+  console.error("### Error Casting to Receiver.", error);
+};
+
+/**
+ * Stops the running receiver application associated with the session.
+ * TODO(aiguha): When the user disconnects using the blue drop down bar,
+ * the client session should notify the CastExtensionHandler, which should
+ * call this method to close the session with the Cast device.
+ */
+remoting.CastExtensionHandler.prototype.stopApp = function() {
+  this.session.stop(this.onStopAppSuccess.bind(this),
+               this.onStopAppError.bind(this));
+};
+
+/**
+ * Listener invoked when the receiver application is stopped successfully.
+ */
+remoting.CastExtensionHandler.prototype.onStopAppSuccess = function() {
+  console.log('### Session Stopped.');
+};
+
+/**
+ * Listener invoked when we fail to stop the receiver application.
+ *
+ * @param {chrome.cast.Error} error The error code.
+ */
+remoting.CastExtensionHandler.prototype.onStopAppError = function(error) {
+  console.error('### Error Stopping App: ', error);
+};