[Bluetooth] Alert observers of advertisement on DeviceAdded event.

Before crrev.com/c/1899420, bluez informed Chrome each time an
advertisement was received, even if nothing had changed about the
advertisement from its previous instance. Once that CL landed,
bluez switched to only informing Chrome of an advertisement when
it was first found during a scan, or when metadata of the
advertisement changed (i.e., RSSI, which can remain unchanged
for several seconds). This CL accounts for that new behavior
by informing observers an advertisement has been received on
the DeviceAdded() event.

I ran an Instant Tethering scan 50 times without this patch, and
50 times with it, and recorded time taken to find an expected BLE
advertisement (found in chrome://histograms):

* Without patch:
  * Mean:             4436 ms.
  * Median:           1680 ms.
  * 75th percentile: 11135 ms.
  * 95th percentile: 12185 ms.

* With patch:
  * Mean:              636 ms.
  * Median:            522 ms.
  * 75th percentile:   895 ms.
  * 95th percentile:  1838 ms.

Fixed: b:149945805,1051769
Change-Id: I73dc1d5678d166a6ce443ddfda76c34212aa92aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071377Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746060}

chrome/browser/chromeos/arc/bluetooth/arc_bluetooth_bridge_unittest.cc

@@ -270,7 +270,7 @@ TEST_F(ArcBluetoothBridgeTest, DeviceFound) {
 TEST_F(ArcBluetoothBridgeTest, LEDeviceFound) {
   EXPECT_EQ(0u, fake_bluetooth_instance_->le_device_found_data().size());
   AddTestDevice();
-  EXPECT_EQ(1u, fake_bluetooth_instance_->le_device_found_data().size());
+  EXPECT_EQ(3u, fake_bluetooth_instance_->le_device_found_data().size());
 
   const auto& le_device_found_data =
       fake_bluetooth_instance_->le_device_found_data().back();
@@ -283,7 +283,7 @@ TEST_F(ArcBluetoothBridgeTest, LEDeviceFound) {
   EXPECT_EQ(kTestRssi, le_device_found_data->rssi());
 
   ChangeTestDeviceRssi(kTestRssi2);
-  EXPECT_EQ(2u, fake_bluetooth_instance_->le_device_found_data().size());
+  EXPECT_EQ(4u, fake_bluetooth_instance_->le_device_found_data().size());
   EXPECT_EQ(kTestRssi2,
             fake_bluetooth_instance_->le_device_found_data().back()->rssi());
 }
@@ -293,7 +293,7 @@ TEST_F(ArcBluetoothBridgeTest, LEDeviceFoundForN) {
       "CHROMEOS_ARC_ANDROID_SDK_VERSION=27", base::Time::Now());
   EXPECT_EQ(0u, fake_bluetooth_instance_->le_device_found_data().size());
   AddTestDevice();
-  EXPECT_EQ(1u, fake_bluetooth_instance_->le_device_found_data().size());
+  EXPECT_EQ(3u, fake_bluetooth_instance_->le_device_found_data().size());
 
   const auto& le_device_found_data =
       fake_bluetooth_instance_->le_device_found_data().back();
@@ -317,7 +317,7 @@ TEST_F(ArcBluetoothBridgeTest, LEDeviceFoundForN) {
   EXPECT_EQ(kTestRssi, le_device_found_data->rssi());
 
   ChangeTestDeviceRssi(kTestRssi2);
-  EXPECT_EQ(2u, fake_bluetooth_instance_->le_device_found_data().size());
+  EXPECT_EQ(4u, fake_bluetooth_instance_->le_device_found_data().size());
   EXPECT_EQ(kTestRssi2,
             fake_bluetooth_instance_->le_device_found_data().back()->rssi());
 }

device/bluetooth/bluez/bluetooth_adapter_bluez.cc

@@ -648,6 +648,11 @@ void BluetoothAdapterBlueZ::DeviceAdded(const dbus::ObjectPath& object_path) {
 
   devices_[device_bluez->GetAddress()] = base::WrapUnique(device_bluez);
 
+  if (properties->rssi.is_valid() && properties->eir.is_valid()) {
+    NotifyDeviceAdvertisementReceived(device_bluez, properties->rssi.value(),
+                                      properties->eir.value());
+  }
+
   for (auto& observer : observers_)
     observer.DeviceAdded(this, device_bluez);
 }
@@ -722,12 +727,15 @@ void BluetoothAdapterBlueZ::DevicePropertyChanged(
   if (property_name == properties->mtu.name())
     NotifyDeviceMTUChanged(device_bluez, properties->mtu.value());
 
-  // We use the RSSI as a proxy for receiving an advertisement because it's
-  // usually updated whenever an advertisement is received.
-  if (property_name == properties->rssi.name() && properties->rssi.is_valid() &&
-      properties->eir.is_valid())
+  // Bluez does not currently provide an explicit signal for an advertisement
+  // packet being received. Currently, it implicitly does so by notifying of an
+  // RSSI change. We also listen for whether the EIR packet data has changed.
+  if ((property_name == properties->rssi.name() ||
+       property_name == properties->eir.name()) &&
+      properties->rssi.is_valid() && properties->eir.is_valid()) {
     NotifyDeviceAdvertisementReceived(device_bluez, properties->rssi.value(),
                                       properties->eir.value());
+  }
 
   if (property_name == properties->connected.name())
     NotifyDeviceConnectedStateChanged(device_bluez,

device/bluetooth/bluez/bluetooth_bluez_unittest.cc

@@ -2112,7 +2112,32 @@ TEST_F(BluetoothBlueZTest, DeviceMTUChanged) {
   EXPECT_EQ(128, observer.last_mtu_value());
 }
 
-TEST_F(BluetoothBlueZTest, DeviceAdvertisementReceived) {
+TEST_F(BluetoothBlueZTest, DeviceAdvertisementReceived_LowEnergyDeviceAdded) {
+  // Simulate reception of advertisement from a low energy device.
+  GetAdapter();
+
+  // Install an observer; expect DeviceAdvertisementReceived method to be called
+  // with the EIR and RSSI.
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  ASSERT_EQ(0, observer.device_advertisement_received_count());
+
+  // A low energy device has a valid RSSI and EIR and should trigger the
+  // advertisement callback.
+  fake_bluetooth_device_client_->CreateDevice(
+      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  EXPECT_EQ(1, observer.device_advertisement_received_count());
+
+  // A classic device does not have a valid RSSI and EIR and should not trigger
+  // the advertisement callback.
+  fake_bluetooth_device_client_->CreateDevice(
+      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kJustWorksPath));
+  EXPECT_EQ(1, observer.device_advertisement_received_count());
+}
+
+TEST_F(BluetoothBlueZTest, DeviceAdvertisementReceived_PropertyChanged) {
   // Simulate reception of advertisement from a device.
   GetAdapter();
 
@@ -2132,22 +2157,12 @@ TEST_F(BluetoothBlueZTest, DeviceAdvertisementReceived) {
           dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   ASSERT_EQ(0, observer.device_advertisement_received_count());
 
-  std::vector<uint8_t> eir = {0x01, 0x02, 0x03};
-  properties->eir.ReplaceValue(eir);
   properties->rssi.ReplaceValue(-73);
-
-  properties->rssi.set_valid(true);
-  properties->NotifyPropertyChanged(properties->rssi.name());
-  EXPECT_EQ(0, observer.device_advertisement_received_count());  // EIR invalid
-
-  properties->eir.set_valid(true);
-  properties->rssi.set_valid(false);
-  properties->NotifyPropertyChanged(properties->rssi.name());
-  EXPECT_EQ(0, observer.device_advertisement_received_count());  // RSSI invalid
-
-  properties->rssi.set_valid(true);
-  properties->NotifyPropertyChanged(properties->rssi.name());
   EXPECT_EQ(1, observer.device_advertisement_received_count());
+
+  std::vector<uint8_t> eir = {0x01, 0x02, 0x03};
+  properties->eir.ReplaceValue(eir);
+  EXPECT_EQ(2, observer.device_advertisement_received_count());
   EXPECT_EQ(eir, observer.device_eir());
 }
 

device/bluetooth/dbus/fake_bluetooth_device_client.cc

@@ -813,6 +813,11 @@ void FakeBluetoothDeviceClient::CreateDevice(
     properties->type.ReplaceValue(BluetoothDeviceClient::kTypeLe);
     properties->uuids.ReplaceValue(std::vector<std::string>(
         {FakeBluetoothGattServiceClient::kHeartRateServiceUUID}));
+    std::vector<uint8_t> eir = {0x0a, 0x0b, 0x0c};
+    properties->eir.ReplaceValue(eir);
+    properties->eir.set_valid(true);
+    properties->rssi.ReplaceValue(-45);
+    properties->rssi.set_valid(true);
   } else if (device_path == dbus::ObjectPath(kDualPath)) {
     properties->address.ReplaceValue(kDualAddress);
     properties->name.ReplaceValue(kDualName);