bluetooth: Add Device connection logic and accompanying user interface.

chrome/browser/resources/bluetooth_internals/device_table.js

Index: chrome/browser/resources/bluetooth_internals/device_table.js
diff --git a/chrome/browser/resources/bluetooth_internals/device_table.js b/chrome/browser/resources/bluetooth_internals/device_table.js
index b4db9b030f079ea9f7947d784a2b3442efd672e9..c57925f1cc8628e096898007d09081ca8fb9ac18 100644
--- a/chrome/browser/resources/bluetooth_internals/device_table.js
+++ b/chrome/browser/resources/bluetooth_internals/device_table.js
@@ -61,6 +61,53 @@ cr.define('device_table', function() {
     },
 
     /**
+     * Generates a function to handle click events on the connect button for the
+     *     given |row|.
+     * @private
+     * @param {HTMLTableRowElement} row The table row that was clicked.
+     * @return {function(Event)}
+     */
+    handleConnectBtn_: function(row) {

[ortuno, 2016/11/08 04:38:02]

So currently our code is split in two UI (Device Table) and Data (Device
Collection):
 - DeviceTable simply updates the UI whenever it's told to i.e. whenever
DeviceCollection fires events.
 - DeviceTable accesses Data (DeviceCollection) directly to retrieve items and
populate rows.
 
 - DeviceCollection is basically an array that fires events when it changes.
 - DeviceCollection is modified by AdapterBroker and DeviceCollection knows
nothing about DeviceTable.

This change makes it so that:
 - DeviceTable now calls functions on Device and needs to know about the
internals of device beyond just information.
 - Device in DeviceCollection now has functions to connect and disconnect.

These two changes are obscuring the definitions of our two modules (DeviceTable
and DeviceCollection) which will eventually make the code harder to follow. Also
this will make separating the DevicesView and DeviceDetailsView harder. I think
we can do better. More specifically:

1. DeviceTable shouldn't need to handle connection itself. Rather we should fire
an event to indicate that a connection has been requested.
2. We need to decide who's going to receive this event. Should this be
Navigation, BluetoothInternals or maybe just DeviceDetails?
3. Whoever receives this event should:
  1. Tell DeviceCollection that a device is connecting. DeviceCollection would
then fire an event to update the table.
  2. Start a connection.
  3. Once the connection is created we need to tell DeviceCollection that a
device is connected. Which would then update DeviceTable.
  4. Hold to the Device proxy.

Does that make sense? We can have a quick VC if you want to brainstorm some
more.

[mbrunson, 2016/11/08 22:45:13]

Ahh. I see your point and agree with the event-based system.

1. Ok.
2. Probably bluetooth_internals or some messaging subsystem in it. Since it's
the container for all of the views, it is best suited to pass events between the
views. 
I added a publish/subscribe system for custom events in bluetooth_internals.
I've put the subscriber code in there for now until DevicesView is made.
3. Ok.

+      return function(event) {
+        var cellCount = row.cells.length;
+        var connectCell = row.cells[cellCount - 2];
+        var connectButton = connectCell.children[0];
+        var connectErrorCell = row.cells[cellCount - 1];
+
+        connectErrorCell.textContent = '';
+
+        var device = this.devices_.getByAddress(row.id);
+        if (device.proxy) {
+          connectButton.textContent = 'Disconnecting...';
+          connectButton.disabled = true;
+          device.disconnect();
+        } else {
+          connectButton.textContent = 'Connecting...';
+          connectButton.disabled = true;
+
+          this.handleConnect_(device).catch(function(error) {
+            connectErrorCell.textContent = error.message;
+            connectButton.textContent = 'Connect';
+            connectButton.disabled = false;
+          });
+        }
+
+      }.bind(this);
+    },
+
+    /**
+     * Creates a connection to the device and updates the device collection.
+     * @private
+     * @param {device_collection.Device} device
+     * @return {Promise} rejects if connection failed, resolves otherwise.
+     */
+    handleConnect_: function(device) {
+      return device.connect().then(function() {
+        this.devices_.addOrUpdate(device.info);
+      }.bind(this));
+    },
+
+    /**
      * Updates table row on splice event of the device collection.
      * @private
      * @param {!CustomEvent} event
@@ -89,6 +136,17 @@ cr.define('device_table', function() {
         row.insertCell();
       }
 
+      // Make two extra cells for the connect button and connect errors.
+      var connectCell = row.insertCell();
+      var connectErrorCell = row.insertCell();
+
+      var connectButton = document.createElement('button');
+      connectCell.appendChild(connectButton);
+
+      connectButton.textContent = 'Connect';
+      connectButton.addEventListener('click',
+                                     this.handleConnectBtn_(row).bind(this));
+
       this.updateRow_(device, row.sectionRowIndex);
     },
 
@@ -123,6 +181,26 @@ cr.define('device_table', function() {
         row.classList.remove(REMOVED_CSS);
       }
 
+      var cellCount = row.cells.length;
+      var connectCell = row.cells[cellCount - 2];

[ortuno, 2016/11/08 04:38:02]

Is there a better way to retrieve this? Would it make sense to have an enum of
the columns and then use that enum to retrieve these e.g.:

var COLUMNS = {
  NAME: 0,
  ADDRESS: 1,
  ...,
  CONNECTION_STATUS: 4,
  CONNECTION_ERROR: 5
};

var connectCell = row.cells[COLUMNS.CONNECTION_STATUS];

[mbrunson, 2016/11/08 22:45:13]

The connection-related columns are always at the end. The rest of the table is
markup driven, explicitly determined by the header layout in the HTML. An enum
seems like overkill.

[ortuno, 2016/11/09 03:24:03]

Right, but getting a cell by counting from the end is really brittle.

[mbrunson, 2016/11/09 23:39:37]

Done.

+      var connectButton = connectCell.children[0];
+      var connectErrorCell = row.cells[cellCount - 1];
+
+      if (!device.connectionPending) {
+        if (device.info.is_gatt_connected) {

[ortuno, 2016/11/08 04:38:02]

This is a little tricky because there are two different 'Connected' states. One
is the Wireless connection between the device and the computer and the other one
is the connection to the Device proxy.

This is really confusing and something I have to explain every time someone uses
the API, so I don't think we want to expose it in that way to users. What do you
think about just having a 'GATT Connection State' column that shows 'Connected'
if is_gatt_connected is true and 'Not Connected' otherwise. Then we could have a
button that just says 'Acquire Connection', or 'Explore' or 'Inspect', if we
don't have a proxy and 'Release Connection', or 'Forget', or 'Close' if we have
a proxy.

[mbrunson, 2016/11/08 22:45:13]

I'll use inspect/forget for now. Done.

+          connectButton.textContent = 'Disconnect';
+        } else if (device.proxy) {
+          connectButton.textContent = 'Connect';
+          connectErrorCell.textContent = 'Lost connection';
+          device.proxy = null;
+        } else {
+          connectButton.textContent = 'Connect';
+        }
+
+        connectButton.disabled = false;
+      }
+
+
       // Update the properties based on the header field path.
       for (var i = 0; i < this.headers_.length; i++) {
         var header = this.headers_[i];
@@ -136,7 +214,7 @@ cr.define('device_table', function() {
         }
 
         var cell = row.cells[i];
-        cell.textContent = obj || 'Unknown';
+        cell.textContent = (obj == null || obj == undefined) ? 'Unknown' : obj;
         cell.dataset.label = header.textContent;
       }
     },