bluetooth: Update the map of GATT services when services resolve

device/bluetooth/bluez/bluetooth_device_bluez.cc

Index: device/bluetooth/bluez/bluetooth_device_bluez.cc
diff --git a/device/bluetooth/bluez/bluetooth_device_bluez.cc b/device/bluetooth/bluez/bluetooth_device_bluez.cc
index b2e0d5210076ea7006c5a7b40b25e727f3835691..4a3edebbf07f8ac3d9f86abfa43a87d1056a0948 100644
--- a/device/bluetooth/bluez/bluetooth_device_bluez.cc
+++ b/device/bluetooth/bluez/bluetooth_device_bluez.cc
@@ -156,9 +156,8 @@ BluetoothDeviceBlueZ::BluetoothDeviceBlueZ(
       weak_ptr_factory_(this) {
   bluez::BluezDBusManager::Get()->GetBluetoothGattServiceClient()->AddObserver(
       this);
-  bluez::BluezDBusManager::Get()->GetBluetoothDeviceClient()->AddObserver(this);
 
-  InitializeGattServiceMap();
+  InitializeGattServicesMap();
 }
 
 BluetoothDeviceBlueZ::~BluetoothDeviceBlueZ() {
@@ -166,8 +165,6 @@ BluetoothDeviceBlueZ::~BluetoothDeviceBlueZ() {
       ->GetBluetoothGattServiceClient()
       ->RemoveObserver(this);
 
-  bluez::BluezDBusManager::Get()->GetBluetoothDeviceClient()->RemoveObserver(
-      this);
   // Copy the GATT services list here and clear the original so that when we
   // send GattServiceRemoved(), GetGattServices() returns no services.
   GattServiceMap gatt_services_swapped;
@@ -578,30 +575,6 @@ BluetoothAdapterBlueZ* BluetoothDeviceBlueZ::adapter() const {
   return static_cast<BluetoothAdapterBlueZ*>(adapter_);
 }
 
-void BluetoothDeviceBlueZ::DevicePropertyChanged(
-    const dbus::ObjectPath& object_path,
-    const std::string& property_name) {
-  if (object_path != object_path_)
-    return;
-
-  bluez::BluetoothDeviceClient::Properties* properties =
-      bluez::BluezDBusManager::Get()->GetBluetoothDeviceClient()->GetProperties(
-          object_path);
-  DCHECK(properties);
-
-  if (property_name == properties->services_resolved.name() &&
-      properties->services_resolved.value()) {
-    VLOG(3) << "All services were discovered for device: "
-            << object_path.value();
-
-    for (const auto iter : newly_discovered_gatt_services_) {
-      adapter()->NotifyGattDiscoveryComplete(
-          static_cast<BluetoothRemoteGattService*>(iter));
-    }
-    newly_discovered_gatt_services_.clear();
-  }
-}
-
 void BluetoothDeviceBlueZ::GattServiceAdded(
     const dbus::ObjectPath& object_path) {
   if (GetGattService(object_path.value())) {
@@ -624,9 +597,6 @@ void BluetoothDeviceBlueZ::GattServiceAdded(
   BluetoothRemoteGattServiceBlueZ* service =
       new BluetoothRemoteGattServiceBlueZ(adapter(), this, object_path);
 
-  newly_discovered_gatt_services_.push_back(
-      static_cast<BluetoothRemoteGattServiceBlueZ*>(service));
-
   gatt_services_.set(service->GetIdentifier(),
                      std::unique_ptr<BluetoothRemoteGattService>(service));
   DCHECK(service->object_path() == object_path);
@@ -657,10 +627,41 @@ void BluetoothDeviceBlueZ::GattServiceRemoved(
       gatt_services_.take_and_erase(iter->first);
 
   DCHECK(adapter());
+  discovery_complete_notified_.erase(service);
   adapter()->NotifyGattServiceRemoved(service);
 }
 
-void BluetoothDeviceBlueZ::InitializeGattServiceMap() {
+void BluetoothDeviceBlueZ::UpdateGattServices(
+    const dbus::ObjectPath& object_path) {
+  if (object_path != object_path_) {
+    // No need to update map if update is for a different device.
+    return;
+  }
+
+  DCHECK(IsGattServicesDiscoveryComplete());
+
+  const std::vector<dbus::ObjectPath> gatt_services =
+      bluez::BluezDBusManager::Get()
+          ->GetBluetoothGattServiceClient()
+          ->GetServices();
+  for (const auto& service_object_path : gatt_services) {
+    // Add all previously unknown GATT services associated with the device.
+    BluetoothRemoteGattService* service =
+        GetGattService(service_object_path.value());
+    if (service == nullptr) {
+      GattServiceAdded(service_object_path);
+    }
+
+    // Notify of GATT discovery complete if we haven't before.
+    service = GetGattService(service_object_path.value());
+    auto notified_pair = discovery_complete_notified_.insert(service);
+    if (notified_pair.second) {
+      adapter()->NotifyGattDiscoveryComplete(service);
+    }
+  }
+}
+
+void BluetoothDeviceBlueZ::InitializeGattServicesMap() {
   DCHECK(gatt_services_.empty());
 
   if (!IsGattServicesDiscoveryComplete()) {

[vudentz, 2016/07/01 11:16:08]

InitializeGattServicesMap is only called by the constructor so this probably
always evaluate to false.

[ortuno, 2016/07/01 15:51:48]

What if a device is connected and all it's attributes have been discovered
already? Wouldn't this be true in that case?

@@ -683,8 +684,11 @@ void BluetoothDeviceBlueZ::InitializeGattServiceMap() {
   // Notify on the discovery complete for each service which is found in the
   // first discovery.
   DCHECK(adapter());
-  for (const auto& iter : gatt_services_)
+  for (const auto& iter : gatt_services_) {

[vudentz, 2016/07/01 11:16:08]

What is the purpose of iterating to gatt_services twice? Can't this be done with
the first iteration?

[ortuno, 2016/07/01 15:51:48]

I combined the two for loops.

+    auto notified_pair = discovery_complete_notified_.insert(iter.second);
+    DCHECK(notified_pair.second);
     adapter()->NotifyGattDiscoveryComplete(iter.second);
+  }
 }
 
 void BluetoothDeviceBlueZ::OnGetConnInfo(const ConnectionInfoCallback& callback,