bluetooth: Add Device service for chrome://bluetooth-internals.

chrome/browser/resources/bluetooth_internals/bluetooth_internals.js

Index: chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
diff --git a/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js b/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
index 961b15d5a7ce860ba34b4c4ea103d7b054ed5725..24799d6f3927783a2104a7b1afe987ee75a4da3d 100644
--- a/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
+++ b/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
@@ -15,28 +15,22 @@ var AdapterClient = function() {};
 AdapterClient.prototype = {
   /**
    * Prints added device to console.
-   * @param {!Object} device the device that was added
+   * @param {!bluetoothDevice.DeviceInfo} device
    */
-  deviceAdded: function(device) {
-    console.log('Device added');
-    console.log(device);
-  },
+  deviceAdded: function(device) { console.log('Device added', device); },
 
   /**
    * Prints removed device to console.
-   * @param {!Object} device the device that was removed
+   * @param {!bluetoothDevice.DeviceInfo} device
    */
-  deviceRemoved: function(device) {
-    console.log('Device removed');
-    console.log(device);
-  }
+  deviceRemoved: function(device) { console.log('Device removed', device); }
 };
 
 (function() {
-  var adapter, adapterClient;
+  var adapter, adapterClient, bluetoothAdapter, bluetoothDevice, connection;
 
   /**
-   * TODO: Move to shared location. See http://crbug.com/652361.
+   * TODO(crbug.com/652361): Move to shared location.
    * Helper to convert callback-based define() API to a promise-based API.
    * @param {!Array<string>} moduleNames
    * @return {!Promise}
@@ -58,10 +52,14 @@ AdapterClient.prototype = {
     return importModules([
       'content/public/renderer/frame_interfaces',
       'device/bluetooth/public/interfaces/adapter.mojom',
+      'device/bluetooth/public/interfaces/device.mojom',
       'mojo/public/js/connection',
-    ]).then(function([frameInterfaces, bluetoothAdapter, connection]) {
+    ]).then(function([frameInterfaces, ...modules]) {
       console.log('Loaded modules');
 
+      // Destructure here to assign global variables.
+      [bluetoothAdapter, bluetoothDevice, connection] = modules;
+
       // Hook up the instance properties.
       AdapterClient.prototype.__proto__ =
           bluetoothAdapter.AdapterClient.stubClass.prototype;
@@ -71,28 +69,57 @@ AdapterClient.prototype = {
           bluetoothAdapter.AdapterFactory);
 
       // Get an Adapter service.
-      return adapterFactory.getAdapter().then(function(response) {
-          if (!response.adapter) {
-            throw new Error('Bluetooth Not Supported on this platform.');
-          }
-
-          adapter = connection.bindHandleToProxy(response.adapter,
-                                                 bluetoothAdapter.Adapter);
-
-          // Create a message pipe and bind one end to client
-          // implementation and the other to the Adapter service.
-          adapterClient = new AdapterClient();
-          adapter.setClient(connection.bindStubDerivedImpl(adapterClient));
-      });
+      return adapterFactory.getAdapter();
+    }).then(function(response) {
+      if (!response.adapter) {
+        throw new Error('Bluetooth Not Supported on this platform.');
+      }
+
+      adapter = connection.bindHandleToProxy(response.adapter,
+                                             bluetoothAdapter.Adapter);
+
+      // Create a message pipe and bind one end to client
+      // implementation and the other to the Adapter service.
+      adapterClient = new AdapterClient();
+      adapter.setClient(connection.bindStubDerivedImpl(adapterClient));
     });
   }
 
+  /**
+   * Prints device info from the device service.
+   * @param {!bluetoothDevice.DeviceInfo} deviceInfo the device for which to
+   *     get the information.
+   * @return {!Promise} resolves if device service is retrieved, rejects
+   *     otherwise.
+   */
+  function logDevice(deviceInfo) {
+    return adapter.getDevice(deviceInfo.address).then(function(response) {
+        var deviceHandle = response.device;
+        if (!deviceHandle) {
+          throw new Error(deviceInfo.name_for_display + ' cannot be found.');
+        }
+
+        var device = connection.bindHandleToProxy(
+              deviceHandle, bluetoothDevice.Device);
+        return device.getInfo();
+      }).then(function(response) {
+        console.log(deviceInfo.name_for_display, response.info);
+      });
+  }
+
   document.addEventListener('DOMContentLoaded', function() {
     initializeProxies()
-      .then(function() {return adapter.getInfo(); })
-      .then(function(response) { console.log('info', response.info); })
+      .then(function() { return adapter.getInfo(); })
+      .then(function(response) { console.log('adapter', response.info); })
       .then(function() { return adapter.getDevices(); })

[dpapad, 2016/10/11 23:51:04]

I am confused about the purpose of these calls here. Specifically:

1) adapter.getDevices() returns an array of DeviceInfo
2) We iterate over that array and call adapter.getDevice(deviceInfo.address).
3) Then we call device.getInfo() to retrieve the same info we already had at
step 1?

Basically I don't understand the usefulness of the new Device mojom interface.
It only has one method, and the information it provides is already available
from the Adapter. Am I missing something?

[mbrunson, 2016/10/12 01:37:42]

No. You're not missing anything. This is the minimal amount of implementation to
get a Device service. 

The main purpose of the Device service is to provide a place to list Bluetooth
GATT services running on the device. Services are not populated until the device
is connected to. The device cannot be connected to until a
BluetoothGattConnection (which will be another mojom interface) is created. This
leads to a whole set of new files and functions that must be implemented to
create the connection, get service info, and manage it appropriately. In order
to keep this patch a reasonable size, I didn't included that stuff. This leaves
the GetInfo function which exists as a convenience for consumers of the Device
service.

I added it to the JS file because I needed to make sure it produced the same
output in the client, and I assumed I needed some consumer of the data to allow
for a security review.

[dpapad, 2016/10/12 17:39:02]

Thanks for the explanation. It still seems odd to me that every device requires
a service. My understanding is that you are trying to mitigate the fact that
Mojo services can only pass plain data objects (see DeviceInfo struct) or
handles back and forth. I am guessing you are trying to achieve something as
close to the following snippet, but taking into account Mojo limitations?

adapter.getDevices().then(function(devices) {
  devices.forEach(fuction(d) {
    // Somehow "magically" returned |devices| are fully fledged classes with
    // their own public API, no need to manually create a service for each
    // device.
    d.getInfo().then(...);
    d.getGattServices().then(...);
  });
});

So given Mojo's limitations, I am not confident that spawning a "Device"
service, for every device is the best path forward. When I think of a "service"
I think of a singleton instance of a class that exposes some functionality, and
is usually instantiated once during startup (similar to the already existing
chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth).

All functionality can be exposed via calls to the top-level (and only) service
"adapter" (or could be renamed to bluetoothService).
bluetoothService.listDeviceInfo() => (array<DeviceInfo)
bluetoothService.getDeviceInfo(string address); => (DeviceInfo)
bluetoothService.getDeviceGattServices(string deviceId) => (array<GattService>)
bluetoothService.connectToDevice(string deviceId) => (boolean)
// add more methods here as needed.
......

I also understand that you've chosen to mirror the C++ class structure in the
exposed Mojo interfaces. Why is this coupling beneficial to the UI page?

Let me take a step back and make a more general comment. As a WebUI OWNER, my
task is not to simply make style comments on JS usage, but to understand and
review the overall design. There is a complete lack of knowledge about
chrome://bluetooth-internals roadmap on my part.

- What is the target group? End users? Bluetooth team developers?
- Are there mocks/specs for the finalized UI?
- Is there an overall design doc I should read?

I feel that I am discovering the design little by little on every review, and
that makes useful reviewing hard. My suggestion is

1) land all Mojo interfaces at once in a single CL, so that there is a
   complete picture of the exposed API and all involved data structures.
2) In follow up CLs start implementing the already existing Mojo interfaces step
   by step.

I think this would make reviewing easier, certainly for me, but perhaps for
other
reviewers too.

[mbrunson, 2016/10/12 18:06:52]

Design doc is a WIP: go/fizzbluetoothinternals. You will find a general overview
and discussion of the design for this Mojo service near the end. This coupling
is beneficial to the UI page because it creates a separation of concerns for
parts of the UI. I'm planning to use Polymer for a web components oriented
design, so I would rather have each component manage one specific layer of the
Bluetooth stack. 

Other concerns mainly focus on adapting developers to the new Mojo interfaces.
We didn't want to create a completely new structure for the transition of
existing users if they wish/prefer to use the Mojo implementation.

The only reason I didn't land all of the Mojo services was because I thought I
needed implementation of the mojom to get a security review. If that's not
necessary I will definitely do that.

-      .then(function(response) { console.log('devices', response.devices); })
+      .then(function(response) {

[dpapad, 2016/10/11 23:51:04]

The error will propagate to the already existing error handler at line 123, no
need for one more call to catch(). 

.then(function(response) {
  ...
  return Promise.all(devices.map(logDevice));
})
.catch(function(error) { console.error(error); });

[mbrunson, 2016/10/12 01:37:42]

Done.

+        var devices = response.devices;
+        console.log('devices', devices.length);
+
+        Promise.all(devices.map(logDevice)).catch(function(error) {
+          console.error(error);
+        });
+      })
       .catch(function(error) { console.error(error); });
   });
 })();