bluetooth: Add device list retrieval for chrome://bluetooth-internals

chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc

+// Copyright 2016 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.
+
+#include <utility>
+#include <vector>
+
+#include "chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h"
+#include "mojo/public/cpp/bindings/string.h"
+
+BluetoothAdapterService::BluetoothAdapterService(
+    bluetooth::AdapterClientPtr client)
+    : client_(std::move(client)), weak_ptr_factory_(this) {
+  if (!GetAdapter()) {
+    device::BluetoothAdapterFactoryWrapper::Get().AcquireAdapter(

[ortuno, 2016/09/22 08:32:53]

Don't use the Wrapper. That class was added because Web Bluetooth does some
weird stuff for testing. Just use BluetoothAdapterFactory. Also I would move the
adapter creation to GetAdapter().

[mbrunson, 2016/09/24 01:05:47]

Done.

+        this, base::Bind(&BluetoothAdapterService::OnAdapterAcquired,
+                         weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+BluetoothAdapterService::~BluetoothAdapterService() {}
+
+void BluetoothAdapterService::OnAdapterAcquired(
+    device::BluetoothAdapter* adapter) {
+  VLOG(1) << "Adapter acquired in BluetoothAdapterService";
+
+  for (const device::BluetoothDevice* device : adapter->GetDevices()) {

[ortuno, 2016/09/22 08:32:54]

q: Why do you do this after adapter construction rather than when clients
request devices?

[mbrunson, 2016/09/24 01:05:47]

Done.

+    addresses_.insert(device->GetAddress());
+  }
+}
+
+void BluetoothAdapterService::GetDevices(int8_t index,
+                                         int8_t count,
+                                         const GetDevicesCallback& callback) {
+  std::vector<bluetooth::DeviceInfoPtr> result;

[ortuno, 2016/09/22 08:32:53]

optional nit: "devices" is more specific.

[mbrunson, 2016/09/24 01:05:47]

Done.

+
+  for (const std::string& address : addresses_) {
+    if (result.size() >= (size_t)(count)) {
+      break;
+    }
+
+    device::BluetoothDevice* device = GetAdapter()->GetDevice(address);
+    bluetooth::DeviceInfoPtr device_info = GetDeviceInfo(device);
+    result.push_back(std::move(device_info));
+  }
+
+  callback.Run(std::move(result));
+}
+
+device::BluetoothAdapter* BluetoothAdapterService::GetAdapter() {
+  return device::BluetoothAdapterFactoryWrapper::Get().GetAdapter(this);
+}
+
+void BluetoothAdapterService::DeviceAdded(device::BluetoothAdapter* adapter,
+                                          device::BluetoothDevice* device) {
+  std::string device_address = device->GetAddress();
+
+  // If known address was added, alert client
+  if (addresses_.insert(device_address).second && client_) {

[ortuno, 2016/09/22 08:32:53]

The comment says that if a *known* address was added alert the client but the
code does the opposite: if a device_address is inserted (therefore unknown) then
notify the client.

Also when do you expect to see known addresses here? You are removing address in
DeviceRemoved so all addresses would be unknown.

[mbrunson, 2016/09/24 01:05:47]

That's a typo.

I don't expect to see known addresses, but I was checking for inconsistent state
just in case.

[ortuno, 2016/09/26 01:57:55]

We should be able to trust the API to not mess up. That said, you could just add
a DCHECK[1] to indicate that you don't expect for that to happen.

bool insertion_result = addresses_.insert(device_address).second;
DCHECK(insertion_result);

[1]
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

+    device::BluetoothDevice* device = GetAdapter()->GetDevice(device_address);
+    auto device_info = GetDeviceInfo(device);
+    client_->DeviceAdded(std::move(device_info));
+  }
+}
+
+void BluetoothAdapterService::DeviceRemoved(device::BluetoothAdapter* adapter,
+                                            device::BluetoothDevice* device) {
+  std::string device_address = device->GetAddress();
+
+  // If known address was removed, alert client
+  if (addresses_.erase(device_address) && client_) {
+    device::BluetoothDevice* device = GetAdapter()->GetDevice(device_address);
+    auto device_info = GetDeviceInfo(device);
+    client_->DeviceRemoved(std::move(device_info));
+  }
+}
+
+bluetooth::DeviceInfoPtr BluetoothAdapterService::GetDeviceInfo(

[ortuno, 2016/09/22 08:32:54]

optional nit: ConstructDeviceInfoStruct would be more specific.

[mbrunson, 2016/09/24 01:05:47]

Done.

+    device::BluetoothDevice* device) {
+  bluetooth::DeviceInfoPtr device_info = bluetooth::DeviceInfo::New();
+  device_info->name = device->GetName().value_or("");

[ortuno, 2016/09/22 08:32:53]

I think you need to surround this by an if statement otherwise you will be
returning an empty string for devices that have no name.

[mbrunson, 2016/09/24 01:05:47]

Removing the value_or should be enough. Name is an optional property here.

+  device_info->name_for_display =
+      base::UTF16ToUTF8(device->GetNameForDisplay());
+  device_info->id = device->GetIdentifier();
+  device_info->address = device->GetAddress();
+  device_info->device_id = device->GetDeviceID();
+  device_info->product_id = device->GetProductID();
+  device_info->vendor_id = device->GetVendorID();
+  return device_info;
+}