Hangout remote desktop part I - It2Me mode

remoting/webapp/hrd_helper_session.js

+// 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
+ * Class to communicate with the it2me background service via chrome runtime
+ * messages.

[Jamie, 2014/08/05 21:15:14]

To do what? AFAICT, this class does two things:

1. Forwards session state notifications (to where?)
2. Closes the window when the session terminates.

I think that should be made explicit in the fileoverview.

[kelvinp, 2014/08/07 18:03:25]

Done.

+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/**
+ * @constructor
+ */
+remoting.HRDHelperSession = function() {

[Jamie, 2014/08/05 21:15:14]

Can you come up with something more descriptive than "Helper"? It's not clear
from the code or comments what this class does.

[kelvinp, 2014/08/07 18:03:25]

Done.

+  /** @type {chrome.extension.Port} */

[Jamie, 2014/08/05 21:15:14]

Add @private?

[kelvinp, 2014/08/07 18:03:25]

Done.

+  this.port_ = null;
+  this.onSessionStateChangedHandler_ = this.onSessionStateChanged_.bind(this);

[Jamie, 2014/08/05 21:15:13]

You only use this in one place, so there's no need to save it to a member
variable.

[kelvinp, 2014/08/07 18:03:25]

Done.

+};
+
+remoting.HRDHelperSession.prototype.init = function() {
+  this.port_ = chrome.runtime.connect({name: 'it2me.helper.webapp'});

[Jamie, 2014/08/05 21:15:13]

Are you missing an extension id here? Without one, you're forwarding state
change notifications to yourself, which doesn't seem right.

[kelvinp, 2014/08/07 18:03:25]

This is intended.  According to the documentation, when the extension id is
omitted.  The message will be broadcasted to all pages (background/other window)
within the app.  This is the recommended way for an app to communicate with its
background page. 

+
+  /** @type {base.EventSource} */
+  var hrdSessionEvents = remoting.hrdSessionEvents;
+  hrdSessionEvents.addEventListener('sessionStateChanged',
+      this.onSessionStateChangedHandler_);
+};
+
+/**
+ * @param {remoting.hrdSessionEvents.SessionStates} state
+ */
+remoting.HRDHelperSession.prototype.onSessionStateChanged_ = function(state) {
+  var State = remoting.hrdSessionEvents.SessionStates;
+  try {
+    this.port_.postMessage({method: 'sessionStateChanged', state: state});
+  } catch (e) {

[Jamie, 2014/08/05 21:15:13]

This ignores any errors. Is that what you want? If so, then I think a comment
here explaining why that's safe would be useful.

[kelvinp, 2014/08/07 18:03:25]

Done.

+  } finally {
+    if (state === State.ERROR || state === State.CLOSED) {
+      // close the current window
+      if (remoting.isAppsV2) {
+        chrome.app.window.current().close();
+      } else {
+        window.close();
+      }
+    }
+  }
+};