bluetooth: Add Device connection logic and accompanying user interface.

device/bluetooth/device.cc

Index: device/bluetooth/device.cc
diff --git a/device/bluetooth/device.cc b/device/bluetooth/device.cc
index 7e662802eabb5aed91c9859f782903049c866170..05dbcde232838761beda4ef6e9c45526fd2a9ee3 100644
--- a/device/bluetooth/device.cc
+++ b/device/bluetooth/device.cc
@@ -3,18 +3,35 @@
 // found in the LICENSE file.
 
 #include <utility>
+#include <vector>
 
+#include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "device/bluetooth/device.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 
 namespace bluetooth {
 
-Device::Device(const std::string& address,
-               scoped_refptr<device::BluetoothAdapter> adapter)
-    : address_(address), adapter_(std::move(adapter)) {}
+Device::Device(scoped_refptr<device::BluetoothAdapter> adapter,
+               std::unique_ptr<device::BluetoothGattConnection> connection,
+               mojom::DeviceRequest request)
+    : adapter_(std::move(adapter)),
+      connection_(std::move(connection)),
+      binding_(this, std::move(request)) {
+  binding_.set_connection_error_handler(
+      base::Bind(&Device::Disconnect, base::Unretained(this)));

[ortuno, 2016/11/01 06:27:39]

base::Unretained always raises some alarms. It's like using new. You have to be
sure that the object will not get deleted before the callback is called. In this
case the binding is owned by the class so it's safe but let's add a comment just
in case.

[mbrunson, 2016/11/02 01:25:45]

Hmm. Should I prefer WeakPtrFactory over base::Unretained then? If it could
cause some concern (whether justified or not), sticking to WeakPtrFactory seems
preferable.

[ortuno, 2016/11/02 07:56:04]

We shouldn't add complexity unless it's needed. A comment indicating that you've
thought about the lifetime and concluded that it's not a problem should be
enough.

 
-Device::~Device() {}
+  adapter_->AddObserver(this);
+}
+
+Device::~Device() {
+  binding_.Close();
+
+  if (connection_->IsConnected())
+    connection_->Disconnect();

[ortuno, 2016/11/01 06:27:39]

No need to call Disconnect. Destroying the object will close the connection.

[mbrunson, 2016/11/02 01:25:45]

Removed. Done.

+
+  adapter_->RemoveObserver(this);
+}
 
 // static
 mojom::DeviceInfoPtr Device::ConstructDeviceInfoStruct(
@@ -25,6 +42,7 @@ mojom::DeviceInfoPtr Device::ConstructDeviceInfoStruct(
   device_info->name_for_display =
       base::UTF16ToUTF8(device->GetNameForDisplay());
   device_info->address = device->GetAddress();
+  device_info->is_gatt_connected = device->IsGattConnected();
 
   if (device->GetInquiryRSSI()) {
     device_info->rssi = mojom::RSSIWrapper::New();
@@ -34,8 +52,36 @@ mojom::DeviceInfoPtr Device::ConstructDeviceInfoStruct(
   return device_info;
 }
 
+void Device::set_connection_error_handler(const base::Closure& closure) {
+  binding_.set_connection_error_handler(closure);
+}
+
+void Device::DeviceChanged(device::BluetoothAdapter* adapter,
+                           device::BluetoothDevice* device) {
+  if (device->GetAddress() == GetAddress()) {

[ortuno, 2016/11/01 06:27:39]

nit:

if (device->GetAddress() != GetAddress()) {
  return;
}
// ...

[mbrunson, 2016/11/02 01:25:45]

Done.

+    if (!device->IsGattConnected()) {
+      delete this;
+    }
+  }
+}
+
+void Device::GattServicesDiscovered(device::BluetoothAdapter* adapter,
+                                    device::BluetoothDevice* device) {
+  if (device->GetAddress() == GetAddress()) {

[ortuno, 2016/11/01 06:27:39]

nit:

if (device->GetAddress() != GetAddress()) {
  return;
}

for (...) {
//...

[mbrunson, 2016/11/02 01:25:45]

Done.

+    for (const base::Closure& request : pending_services_requests_) {
+      request.Run();

[ortuno, 2016/11/01 06:27:39]

Running closures in a for loop could lead to unexpected reentrancy issues. We've
had a couple of bugs like that in device/bluetooth. The recommended pattern is
to swap the callbacks first:

std::vector<base::Closure> requests;
requests.swap(pending_services_requests);

for (const base::Closure& request : requests) {
  request.Run();
}

// We should have no requests left.
DCHECK(requests.empty());

[mbrunson, 2016/11/02 01:25:45]

Are you saying running the requests removes them from the requests vector
automatically? That seems strange...

[ortuno, 2016/11/02 07:56:04]

Or adds them. For example we in device/bluetooth we queue calls so
StartNotifySession. We had a bug in which if in the callback for
StartNotifySession someone called StartNotifySession we would queue the callback
and invalidate the iterator that was being used for the for loop.

+    }
+
+    pending_services_requests_.clear();
+  }
+}
+
+void Device::Disconnect() {
+  delete this;
+}
+
 void Device::GetInfo(const GetInfoCallback& callback) {
-  device::BluetoothDevice* device = adapter_->GetDevice(address_);
+  device::BluetoothDevice* device = adapter_->GetDevice(GetAddress());
   if (device) {

[ortuno, 2016/11/01 06:27:39]

Now that lifetime of this class is owned by the connection I don't think you
need these if statements. A DCHECK should be enough.

[mbrunson, 2016/11/02 01:25:46]

Done.

     mojom::DeviceInfoPtr device_info = ConstructDeviceInfoStruct(device);
     callback.Run(std::move(device_info));
@@ -44,4 +90,48 @@ void Device::GetInfo(const GetInfoCallback& callback) {
   }
 }
 
+void Device::GetServices(const GetServicesCallback& callback) {
+  device::BluetoothDevice* device = adapter_->GetDevice(GetAddress());
+
+  if (device) {

[ortuno, 2016/11/01 06:27:39]

Same here. I don't think you need the if anymore.

[mbrunson, 2016/11/02 01:25:45]

Done.

+    if (device->IsGattServicesDiscoveryComplete()) {
+      GetServicesImpl(callback);
+      return;
+    }
+
+    pending_services_requests_.push_back(
+        base::Bind(&Device::GetServicesImpl, base::Unretained(this), callback));

[ortuno, 2016/11/01 06:27:39]

Here it's a bit harder to see why base::Unretained is safe. Please add a
comment.

+    return;
+  }
+
+  callback.Run(std::vector<mojom::ServiceInfoPtr>());
+}
+
+void Device::GetServicesImpl(const GetServicesCallback& callback) {
+  device::BluetoothDevice* device = adapter_->GetDevice(GetAddress());
+  std::vector<mojom::ServiceInfoPtr> services;
+
+  for (const device::BluetoothRemoteGattService* service :
+       device->GetGattServices()) {
+    mojom::ServiceInfoPtr service_info = ConstructServiceInfoStruct(service);
+    services.push_back(std::move(service_info));
+  }
+
+  callback.Run(std::move(services));
+}
+
+mojom::ServiceInfoPtr Device::ConstructServiceInfoStruct(

[ortuno, 2016/11/01 06:27:39]

Since you know service will always be there just take a const &.

[mbrunson, 2016/11/02 01:25:46]

Done.

+    const device::BluetoothRemoteGattService* service) {
+  mojom::ServiceInfoPtr service_info = mojom::ServiceInfo::New();
+
+  service_info->uuid = service->GetUUID();
+  service_info->is_primary = service->IsPrimary();
+
+  return service_info;
+}
+
+std::string Device::GetAddress() {
+  return connection_->GetDeviceAddress();
+}
+
 }  // namespace bluetooth