Separate host desktop related functionality into remoting.HostDesktop

remoting/webapp/crd/js/client_plugin_host_desktop_impl.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
+ * Provides an interface to manage the Host Desktop of a remoting session.
+ */
+
+var remoting = remoting || {};
+remoting.ClientPlugin = remoting.ClientPlugin || {};
+
+(function() {
+
+'use strict';
+
+/**
+ * @param {remoting.ClientPluginImpl} plugin
+ * @param {function(Object):void} postMessageCallback Callback to post a message
+ *   to the Client Plugin.
+ *
+ * @implements {remoting.HostDesktop}
+ * @extends {base.EventSourceImpl}
+ * @constructor
+ */
+remoting.ClientPlugin.HostDesktopImpl = function(plugin, postMessageCallback) {

[Jamie, 2015/02/18 23:06:19]

The fact that you need an explicit reference to the plugin and also a function
for posting a message to it is a red flag. If the plugin is intrinsically
needed, then there's no point abstracting its postMessage method. However, I
don't think it is needed if you adopt some of my other suggestions.

Then, if this is no longer tied so closely to the ClientPlugin implementation,
it doesn't need to be a nested class any more.

[kelvinp, 2015/02/19 20:58:15]

As discussed, the intent of this class is to separate the functionality of the
plugin into logical groups.  I think it makes sense for this class to live under
the ClientPlugin namespace, so that the reader knows that this it is closely
related to the ClientPlugin.

postMessageCallback is passed in as I don't want the other clients of
clientPlugin to call postMessage() on clientPlugin.
Alternatively, this could be done via a private interface, but would probably be
an overkill for now.

+  /** @private */
+  this.plugin_ = plugin;
+  /** @private */
+  this.width_ = 0;
+  /** @private */
+  this.height_ = 0;
+  /** @private */
+  this.xDpi_ = 96;
+  /** @private */
+  this.yDpi_ = 96;
+  /** @private */
+  this.hasResizeRateLimit_ = false;
+  /** @private */
+  this.postMessageCallback_ = postMessageCallback;
+
+  this.defineEvents(Object.keys(remoting.HostDesktop.Events));

[Jamie, 2015/02/18 23:06:19]

s/Object.keys/base.values/

[kelvinp, 2015/02/19 20:58:15]

Done.

+};
+base.extend(remoting.ClientPlugin.HostDesktopImpl, base.EventSourceImpl);
+
+/** @return {boolean} */
+remoting.ClientPlugin.HostDesktopImpl.prototype.hasResizeRateLimit =
+    function() {
+  return this.hasResizeRateLimit_;
+};
+
+/** @return {boolean} Whether the host supports desktop resizing. */
+remoting.ClientPlugin.HostDesktopImpl.prototype.isResizable = function() {
+  return this.plugin_.hasFeature(
+      remoting.ClientPlugin.Feature.NOTIFY_CLIENT_RESOLUTION);
+};
+
+/** @return {{width:number, height:number, xDpi:number, yDpi:number}} */
+remoting.ClientPlugin.HostDesktopImpl.prototype.getDimensions = function() {
+  return {
+    width: this.width_,
+    height: this.height_,
+    xDpi: this.xDpi_,
+    yDpi: this.yDpi_
+  };
+};
+
+/**
+ * @param {number} width
+ * @param {number} height
+ * @param {number} device_scale

[Jamie, 2015/02/18 23:06:19]

The API declares this parameter as |dpi|, and it refers to the DPI to set on the
host. |device_scale| is used elsewhere to refer to the scale (ie, dpi/96) of the
client device.

[kelvinp, 2015/02/19 20:58:15]

Done.

+ */
+remoting.ClientPlugin.HostDesktopImpl.prototype.resize = function(
+    width, height, device_scale) {
+  if (this.isResizable()) {
+    var dpi = Math.floor(device_scale * 96);
+    this.postMessageCallback_({
+      method: 'notifyClientResolution',
+      data: {
+        width: Math.floor(width * device_scale),
+        height: Math.floor(height * device_scale),
+        x_dpi: dpi,
+        y_dpi: dpi
+      }
+    });
+  }
+};
+
+/**
+ * @param {Array<string>} capabilities
+ */
+remoting.ClientPlugin.HostDesktopImpl.prototype.onSetCapabilities = function(

[Jamie, 2015/02/18 23:06:19]

I think this method is only used to set hasResizeRateLimit, so if you get rid of
that, this method can go as well.

[kelvinp, 2015/02/19 20:58:15]

Done.

+    capabilities) {
+  this.hasResizeRateLimit_ = this.plugin_.hasCapability(
+      remoting.ClientSession.Capability.RATE_LIMIT_RESIZE_REQUESTS);
+};
+
+/**
+ * This function is called by |this.plugin_| when the size of the host
+ * desktop is changed.
+ *
+ * @param {remoting.ClientPluginMessage} message
+ */
+remoting.ClientPlugin.HostDesktopImpl.prototype.onSizeUpdated = function(

[Jamie, 2015/02/18 23:06:19]

This method is not declared on the interface and should take width, height and
dpis explicitly. Decoding them here couples this class to ClientPlugin more
tightly than it needs to be.

[kelvinp, 2015/02/19 20:58:15]

This class is under the clientPlugin namespace and is by design coupled.
As long as we have a well defined type for the ClientPluginMessage, I think it
should be fine here.

+    message) {
+  this.width_ = getNumberAttr(message.data, 'width');
+  this.height_ = getNumberAttr(message.data, 'height');
+  this.xDpi_ = getNumberAttr(message.data, 'x_dpi', 96);
+  this.yDpi_ = getNumberAttr(message.data, 'y_dpi', 96);
+  this.raiseEvent(remoting.HostDesktop.Events.sizeChanged);
+};
+
+/**
+ * This function is called by |this.plugin_| when the shape of the host
+ * desktop is changed.
+ *
+ * @param {remoting.ClientPluginMessage} message
+ * @return {Array<{left:number, top:number, width:number, height:number}>}
+ *    rectangles of the desktop shape.
+ */
+remoting.ClientPlugin.HostDesktopImpl.prototype.onShapeUpdated =
+    function(message) {
+  var shapes = getArrayAttr(message.data, 'rects');
+  var rects = shapes.map(
+    /** @param {Array.<number>} shape */
+    function(shape) {
+      if (!Array.isArray(shape) || shape.length != 4) {
+        throw 'Received invalid onDesktopShape message';
+      }
+      var rect = {};

[Jamie, 2015/02/18 23:06:19]

Optional: Elsewhere, we have code to force left, right, width and height to be
integers. It might make sense for that to happen here.

[kelvinp, 2015/02/19 20:58:15]

Done.

+      rect.left = shape[0];
+      rect.top = shape[1];
+      rect.width = shape[2];
+      rect.height = shape[3];
+      return rect;
+  });
+
+  this.raiseEvent(remoting.HostDesktop.Events.shapeChanged, rects);
+  return rects;
+};
+
+}());