bluetooth: Add device details page with basic properties to internals page.

chrome/browser/resources/bluetooth_internals/device_details_page.js

Index: chrome/browser/resources/bluetooth_internals/device_details_page.js
diff --git a/chrome/browser/resources/bluetooth_internals/device_details_page.js b/chrome/browser/resources/bluetooth_internals/device_details_page.js
new file mode 100644
index 0000000000000000000000000000000000000000..9f00805541eea8c299d015ba14afe368ec049870
--- /dev/null
+++ b/chrome/browser/resources/bluetooth_internals/device_details_page.js
@@ -0,0 +1,210 @@
+// Copyright 2017 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.
+
+/**
+ * Javascript for DeviceDetailsPage which displays all of the details of a
+ * device. The page is generated and managed dynamically in bluetooth_internals.
+ * served from chrome://bluetooth-internals/.
+ */
+
+cr.define('device_details_page', function() {
+  /** @const */ var Page = cr.ui.pageManager.Page;
+  /** @const */ var Snackbar = snackbar.Snackbar;
+  /** @const */ var SnackbarType = snackbar.SnackbarType;
+
+  /**
+   * Property names that will be displayed in the ObjectFieldSet which contains
+   * the DeviceInfo object.
+   */
+  var PROPERTY_NAMES = {
+    name: 'Name',
+    address: 'Address',
+    is_gatt_connected: 'GATT Connected',
+    'rssi.value': 'Latest RSSI',
+    'services.length': 'Services',
+  };
+
+  /**
+   * Page that displays all of the details of a device. This page is generated
+   * and managed dynamically in bluetooth_internals. This page is the owner of
+   * the DevicePtr when a connection is created. Therefore, it manages the
+   * connection to the Bluetooth device and performs all Device interface
+   * related actions.
+   * @constructor
+   * @param {string} id
+   * @param {!interfaces.BluetoothDevice.DeviceInfo} deviceInfo
+   * @extends {cr.ui.pageManager.Page}
+   */
+  function DeviceDetailsPage(id, deviceInfo) {
+    Page.call(this, id, deviceInfo.name_for_display, id);
+
+    this.deviceInfo = deviceInfo;
+
+    /** @type {interfaces.BluetoothDevice.Device.ptrClass} */
+    this.devicePtr = null;
+    /** @private {!device_collection.ConnectionStatus} */
+    this.status_ = device_collection.ConnectionStatus.DISCONNECTED;
+
+    this.pageDiv.appendChild(
+        document.importNode($('device-details-template').content,
+                            true /* deep */));
+
+    this.pageDiv.querySelector('.forget-btn').addEventListener('click',

[dpapad, 2017/01/12 18:37:09]

Nit: Move 'click' parameter to the next line? This violates the "rectangle
rule", see
https://google.github.io/styleguide/jsguide.html#formatting-rectangle-rule.

[mbrunson, 2017/01/12 22:16:28]

Done.

+        function() {
+          this.disconnect();
+          this.pageDiv.dispatchEvent(new CustomEvent('forgetpressed', {
+              detail: {
+                address: this.deviceInfo.address,
+              },
+          }));
+        }.bind(this));
+
+    this.disconnectBtn_ = this.pageDiv.querySelector('.disconnect-btn');
+    this.disconnectBtn_.addEventListener('click', function() {
+      if (this.devicePtr) {

[dpapad, 2017/01/12 18:37:09]

Nit(optional): Perhaps compress lines 65-69 as follows?

this.devicePtr !== null ? this.disconnect() : this.connect();

[mbrunson, 2017/01/12 22:16:28]

Done.

+        this.disconnect();
+        return;
+      }
+      this.connect();
+    }.bind(this));
+
+    this.deviceFieldSet = new object_fieldset.ObjectFieldSet();

[dpapad, 2017/01/12 18:37:09]

This constructor can be cleaned up. Currently it mixes up
initialization/declaration of variables with actual work (registering listeners,
setting up the DOM). Can we separate those as follows?

function DeviceDetailsPage(id, deviceInfo) {
 this.foo = 'foo';
 this.bar = 'bar';
 ... // Remaining declarations/initialization of member variables here.

 // Set up DOM.
 this.pageDiv.appendChild(
     document.importNode($('device-details-template').content,
                         true /* deep */));
 this.pageDiv.querySelector('.device-details').appendChild(
     this.deviceFieldSet);

 // Add event listeners.
 this.pageDiv.querySelector('.forget-btn').addEventListener('click', ...);
 ...

 this.redraw();
}

[mbrunson, 2017/01/12 22:16:28]

Done.

+    this.deviceFieldSet.setPropertyDisplayNames(PROPERTY_NAMES);
+    this.pageDiv.querySelector('.device-details').appendChild(
+        this.deviceFieldSet);
+
+    this.redraw();
+  }
+
+  DeviceDetailsPage.prototype = {
+    __proto__: Page.prototype,
+
+    /** Creates a connection to the Bluetooth device. */
+    connect: function() {
+      if (this.status_ !== device_collection.ConnectionStatus.DISCONNECTED)
+        return;
+
+      this.updateConnectionStatus_(
+          device_collection.ConnectionStatus.CONNECTING);
+      this.connecting_ = true;
+
+      adapter_broker.getAdapterBroker().then(function(adapterBroker) {
+          adapterBroker.connectToDevice(this.deviceInfo.address).then(

[dpapad, 2017/01/12 18:37:09]

Wrong indent, 4 instead of 2? But also, you can remove one more level of
indentation by chaining as follows

adapter_broker.getAdapterBroker().then(
    function(adapterBroker) {
      return adapterBroker.connectToDevice(this.deviceInfo.address);
    }.bind(this)).then(
    function(devicePtr) {
      ...
      return this.devicePtr.getServices();
    }.bind(this)).then(
    function(response) {
      ...
    }.bind(this)).catch(
    function(error) {
      ...
    }.bind(this));

[mbrunson, 2017/01/12 22:16:28]

Done.

+              function(devicePtr) {
+                this.devicePtr = devicePtr;
+
+                this.updateConnectionStatus_(
+                    device_collection.ConnectionStatus.CONNECTED);
+
+                // Fetch services asynchronously.
+                return this.devicePtr.getServices();
+              }.bind(this)).then(function(response) {
+                this.deviceInfo.services = response.services;
+                this.redraw();
+                this.fireDeviceInfoChanged_();
+              }.bind(this)).catch(function(error) {
+                // If a connection error occurs while fetching the services, the
+                // devicePtr reference must be removed.
+                if (this.devicePtr) {
+                  this.devicePtr.disconnect();
+                  this.devicePtr = null;
+                }
+
+                Snackbar.show(
+                    this.deviceInfo.name_for_display + ': ' + error.message,
+                    SnackbarType.DANGER, 'Retry', function() {

[dpapad, 2017/01/12 18:37:09]

Nit: No need to wrap connect() to a new function anonymous function.

Snackbar.show(
    this.deviceInfo.name_for_display + ': ' + error.message,
    SnackbarType.DANGER, 'Retry', this.connect.bind(this));

[mbrunson, 2017/01/12 22:16:28]

Done.

+                      this.connect();
+                    }.bind(this));
+
+                this.updateConnectionStatus_(
+                    device_collection.ConnectionStatus.DISCONNECTED);
+              }.bind(this));
+      }.bind(this));
+    },
+
+    /** Disconnects the page from the Bluetooth device. */
+    disconnect: function() {
+      if (!this.devicePtr) return;
+
+      this.devicePtr.disconnect();
+      this.devicePtr = null;
+      this.updateConnectionStatus_(
+          device_collection.ConnectionStatus.DISCONNECTED);
+    },
+
+    /** Redraws the contents of the page with the current |deviceInfo|. */
+    redraw: function() {
+      var isConnected = this.deviceInfo.is_gatt_connected;
+
+      // Update status if connection was dropped.
+      if (!isConnected) this.disconnect();
+      var connectedText = isConnected ? 'Connected' : 'Not Connected';
+
+      var rssi = this.deviceInfo.rssi;
+      var services = this.deviceInfo.services;
+
+      var deviceViewObj = {
+        name: this.deviceInfo.name_for_display,
+        address: this.deviceInfo.address,
+        is_gatt_connected: connectedText,
+        'rssi.value': (rssi && rssi.value) || 'Unknown',

[dpapad, 2017/01/12 18:37:09]

Is 0 (zero) a legit value for rssi.value? If so, this will incorrectly display
"Unknown" instead of 0.

[mbrunson, 2017/01/12 22:16:28]

It's nearly impossible to hit 0 (I think -25 is an amazingly good value for
received signal strength). That being said, I will change this to account for
it. Done.

+        'services.length': (services && services.length) || 'Unknown',

[dpapad, 2017/01/12 18:37:09]

Same here. Should a value of zero be displayed instead of Unknown?

[mbrunson, 2017/01/12 22:16:28]

0 should be shown here if it's defined. I've changed both of these. Done.

+      };
+
+      this.deviceFieldSet.setObject(deviceViewObj);
+    },
+
+    /**
+     * Sets the page's device info and forces a redraw.
+     * @param {!interfaces.BluetoothDevice.DeviceInfo}
+     */
+    setDeviceInfo: function(info) {
+      this.deviceInfo = info;
+      this.redraw();
+    },
+
+    /** Fires an 'infochanged' event with the current |deviceInfo| */

[dpapad, 2017/01/12 18:37:09]

@private missing here and elsewhere.

[mbrunson, 2017/01/12 22:16:28]

Done.

+    fireDeviceInfoChanged_: function() {
+      this.pageDiv.dispatchEvent(new CustomEvent('infochanged', {
+        bubbles: true,
+        detail: {
+          info: this.deviceInfo,
+        },
+      }));
+    },
+
+    /**
+    * Updates the current connection status. Caches the latest status, updates
+    * the connection button message, and fires a 'connectionchanged' event when
+    * finished.
+    * @param {!device_collection.ConnectionStatus} status
+    * @param {Error=} opt_error

[dpapad, 2017/01/12 18:37:09]

Is this still relevant?

[mbrunson, 2017/01/12 22:16:28]

Removed. Done.

+    */
+    updateConnectionStatus_: function(status) {

[dpapad, 2017/01/12 18:37:09]

Should there be an early return check?

if (this.status === status)
  return;

this.status = status;
...

[mbrunson, 2017/01/12 22:16:28]

Done.

+      this.status_ = status;
+      if (status === device_collection.ConnectionStatus.DISCONNECTED) {
+        this.disconnectBtn_.textContent = 'Connect';
+        this.disconnectBtn_.disabled = false;

[dpapad, 2017/01/12 18:37:09]

The naming of |disconnectBtn| is confusing. If status is DISCONNECTED, the
button is enabled? I would expect a disconnect button to be disabled if I am
already disconnected.

Should it be renamed to connectBtn?

[mbrunson, 2017/01/12 22:16:28]

Renamed. Done.

+      } else if (status === device_collection.ConnectionStatus.CONNECTING) {
+        this.disconnectBtn_.textContent = 'Connecting';
+        this.disconnectBtn_.disabled = true;
+      } else {
+        this.disconnectBtn_.textContent = 'Disconnect';
+        this.disconnectBtn_.disabled = false;
+      }
+
+      this.pageDiv.dispatchEvent(new CustomEvent('connectionchanged', {
+        bubbles: true,
+        detail: {
+          address: this.deviceInfo.address,
+          status: status,
+        }
+      }));
+    },
+  };
+
+  return {
+    DeviceDetailsPage: DeviceDetailsPage,
+  };
+});