Requiem for client_screen.js

remoting/webapp/crd/js/desktop_connected_view.js

Index: remoting/webapp/crd/js/desktop_connected_view.js
diff --git a/remoting/webapp/crd/js/desktop_connected_view.js b/remoting/webapp/crd/js/desktop_connected_view.js
index 5e6d2575d23ae88cbfe72fa35063ab680d68bb19..6db0c3ad74f1e7dcb0631022d1c6cfa6b3e45d49 100644
--- a/remoting/webapp/crd/js/desktop_connected_view.js
+++ b/remoting/webapp/crd/js/desktop_connected_view.js
@@ -60,9 +60,6 @@ remoting.DesktopConnectedView = function(session, container, host, mode,
    */
   this.onInitialized_ = onInitialized;
 
-  /** @type {function(boolean=):void} @private */
-  this.callOnFullScreenChanged_ = this.onFullScreenChanged_.bind(this)
-
   /** @private */
   this.callPluginLostFocus_ = this.pluginLostFocus_.bind(this);
   /** @private */
@@ -88,6 +85,9 @@ remoting.DesktopConnectedView = function(session, container, host, mode,
 
   /** @type {remoting.VideoFrameRecorder} @private */
   this.videoFrameRecorder_ = null;
+
+  /** private {base.Disposable} */
+  this.eventHooks_ = null;
 };
 
 // The mode of this session.
@@ -222,14 +222,23 @@ remoting.DesktopConnectedView.prototype.onPluginInitialized_ = function(
  * This is a callback that gets called when the window is resized.
  *
  * @return {void} Nothing.
+ * @private.
  */
-remoting.DesktopConnectedView.prototype.onResize = function() {
+remoting.DesktopConnectedView.prototype.onResize_ = function() {
   if (this.viewport_) {
     this.viewport_.onResize();
   }
 };
 
 /**
+ * Called when the app window is hidden.
+ * @return {void} Nothing.
+ */
+remoting.DesktopConnectedView.prototype.onVisibilityChanged_ = function() {
+  this.pauseVideo(document.hidden);
+};
+
+/**
  * Callback that the plugin invokes to indicate when the connection is
  * ready.
  *
@@ -269,7 +278,7 @@ remoting.DesktopConnectedView.prototype.removePlugin = function() {
  */
 remoting.DesktopConnectedView.prototype.updateClientSessionUi_ = function(
     clientSession) {
-  if (clientSession == null) {
+  if (clientSession === null) {
     if (remoting.windowFrame) {
       remoting.windowFrame.setDesktopConnectedView(null);
     }
@@ -283,9 +292,8 @@ remoting.DesktopConnectedView.prototype.updateClientSessionUi_ = function(
     document.body.classList.remove('connected');
     this.container_.removeEventListener(
         'mousemove', this.updateMouseCursorPosition_, true);
-    // Stop listening for full-screen events.
-    remoting.fullscreen.removeListener(this.callOnFullScreenChanged_);
-
+    base.dispose(this.eventHooks_);
+    this.eventHooks_ = null;
     base.dispose(this.viewport_);
     this.viewport_ = null;
   } else {
@@ -308,8 +316,20 @@ remoting.DesktopConnectedView.prototype.updateClientSessionUi_ = function(
     this.container_.addEventListener(
         'mousemove', this.updateMouseCursorPosition_, true);
     // Activate full-screen related UX.
-    remoting.fullscreen.addListener(this.callOnFullScreenChanged_);
     this.setFocusHandlers_();
+    var fullscreen = /** @type {chrome.Event} */ (remoting.fullscreen);

[Jamie, 2015/03/04 01:06:54]

remoting.fullscreen is not a chrome.Event, so you shouldn't cast it like this
(it would cause a runtime error if, for example, base.ChromeEventHook was ever
updated to use the hasListener property). The cleanest option is probably to add
an MiscEventHook class similar to ChromeEventHook (in fact, ChromeEventHook
could just be a typedef) that accepts a parameter of type {addListener:Function,
removeListener:Function}. If that doesn't appeal, then you could define
FullscreenEventHook in fullscreen.js. WDYT?

[kelvinp, 2015/03/04 21:02:21]

Good point.  I will create FullscreenEventHook.
I think the long term solution is to change the event hooking interface in
fullScreen.js to match other events listeners, so I am reluctant to create a
shared component MiscEventHook.

+    this.eventHooks_ = new base.Disposables(
+      // When a window goes full-screen, a resize event is triggered, but the
+      // Fullscreen.isActive call is not guaranteed to return true until the
+      // full-screen event is triggered. In apps v2, the size of the window's
+      // client area is calculated differently in full-screen mode, so register
+      // for both events.
+      new base.DomEventHook(window, 'resize', this.onResize_.bind(this), false),
+      new base.ChromeEventHook(fullscreen, this.onResize_.bind(this)),

[Jamie, 2015/03/04 01:06:54]

I think it would be clearer not to hook fullscreen here, but to explicitly call
onResize_ from onFullScreenChanged_. You can also move the comment about why
that's necessary to the call-site (and so ignore my next comment).

[kelvinp, 2015/03/04 21:02:20]

I like this approach better as well :)

+      new base.DomEventHook(document, 'visibilitychange',
+                            this.onVisibilityChanged_.bind(this), false),

[Jamie, 2015/03/04 01:06:54]

This hook is unrelated to the comment above. It would be better moved to the
bottom with its own comment (or just a blank line if you don't feel it warrants
one).

[kelvinp, 2015/03/04 21:02:21]

Done.

+      new base.ChromeEventHook(fullscreen, this.onFullScreenChanged_.bind(this))
+    );
   }
 };
 
@@ -580,4 +600,4 @@ remoting.DesktopConnectedView.prototype.handleDebugRegion = function(region) {
       this.debugRegionContainer_.appendChild(rect);
     }
   }
-}
+};