Requiem for client_screen.js

remoting/webapp/crd/js/desktop_connected_view.js

 // Copyright 2015 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 handling user-facing aspects of the client session.
  */
 
 'use strict';
@@ skipping 42 matching lines @@
 
   /** @type {string} @private */
   this.defaultRemapKeys_ = defaultRemapKeys;
 
   /**
    * Called when the UI is finished initializing.
    * @type {function(remoting.Error, remoting.ClientPlugin):void}
    */
   this.onInitialized_ = onInitialized;
 
-  /** @type {function(boolean=):void} @private */
-  this.callOnFullScreenChanged_ = this.onFullScreenChanged_.bind(this)
-
   /** @private */
   this.callPluginLostFocus_ = this.pluginLostFocus_.bind(this);
   /** @private */
   this.callPluginGotFocus_ = this.pluginGotFocus_.bind(this);
   /** @type {Element} @private */
   this.debugRegionContainer_ =
       this.container_.querySelector('.debug-region-container');
 
   /** @type {Element} @private */
   this.mouseCursorOverlay_ =
       this.container_.querySelector('.mouse-cursor-overlay');
 
   /** @private {remoting.DesktopViewport} */
   this.viewport_ = null;
 
   /** @type {Element} */
   var img = this.mouseCursorOverlay_;
   /** @param {Event} event @private */
   this.updateMouseCursorPosition_ = function(event) {
     img.style.top = event.offsetY + 'px';
     img.style.left = event.offsetX + 'px';
   };
 
   /** @type {remoting.VideoFrameRecorder} @private */
   this.videoFrameRecorder_ = null;
+
+  /** private {base.Disposable} */
+  this.eventHooks_ = null;
 };
 
 // The mode of this session.
 /** @enum {number} */
 remoting.DesktopConnectedView.Mode = {
   IT2ME: 0,
   ME2ME: 1,
   APP_REMOTING: 2
 };
 
@@ skipping 114 matching lines @@
 
   this.plugin_.setMouseCursorHandler(this.updateMouseCursorImage_.bind(this));
 
   this.onInitialized_(remoting.Error.NONE, this.plugin_);
 };
 
 /**
  * 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.
  *
  * @param {boolean} ready True if the connection is ready.
  */
 remoting.DesktopConnectedView.prototype.onConnectionReady = function(ready) {
   if (!ready) {
     this.container_.classList.add('session-client-inactive');
   } else {
     this.container_.classList.remove('session-client-inactive');
@@ skipping 19 matching lines @@
 
   this.updateClientSessionUi_(null);
 };
 
 /**
  * @param {remoting.ClientSession} clientSession The active session, or null if
  *     there is no connection.
  */
 remoting.DesktopConnectedView.prototype.updateClientSessionUi_ = function(
     clientSession) {
-  if (clientSession == null) {
+  if (clientSession === null) {
     if (remoting.windowFrame) {
       remoting.windowFrame.setDesktopConnectedView(null);
     }
     if (remoting.toolbar) {
       remoting.toolbar.setDesktopConnectedView(null);
     }
     if (remoting.optionsMenu) {
       remoting.optionsMenu.setDesktopConnectedView(null);
     }
 
     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 {
     var scrollerElement = document.getElementById('scroller');
     this.viewport_ = new remoting.DesktopViewport(
         scrollerElement || document.body,
         this.plugin_.hostDesktop(),
         this.host_.options);
     if (remoting.windowFrame) {
       remoting.windowFrame.setDesktopConnectedView(this);
     }
     if (remoting.toolbar) {
       remoting.toolbar.setDesktopConnectedView(this);
     }
     if (remoting.optionsMenu) {
       remoting.optionsMenu.setDesktopConnectedView(this);
     }
 
     document.body.classList.add('connected');
     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))
+    );
   }
 };
 
 /**
  * Constrains the focus to the plugin element.
  * @private
  */
 remoting.DesktopConnectedView.prototype.setFocusHandlers_ = function() {
   this.plugin_.element().addEventListener(
       'focus', this.callPluginGotFocus_, false);
@@ skipping 250 matching lines @@
     for (var i = 0; i < rects.length; ++i) {
       var rect = document.createElement('div');
       rect.classList.add('debug-region-rect');
       rect.style.left = rects[i][0] + 'px';
       rect.style.top = rects[i][1] +'px';
       rect.style.width = rects[i][2] +'px';
       rect.style.height = rects[i][3] + 'px';
       this.debugRegionContainer_.appendChild(rect);
     }
   }
-}
+};