HostTableEntry refactor

remoting/webapp/crd/js/local_host_section.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.
+
+var remoting = remoting || {};
+
+(function() {
+
+'use strict';
+
+/**
+ * @param {HTMLElement} rootElement
+ * @param {remoting.LocalHostSection.Controller} controller
+ *
+ * @constructor
+ * @implements {base.Disposable}
+ */
+remoting.LocalHostSection = function(rootElement, controller) {
+  /** @private */
+  this.rootElement_ = rootElement;
+  /** @private */
+  this.controller_ = controller;
+  /** @private {remoting.Host} */
+  this.host_ = null;
+  /** @private {remoting.HostController.State} */
+  this.state_ = remoting.HostController.State.UNKNOWN;
+
+  var hostContainer = rootElement.querySelector('.host-entry');
+  /** @private */
+  this.hostTableEntry_ = new remoting.HostTableEntry(
+      parseInt(chrome.runtime.getManifest().version, 10),
+      remoting.connectMe2Me,
+      this.rename_.bind(this));
+  hostContainer.appendChild(this.hostTableEntry_.element());
+
+  var startButton = rootElement.querySelector('#start-daemon');
+  var stopButton = rootElement.querySelector('#stop-daemon');
+  var changePINButton = rootElement.querySelector('#change-daemon-pin');

[Jamie, 2015/02/27 18:19:41]

Can you query these by class instead of id? Querying by id breaks the
reusability abstraction that you gain by passing in the root element to the
ctor.

[kelvinp, 2015/03/03 21:57:54]

Done.

+
+  /** @private */
+  this.eventHooks_ = new base.Disposables(
+    new base.DomEventHook(startButton, 'click', controller.start, false),
+    new base.DomEventHook(stopButton, 'click', controller.stop, false),
+    new base.DomEventHook(changePINButton, 'click', controller.changePIN, false)

[Jamie, 2015/02/27 18:19:41]

I think these are all missing bind()s.

[kelvinp, 2015/03/03 21:57:53]

Done.

+  );
+
+  /** @private */
+  this.hasError_ = false;
+};
+
+remoting.LocalHostSection.prototype.dispose = function() {
+  base.dispose(this.eventHooks_);
+  this.eventHooks_ = null;
+};
+
+/**
+ * @param {remoting.Host} host
+ * @param {remoting.HostController.State} state
+ * @param {boolean} hasError Whether the host list is in an error state.
+ */
+remoting.LocalHostSection.prototype.setModel = function(host, state, hasError) {
+  this.host_ = host;
+  this.state_ = state;
+  this.hasError_ = hasError;
+  this.updateUI_();
+};
+
+/**
+ * @return {?string}
+ */
+remoting.LocalHostSection.prototype.getHostId = function() {
+  return this.host_ ? this.host_.hostId : null;
+};
+
+/** @return {boolean} */
+remoting.LocalHostSection.prototype.isEnabled_ = function() {
+  return (this.state_ == remoting.HostController.State.STARTING) ||
+      (this.state_ == remoting.HostController.State.STARTED);
+};
+
+/** @return {boolean} */
+remoting.LocalHostSection.prototype.canChangeState = function() {
+  // The local host cannot be stopped or started if the host controller is not
+  // implemented for this platform. Additionally, it cannot be started if there
+  // is an error (in many error states, the start operation will fail anyway,
+  // but even if it succeeds, the chance of a related but hard-to-diagnose
+  // future error is high).
+  var state = this.state_;
+  return (state != remoting.HostController.State.NOT_IMPLEMENTED) &&
+         (state != remoting.HostController.State.UNKNOWN) &&
+         (state != remoting.HostController.State.NOT_INSTALLED ||
+          remoting.isMe2MeInstallable()) &&
+         (this.isEnabled_() || !this.hasError_);

[Jamie, 2015/02/27 18:19:41]

This conditional is hard to parse. Can you break it into assignments to
appropriately named variables, and then return a simple boolean function of
those variables?

[kelvinp, 2015/03/03 21:57:54]

Done.

+};
+
+/** @private */
+remoting.LocalHostSection.prototype.updateUI_ = function() {
+  this.hostTableEntry_.setHost(this.host_);
+
+  // Disable elements.
+  var enabled = this.isEnabled_();
+  var canChangeLocalHostState = this.canChangeState();
+  remoting.updateModalUi(enabled ? 'enabled' : 'disabled', 'data-daemon-state');
+  this.rootElement_.hidden = !canChangeLocalHostState;
+};
+
+remoting.LocalHostSection.prototype.rename_ = function() {
+  return this.controller_.rename(this.hostTableEntry_);
+};
+
+/**
+ * @constructor
+ * @param {remoting.HostList} hostList
+ */
+remoting.LocalHostSection.Controller = function(hostList) {
+  /** @private */
+  this.hostList_ = hostList;
+};
+
+remoting.LocalHostSection.Controller.prototype.start = function() {
+  remoting.hostSetupDialog.showForStart();

[Jamie, 2015/02/27 18:19:41]

Maybe pass the HostSetupDialog to the ctor for better encapsulation?

[kelvinp, 2015/03/03 21:57:54]

Done.

+};
+
+remoting.LocalHostSection.Controller.prototype.stop = function() {
+  remoting.hostSetupDialog.showForStop();
+};
+
+remoting.LocalHostSection.Controller.prototype.changePIN = function() {
+  remoting.hostSetupDialog.showForPin();
+};
+
+/** @param {remoting.HostTableEntry} host */
+remoting.LocalHostSection.Controller.prototype.rename = function(host) {
+  this.hostList_.renameHost(host);
+};
+
+}());
+