bluetoothLowEnergy: Send onServiceAdded after all characteristics are discovered

This CL makes the following changes:

  - Add support for the new "Characteristics" and "Descriptors" properties
    exposed under org.bluez.GattService1 and org.bluez.GattCharacteristic1,
    respectively.

  - Add device::BluetoothGattService::Observer::GattDiscoveryCompleteForService
    event, which gets called after the first PropertiesChanged signal is
    received for the "Characteristics" property of a service.

  - Change BluetoothLowEnergyEventRouter, so that it sends the "onServiceAdded"
    event not in GattServiceAdded but in GattDiscoveryCompleteForService.

BUG=394537
TEST=browser_tests:BluetoothLowEnergyApiTest,
device_unittests:BluetoothGattChromeOSTest

Review URL: https://codereview.chromium.org/402303002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284955 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 77 insertions, 49 deletions

diff --git a/device/bluetooth/bluetooth_device_chromeos.cc b/device/bluetooth/bluetooth_device_chromeos.cc
index de46e92..f607247 100644
--- a/device/bluetooth/bluetooth_device_chromeos.cc
+++ b/device/bluetooth/bluetooth_device_chromeos.cc
@@ -518,7 +518,7 @@ void BluetoothDeviceChromeOS::GattServiceRemoved(
     const dbus::ObjectPath& object_path) {
   GattServiceMap::iterator iter = gatt_services_.find(object_path.value());
   if (iter == gatt_services_.end()) {
-    VLOG(2) << "Unknown GATT service removed: " << object_path.value();
+    VLOG(3) << "Unknown GATT service removed: " << object_path.value();
     return;
   }
 
diff --git a/device/bluetooth/bluetooth_gatt_chromeos_unittest.cc b/device/bluetooth/bluetooth_gatt_chromeos_unittest.cc
index 673ed5f..a585f81 100644
--- a/device/bluetooth/bluetooth_gatt_chromeos_unittest.cc
+++ b/device/bluetooth/bluetooth_gatt_chromeos_unittest.cc
@@ -134,6 +134,7 @@ class TestGattServiceObserver : public BluetoothGattService::Observer {
                           BluetoothDevice* device,
                           BluetoothGattService* service)
       : gatt_service_changed_count_(0),
+        gatt_discovery_complete_count_(0),
         gatt_characteristic_added_count_(0),
         gatt_characteristic_removed_count_(0),
         gatt_characteristic_value_changed_count_(0),
@@ -160,6 +161,14 @@ class TestGattServiceObserver : public BluetoothGattService::Observer {
   }
 
   // BluetoothGattService::Observer overrides.
+  virtual void GattDiscoveryCompleteForService(
+      BluetoothGattService* service) OVERRIDE {
+    ASSERT_EQ(gatt_service_id_, service->GetIdentifier());
+    ++gatt_discovery_complete_count_;
+
+    QuitMessageLoop();
+  }
+
   virtual void GattServiceChanged(BluetoothGattService* service) OVERRIDE {
     ASSERT_EQ(gatt_service_id_, service->GetIdentifier());
     ++gatt_service_changed_count_;
@@ -268,6 +277,7 @@ class TestGattServiceObserver : public BluetoothGattService::Observer {
   }
 
   int gatt_service_changed_count_;
+  int gatt_discovery_complete_count_;
   int gatt_characteristic_added_count_;
   int gatt_characteristic_removed_count_;
   int gatt_characteristic_value_changed_count_;
@@ -608,6 +618,7 @@ TEST_F(BluetoothGattChromeOSTest, GattCharacteristicAddedAndRemoved) {
 
   TestGattServiceObserver service_observer(adapter_, device, service);
   EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_discovery_complete_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_added_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_value_changed_count_);
@@ -619,7 +630,8 @@ TEST_F(BluetoothGattChromeOSTest, GattCharacteristicAddedAndRemoved) {
   // 3 characteristics should appear. Only 1 of the characteristics sends
   // value changed signals. Service changed should be fired once for
   // descriptor added.
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(1, service_observer.gatt_discovery_complete_count_);
   EXPECT_EQ(3, service_observer.gatt_characteristic_added_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_value_changed_count_);
@@ -627,16 +639,20 @@ TEST_F(BluetoothGattChromeOSTest, GattCharacteristicAddedAndRemoved) {
 
   // Hide the characteristics. 3 removed signals should be received.
   fake_bluetooth_gatt_characteristic_client_->HideHeartRateCharacteristics();
-  EXPECT_EQ(8, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(3, service_observer.gatt_characteristic_added_count_);
   EXPECT_EQ(3, service_observer.gatt_characteristic_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_value_changed_count_);
   EXPECT_TRUE(service->GetCharacteristics().empty());
 
-  // Re-expose the heart rate characteristics.
+  // Re-expose the heart rate characteristics. We shouldn't get another
+  // GattDiscoveryCompleteForService call, since the service thinks that
+  // discovery is done. On the bluetoothd side, characteristics will be removed
+  // only if the service will also be subsequently removed.
   fake_bluetooth_gatt_characteristic_client_->ExposeHeartRateCharacteristics(
       fake_bluetooth_gatt_service_client_->GetHeartRateServicePath());
-  EXPECT_EQ(12, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(1, service_observer.gatt_discovery_complete_count_);
   EXPECT_EQ(6, service_observer.gatt_characteristic_added_count_);
   EXPECT_EQ(3, service_observer.gatt_characteristic_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_value_changed_count_);
@@ -644,7 +660,7 @@ TEST_F(BluetoothGattChromeOSTest, GattCharacteristicAddedAndRemoved) {
 
   // Hide the service. All characteristics should disappear.
   fake_bluetooth_gatt_service_client_->HideHeartRateService();
-  EXPECT_EQ(16, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(6, service_observer.gatt_characteristic_added_count_);
   EXPECT_EQ(6, service_observer.gatt_characteristic_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_characteristic_value_changed_count_);
@@ -679,7 +695,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorAddedAndRemoved) {
 
   // Run the message loop so that the characteristics appear.
   base::MessageLoop::current()->Run();
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
 
   // Only the Heart Rate Measurement characteristic has a descriptor.
   EXPECT_EQ(1, service_observer.gatt_descriptor_added_count_);
@@ -717,7 +733,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorAddedAndRemoved) {
   fake_bluetooth_gatt_descriptor_client_->HideDescriptor(
       dbus::ObjectPath(descriptor->GetIdentifier()));
   EXPECT_TRUE(characteristic->GetDescriptors().empty());
-  EXPECT_EQ(5, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(1, service_observer.gatt_descriptor_added_count_);
   EXPECT_EQ(1, service_observer.gatt_descriptor_removed_count_);
   EXPECT_EQ(0, service_observer.gatt_descriptor_value_changed_count_);
@@ -729,7 +745,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorAddedAndRemoved) {
       dbus::ObjectPath(characteristic->GetIdentifier()),
       FakeBluetoothGattDescriptorClient::
           kClientCharacteristicConfigurationUUID);
-  EXPECT_EQ(6, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(1U, characteristic->GetDescriptors().size());
   EXPECT_EQ(2, service_observer.gatt_descriptor_added_count_);
   EXPECT_EQ(1, service_observer.gatt_descriptor_removed_count_);
@@ -1001,12 +1017,14 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorValue) {
 
   TestGattServiceObserver service_observer(adapter_, device, service);
   EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_discovery_complete_count_);
   EXPECT_EQ(0, service_observer.gatt_descriptor_value_changed_count_);
   EXPECT_TRUE(service->GetCharacteristics().empty());
 
   // Run the message loop so that the characteristics appear.
   base::MessageLoop::current()->Run();
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(1, service_observer.gatt_discovery_complete_count_);
 
   // Only the Heart Rate Measurement characteristic has a descriptor.
   BluetoothGattCharacteristic* characteristic = service->GetCharacteristic(
@@ -1043,7 +1061,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorValue) {
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_TRUE(ValuesEqual(last_read_value_, descriptor->GetValue()));
   EXPECT_TRUE(ValuesEqual(desc_value, descriptor->GetValue()));
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(1, service_observer.gatt_descriptor_value_changed_count_);
 
   // Write value. Writes to this descriptor will fail.
@@ -1058,7 +1076,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorValue) {
   EXPECT_EQ(1, error_callback_count_);
   EXPECT_TRUE(ValuesEqual(last_read_value_, descriptor->GetValue()));
   EXPECT_FALSE(ValuesEqual(desc_value, descriptor->GetValue()));
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(1, service_observer.gatt_descriptor_value_changed_count_);
 
   // Read new value.
@@ -1071,7 +1089,7 @@ TEST_F(BluetoothGattChromeOSTest, GattDescriptorValue) {
   EXPECT_EQ(1, error_callback_count_);
   EXPECT_TRUE(ValuesEqual(last_read_value_, descriptor->GetValue()));
   EXPECT_FALSE(ValuesEqual(desc_value, descriptor->GetValue()));
-  EXPECT_EQ(4, service_observer.gatt_service_changed_count_);
+  EXPECT_EQ(0, service_observer.gatt_service_changed_count_);
   EXPECT_EQ(2, service_observer.gatt_descriptor_value_changed_count_);
 }
 
diff --git a/device/bluetooth/bluetooth_gatt_service.h b/device/bluetooth/bluetooth_gatt_service.h
index 16f4ae5..583c136 100644
--- a/device/bluetooth/bluetooth_gatt_service.h
+++ b/device/bluetooth/bluetooth_gatt_service.h
@@ -131,16 +131,18 @@ class BluetoothGattService {
   // as well as when successive changes occur during its life cycle.
   class Observer {
    public:
+    // Called when all characteristic and descriptor discovery procedures are
+    // known to be completed for the GATT service |service|. This method will be
+    // called after the initial discovery of a GATT service and will usually be
+    // preceded by calls to GattCharacteristicAdded and GattDescriptorAdded.
+    virtual void GattDiscoveryCompleteForService(
+        BluetoothGattService* service) {}
+
     // Called when properties of the remote GATT service |service| have changed.
     // This will get called for properties such as UUID, as well as for changes
     // to the list of known characteristics and included services. Observers
     // should read all GATT characteristic and descriptors objects and do any
-    // necessary set up required for a changed service. This method may be
-    // called several times, especially when the service is discovered for the
-    // first time. It will be called for each characteristic and characteristic
-    // descriptor that is discovered or removed. Hence this method should be
-    // used to check whether or not all characteristics of a service have been
-    // discovered that correspond to the profile implemented by the Observer.
+    // necessary set up required for a changed service.
     virtual void GattServiceChanged(BluetoothGattService* service) {}
 
     // Called when the remote GATT characteristic |characteristic| belonging to
@@ -153,22 +155,12 @@ class BluetoothGattService {
     // depends on the particular profile the remote device implements, hence the
     // client of a GATT based profile will usually operate on the whole set of
     // characteristics and not just one.
-    //
-    // This method will always be followed by a call to GattServiceChanged,
-    // which can be used by observers to get all the characteristics of a
-    // service and perform the necessary updates. GattCharacteristicAdded exists
-    // mostly for convenience.
     virtual void GattCharacteristicAdded(
         BluetoothGattService* service,
         BluetoothGattCharacteristic* characteristic) {}
 
     // Called when a GATT characteristic |characteristic| belonging to GATT
-    // service |service| has been removed. This method is for convenience
-    // and will be followed by a call to GattServiceChanged (except when called
-    // after the service gets removed) which should be used for bootstrapping a
-    // GATT based profile. See the documentation of GattCharacteristicAdded and
-    // GattServiceChanged for more information. Try to obtain the service from
-    // its device to see whether or not the service has been removed.
+    // service |service| has been removed.
     virtual void GattCharacteristicRemoved(
         BluetoothGattService* service,
         BluetoothGattCharacteristic* characteristic) {}
@@ -178,19 +170,12 @@ class BluetoothGattService {
     // cache the arguments as the pointers may become invalid. Instead, use the
     // specially assigned identifier to obtain a descriptor and cache that
     // identifier as necessary.
-    //
-    // This method will always be followed by a call to GattServiceChanged,
-    // which can be used by observers to get all the characteristics of a
-    // service and perform the necessary updates. GattDescriptorAdded exists
-    // mostly for convenience.
     virtual void GattDescriptorAdded(
         BluetoothGattCharacteristic* characteristic,
         BluetoothGattDescriptor* descriptor) {}
 
     // Called when a GATT characteristic descriptor |descriptor| belonging to
-    // characteristic |characteristic| has been removed. This method is for
-    // convenience and will be followed by a call to GattServiceChanged (except
-    // when called after the service gets removed).
+    // characteristic |characteristic| has been removed.
     virtual void GattDescriptorRemoved(
         BluetoothGattCharacteristic* characteristic,
         BluetoothGattDescriptor* descriptor) {}
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_chromeos.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_chromeos.cc
index 83231b9..ad15a06 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_chromeos.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_chromeos.cc
@@ -341,7 +341,7 @@ void BluetoothRemoteGattCharacteristicChromeOS::GattDescriptorAdded(
           GetProperties(object_path);
   DCHECK(properties);
   if (properties->characteristic.value() != object_path_) {
-    VLOG(2) << "Remote GATT descriptor does not belong to this characteristic.";
+    VLOG(3) << "Remote GATT descriptor does not belong to this characteristic.";
     return;
   }
 
@@ -356,7 +356,6 @@ void BluetoothRemoteGattCharacteristicChromeOS::GattDescriptorAdded(
   DCHECK(service_);
 
   service_->NotifyDescriptorAddedOrRemoved(this, descriptor, true /* added */);
-  service_->NotifyServiceChanged();
 }
 
 void BluetoothRemoteGattCharacteristicChromeOS::GattDescriptorRemoved(
@@ -374,12 +373,10 @@ void BluetoothRemoteGattCharacteristicChromeOS::GattDescriptorRemoved(
   DCHECK(descriptor->object_path() == object_path);
   descriptors_.erase(iter);
 
-  service_->NotifyDescriptorAddedOrRemoved(this, descriptor, false /* added */);
-  delete descriptor;
-
   DCHECK(service_);
+  service_->NotifyDescriptorAddedOrRemoved(this, descriptor, false /* added */);
 
-  service_->NotifyServiceChanged();
+  delete descriptor;
 }
 
 void BluetoothRemoteGattCharacteristicChromeOS::OnValueSuccess(
diff --git a/device/bluetooth/bluetooth_remote_gatt_service_chromeos.cc b/device/bluetooth/bluetooth_remote_gatt_service_chromeos.cc
index 7038270..f7e99b6 100644
--- a/device/bluetooth/bluetooth_remote_gatt_service_chromeos.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_service_chromeos.cc
@@ -22,6 +22,7 @@ BluetoothRemoteGattServiceChromeOS::BluetoothRemoteGattServiceChromeOS(
     : object_path_(object_path),
       adapter_(adapter),
       device_(device),
+      discovery_complete_(false),
       weak_ptr_factory_(this) {
   VLOG(1) << "Creating remote GATT service with identifier: "
           << object_path.value() << ", UUID: " << GetUUID().canonical_value();
@@ -157,6 +158,12 @@ BluetoothRemoteGattServiceChromeOS::GetAdapter() const {
 }
 
 void BluetoothRemoteGattServiceChromeOS::NotifyServiceChanged() {
+  // Don't send service changed unless we know that all characteristics have
+  // already been discovered. This is to prevent spammy events before sending
+  // out the first Gatt
+  if (!discovery_complete_)
+    return;
+
   FOR_EACH_OBSERVER(device::BluetoothGattService::Observer, observers_,
                     GattServiceChanged(this));
 }
@@ -203,8 +210,27 @@ void BluetoothRemoteGattServiceChromeOS::GattServicePropertyChanged(
   if (object_path != object_path_)
     return;
 
-  VLOG(1) << "Service property changed: " << object_path.value();
-  NotifyServiceChanged();
+  VLOG(1) << "Service property changed: \"" << property_name << "\", "
+          << object_path.value();
+  BluetoothGattServiceClient::Properties* properties =
+      DBusThreadManager::Get()->GetBluetoothGattServiceClient()->GetProperties(
+          object_path);
+  DCHECK(properties);
+
+  if (property_name != properties->characteristics.name()) {
+    NotifyServiceChanged();
+    return;
+  }
+
+  if (discovery_complete_)
+    return;
+
+  VLOG(1) << "All characteristics were discovered for service: "
+          << object_path.value();
+  discovery_complete_ = true;
+  FOR_EACH_OBSERVER(device::BluetoothGattService::Observer,
+                    observers_,
+                    GattDiscoveryCompleteForService(this));
 }
 
 void BluetoothRemoteGattServiceChromeOS::GattCharacteristicAdded(
@@ -235,7 +261,6 @@ void BluetoothRemoteGattServiceChromeOS::GattCharacteristicAdded(
 
   FOR_EACH_OBSERVER(device::BluetoothGattService::Observer, observers_,
                     GattCharacteristicAdded(this, characteristic));
-  NotifyServiceChanged();
 }
 
 void BluetoothRemoteGattServiceChromeOS::GattCharacteristicRemoved(
@@ -255,7 +280,6 @@ void BluetoothRemoteGattServiceChromeOS::GattCharacteristicRemoved(
 
   FOR_EACH_OBSERVER(device::BluetoothGattService::Observer, observers_,
                     GattCharacteristicRemoved(this, characteristic));
-  NotifyServiceChanged();
 
   delete characteristic;
 }
@@ -264,7 +288,7 @@ void BluetoothRemoteGattServiceChromeOS::GattCharacteristicPropertyChanged(
     const dbus::ObjectPath& object_path,
     const std::string& property_name) {
   if (characteristics_.find(object_path) == characteristics_.end()) {
-    VLOG(2) << "Properties of unknown characteristic changed";
+    VLOG(3) << "Properties of unknown characteristic changed";
     return;
   }
 
diff --git a/device/bluetooth/bluetooth_remote_gatt_service_chromeos.h b/device/bluetooth/bluetooth_remote_gatt_service_chromeos.h
index ce481ec..600233d 100644
--- a/device/bluetooth/bluetooth_remote_gatt_service_chromeos.h
+++ b/device/bluetooth/bluetooth_remote_gatt_service_chromeos.h
@@ -146,6 +146,10 @@ class BluetoothRemoteGattServiceChromeOS
       CharacteristicMap;
   CharacteristicMap characteristics_;
 
+  // Indicates whether or not the characteristics of this service are known to
+  // have been discovered.
+  bool discovery_complete_;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<BluetoothRemoteGattServiceChromeOS> weak_ptr_factory_;