[Bluetooth][WinRT] Track device name changes

This change adds support for tracking device name changes in the WinRT
Bluetooth Low Energy backend. This is done in two ways:

* When advertisement packets are received for a known device the "local
  name" is updated if previous advertisements did not provide one.

* BluetoothDeviceWinrt subscribes to the NameChanged event on the
  IBluetoothLEDevice. This tracks name changes once a connection is
  established.

Bug: 902241
Change-Id: I489fc1b7fb86da3c28fde1ec4459d0c736f27326
Reviewed-on: https://chromium-review.googlesource.com/c/1328222Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608686}

device/bluetooth/bluetooth_adapter.h

@@ -89,6 +89,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapter
     // |device| changes:
     //  * GetAddress()
     //  * GetAppearance()
+    //  * GetName() (Chrome OS and Windows only)
     //  * GetBluetoothClass()
     //  * GetInquiryRSSI()
     //  * GetInquiryTxPower()

device/bluetooth/bluetooth_adapter_winrt.cc

@@ -421,9 +421,8 @@ ComPtr<IBluetoothLEAdvertisement> GetAdvertisement(
   return advertisement;
 }
 
-base::Optional<std::string> GetDeviceName(
-    IBluetoothLEAdvertisementReceivedEventArgs* received) {
-  ComPtr<IBluetoothLEAdvertisement> advertisement = GetAdvertisement(received);
+base::Optional<std::string> ExtractDeviceName(
+    IBluetoothLEAdvertisement* advertisement) {
   if (!advertisement)
     return base::nullopt;
 
@@ -435,6 +434,10 @@ base::Optional<std::string> GetDeviceName(
     return base::nullopt;
   }
 
+  // Return early otherwise ScopedHString will create an empty string.
+  if (!local_name)
+    return base::nullopt;
+
   return base::win::ScopedHString(local_name).GetAsUTF8();
 }
 
@@ -449,6 +452,8 @@ void ExtractAndUpdateAdvertisementData(
   }
 
   ComPtr<IBluetoothLEAdvertisement> advertisement = GetAdvertisement(received);
+  static_cast<BluetoothDeviceWinrt*>(device)->UpdateLocalName(
+      ExtractDeviceName(advertisement.Get()));
   device->UpdateAdvertisementData(rssi, ExtractFlags(advertisement.Get()),
                                   ExtractAdvertisedUUIDs(advertisement.Get()),
                                   ExtractTxPower(advertisement.Get()),
@@ -871,10 +876,8 @@ BluetoothAdapterWinrt::CreateAdvertisement() const {
 }
 
 std::unique_ptr<BluetoothDeviceWinrt> BluetoothAdapterWinrt::CreateDevice(
-    uint64_t raw_address,
-    base::Optional<std::string> local_name) {
-  return std::make_unique<BluetoothDeviceWinrt>(this, raw_address,
-                                                std::move(local_name));
+    uint64_t raw_address) {
+  return std::make_unique<BluetoothDeviceWinrt>(this, raw_address);
 }
 
 void BluetoothAdapterWinrt::OnGetDefaultAdapter(
@@ -1144,8 +1147,7 @@ void BluetoothAdapterWinrt::OnAdvertisementReceived(
   if (is_new_device) {
     bool was_inserted = false;
     std::tie(it, was_inserted) = devices_.emplace(
-        std::move(bluetooth_address),
-        CreateDevice(raw_bluetooth_address, GetDeviceName(received)));
+        std::move(bluetooth_address), CreateDevice(raw_bluetooth_address));
     DCHECK(was_inserted);
   }
 

device/bluetooth/bluetooth_adapter_winrt.h

@@ -117,8 +117,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWinrt : public BluetoothAdapter {
       const;
 
   virtual std::unique_ptr<BluetoothDeviceWinrt> CreateDevice(
-      uint64_t raw_address,
-      base::Optional<std::string> local_name);
+      uint64_t raw_address);
 
  private:
   void OnGetDefaultAdapter(

device/bluetooth/bluetooth_device_unittest.cc

@@ -293,6 +293,30 @@ TEST_F(BluetoothTest, LowEnergyDeviceProperties) {
       base::ContainsKey(uuids, BluetoothUUID(kTestUUIDGenericAttribute)));
 }
 
+// Verifies that the device name can be populated by later advertisement
+// packets and is persistent.
+#if defined(OS_WIN)
+TEST_P(BluetoothTestWinrt, LowEnergyDeviceNameDelayed) {
+#else
+// This test does not yet pass on any other platform.
+TEST_F(BluetoothTest, DISABLED_LowEnergyDeviceNameDelayed) {
+#endif
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  InitWithFakeAdapter();
+  StartLowEnergyDiscoverySession();
+  BluetoothDevice* device = SimulateLowEnergyDevice(3);
+  ASSERT_TRUE(device);
+  // GetName() returns a base::Optional<std:string> however some backends still
+  // return an empty string rather than nullopt when no name is available.
+  EXPECT_TRUE(!device->GetName().has_value() || device->GetName()->empty());
+
+  SimulateLowEnergyDevice(1);
+  EXPECT_EQ(base::UTF8ToUTF16(kTestDeviceName), device->GetNameForDisplay());
+}
+
 // Device with no advertised Service UUIDs.
 #if defined(OS_WIN)
 TEST_P(BluetoothTestWinrt, LowEnergyDeviceNoUUIDs) {
@@ -2130,4 +2154,34 @@ TEST_F(BluetoothTest, MAYBE_GetPrimaryServicesByUUID) {
   }
 }
 
+#if defined(OS_WIN)
+TEST_P(BluetoothTestWinrtOnly, GattConnectedNameChange) {
+#else
+// The SimulateGattNameChange() function is not yet available on other
+// platforms.
+TEST_F(BluetoothTest, DISABLED_GattConnectedNameChange) {
+#endif
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  InitWithFakeAdapter();
+
+  StartLowEnergyDiscoverySession();
+  BluetoothDevice* device = SimulateLowEnergyDevice(3);
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattConnection(device);
+  base::RunLoop().RunUntilIdle();
+  // GetName() returns a base::Optional<std:string> however some backends still
+  // return an empty string rather than nullopt when no name is available.
+  EXPECT_TRUE(!device->GetName() || device->GetName()->empty());
+
+  TestBluetoothAdapterObserver observer(adapter_);
+  SimulateGattNameChange(device, kTestDeviceName);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_EQ(base::UTF8ToUTF16(kTestDeviceName), device->GetNameForDisplay());
+}
+
 }  // namespace device

device/bluetooth/bluetooth_device_winrt.cc

@@ -136,24 +136,27 @@ void RemoveGattServicesChangedHandler(IBluetoothLEDevice* ble_device,
   }
 }
 
+void RemoveNameChangedHandler(IBluetoothLEDevice* ble_device,
+                              EventRegistrationToken token) {
+  HRESULT hr = ble_device->remove_NameChanged(token);
+  if (FAILED(hr)) {
+    VLOG(2) << "Removing NameChanged Handler failed: "
+            << logging::SystemErrorCodeToString(hr);
+  }
+}
+
 }  // namespace
 
-BluetoothDeviceWinrt::BluetoothDeviceWinrt(
-    BluetoothAdapterWinrt* adapter,
-    uint64_t raw_address,
-    base::Optional<std::string> local_name)
+BluetoothDeviceWinrt::BluetoothDeviceWinrt(BluetoothAdapterWinrt* adapter,
+                                           uint64_t raw_address)
     : BluetoothDevice(adapter),
       raw_address_(raw_address),
       address_(CanonicalizeAddress(raw_address)),
-      local_name_(std::move(local_name)),
       weak_ptr_factory_(this) {}
 
 BluetoothDeviceWinrt::~BluetoothDeviceWinrt() {
   CloseDevice(ble_device_);
-  if (!connection_changed_token_)
-    return;
-
-  RemoveConnectionStatusHandler(ble_device_.Get(), *connection_changed_token_);
+  ClearEventRegistrations();
 }
 
 uint32_t BluetoothDeviceWinrt::GetBluetoothClass() const {
@@ -202,6 +205,10 @@ base::Optional<std::string> BluetoothDeviceWinrt::GetName() const {
     return local_name_;
   }
 
+  // Prefer returning |local_name_| over an empty string.
+  if (!name)
+    return local_name_;
+
   return base::win::ScopedHString(name).GetAsUTF8();
 }
 
@@ -404,6 +411,14 @@ std::string BluetoothDeviceWinrt::CanonicalizeAddress(uint64_t address) {
   return bluetooth_address;
 }
 
+void BluetoothDeviceWinrt::UpdateLocalName(
+    base::Optional<std::string> local_name) {
+  if (!local_name)
+    return;
+
+  local_name_ = std::move(local_name);
+}
+
 void BluetoothDeviceWinrt::CreateGattConnectionImpl() {
   ComPtr<IBluetoothLEDeviceStatics> device_statics;
   HRESULT hr = GetBluetoothLEDeviceStaticsActivationFactory(&device_statics);
@@ -482,15 +497,12 @@ void BluetoothDeviceWinrt::OnFromBluetoothAddress(
     return;
   }
 
-  if (connection_changed_token_) {
-    // As we are about to replace |ble_device_| with |ble_device| we will also
-    // unregister the existing event handler and add a new one to the new
-    // device.
-    RemoveConnectionStatusHandler(ble_device_.Get(),
-                                  *connection_changed_token_);
-  }
+  // As we are about to replace |ble_device_| with |ble_device| existing event
+  // handlers need to be unregistered. New ones will be added below.
+  ClearEventRegistrations();
 
   ble_device_ = std::move(ble_device);
+
   connection_changed_token_ = AddTypedEventHandler(
       ble_device_.Get(), &IBluetoothLEDevice::add_ConnectionStatusChanged,
       base::BindRepeating(&BluetoothDeviceWinrt::OnConnectionStatusChanged,
@@ -503,11 +515,6 @@ void BluetoothDeviceWinrt::OnFromBluetoothAddress(
   if (IsGattConnected())
     DidConnectGatt();
 
-  if (gatt_services_changed_token_) {
-    RemoveGattServicesChangedHandler(ble_device_.Get(),
-                                     *gatt_services_changed_token_);
-  }
-
   gatt_services_changed_token_ = AddTypedEventHandler(
       ble_device_.Get(), &IBluetoothLEDevice::add_GattServicesChanged,
       base::BindRepeating(&BluetoothDeviceWinrt::OnGattServicesChanged,
@@ -520,6 +527,11 @@ void BluetoothDeviceWinrt::OnFromBluetoothAddress(
   gatt_discoverer_->StartGattDiscovery(
       base::BindOnce(&BluetoothDeviceWinrt::OnGattDiscoveryComplete,
                      weak_ptr_factory_.GetWeakPtr()));
+
+  name_changed_token_ = AddTypedEventHandler(
+      ble_device_.Get(), &IBluetoothLEDevice::add_NameChanged,
+      base::BindRepeating(&BluetoothDeviceWinrt::OnNameChanged,
+                          weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BluetoothDeviceWinrt::OnConnectionStatusChanged(
@@ -554,6 +566,12 @@ void BluetoothDeviceWinrt::OnGattServicesChanged(IBluetoothLEDevice* ble_device,
   }
 }
 
+void BluetoothDeviceWinrt::OnNameChanged(IBluetoothLEDevice* ble_device,
+                                         IInspectable* object) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  adapter_->NotifyDeviceChanged(this);
+}
+
 void BluetoothDeviceWinrt::OnGattDiscoveryComplete(bool success) {
   if (!success) {
     if (!IsGattConnected())
@@ -600,4 +618,19 @@ void BluetoothDeviceWinrt::ClearGattServices() {
   SetGattServicesDiscoveryComplete(false);
 }
 
+void BluetoothDeviceWinrt::ClearEventRegistrations() {
+  if (connection_changed_token_) {
+    RemoveConnectionStatusHandler(ble_device_.Get(),
+                                  *connection_changed_token_);
+  }
+
+  if (gatt_services_changed_token_) {
+    RemoveGattServicesChangedHandler(ble_device_.Get(),
+                                     *gatt_services_changed_token_);
+  }
+
+  if (name_changed_token_)
+    RemoveNameChangedHandler(ble_device_.Get(), *name_changed_token_);
+}
+
 }  // namespace device

device/bluetooth/bluetooth_device_winrt.h

@@ -37,9 +37,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceWinrt : public BluetoothDevice {
   static constexpr uint8_t k32BitServiceDataSection = 0x20;
   static constexpr uint8_t k128BitServiceDataSection = 0x21;
 
-  BluetoothDeviceWinrt(BluetoothAdapterWinrt* adapter,
-                       uint64_t raw_address,
-                       base::Optional<std::string> local_name);
+  BluetoothDeviceWinrt(BluetoothAdapterWinrt* adapter, uint64_t raw_address);
   ~BluetoothDeviceWinrt() override;
 
   // BluetoothDevice:
@@ -91,6 +89,9 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceWinrt : public BluetoothDevice {
   // each 'X' is a hex digit.
   static std::string CanonicalizeAddress(uint64_t address);
 
+  // Called by BluetoothAdapterWinrt when an advertisement packet is received.
+  void UpdateLocalName(base::Optional<std::string> local_name);
+
  protected:
   // BluetoothDevice:
   void CreateGattConnectionImpl() override;
@@ -117,9 +118,14 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceWinrt : public BluetoothDevice {
       ABI::Windows::Devices::Bluetooth::IBluetoothLEDevice* ble_device,
       IInspectable* object);
 
+  void OnNameChanged(
+      ABI::Windows::Devices::Bluetooth::IBluetoothLEDevice* ble_device,
+      IInspectable* object);
+
   void OnGattDiscoveryComplete(bool success);
 
   void ClearGattServices();
+  void ClearEventRegistrations();
 
   uint64_t raw_address_;
   std::string address_;
@@ -131,6 +137,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceWinrt : public BluetoothDevice {
 
   base::Optional<EventRegistrationToken> connection_changed_token_;
   base::Optional<EventRegistrationToken> gatt_services_changed_token_;
+  base::Optional<EventRegistrationToken> name_changed_token_;
 
   THREAD_CHECKER(thread_checker_);
 

device/bluetooth/test/bluetooth_test.h

@@ -324,6 +324,11 @@ class BluetoothTestBase : public testing::Test {
   // SimulateGattDisconnection(device).
   virtual void SimulateDeviceBreaksConnection(BluetoothDevice* device);
 
+  // Simulates a device changing its name property while a GATT connection is
+  // open.
+  virtual void SimulateGattNameChange(BluetoothDevice* device,
+                                      const std::string& new_name) {}
+
   // Simulates success of discovering services. |uuids| is used to create a
   // service for each UUID string. Multiple UUIDs with the same value produce
   // multiple service instances.

device/bluetooth/test/bluetooth_test_win.cc

@@ -120,9 +120,8 @@ class TestBluetoothDeviceWinrt : public BluetoothDeviceWinrt {
  public:
   TestBluetoothDeviceWinrt(BluetoothAdapterWinrt* adapter,
                            uint64_t raw_address,
-                           base::Optional<std::string> local_name,
                            BluetoothTestWinrt* bluetooth_test_winrt)
-      : BluetoothDeviceWinrt(adapter, raw_address, std::move(local_name)),
+      : BluetoothDeviceWinrt(adapter, raw_address),
         bluetooth_test_winrt_(bluetooth_test_winrt) {}
 
   HRESULT GetBluetoothLEDeviceStaticsActivationFactory(
@@ -183,10 +182,9 @@ class TestBluetoothAdapterWinrt : public BluetoothAdapterWinrt {
   }
 
   std::unique_ptr<BluetoothDeviceWinrt> CreateDevice(
-      uint64_t raw_address,
-      base::Optional<std::string> local_name) override {
-    return std::make_unique<TestBluetoothDeviceWinrt>(
-        this, raw_address, std::move(local_name), bluetooth_test_winrt_);
+      uint64_t raw_address) override {
+    return std::make_unique<TestBluetoothDeviceWinrt>(this, raw_address,
+                                                      bluetooth_test_winrt_);
   }
 
  private:
@@ -821,6 +819,17 @@ void BluetoothTestWinrt::SimulateGattConnection(BluetoothDevice* device) {
   ble_device->SimulateGattConnection();
 }
 
+void BluetoothTestWinrt::SimulateGattNameChange(BluetoothDevice* device,
+                                                const std::string& new_name) {
+  if (!GetParam() || !PlatformSupportsLowEnergy())
+    return BluetoothTestWin::SimulateGattNameChange(device, new_name);
+
+  auto* const ble_device =
+      static_cast<TestBluetoothDeviceWinrt*>(device)->ble_device();
+  DCHECK(ble_device);
+  ble_device->SimulateGattNameChange(new_name);
+}
+
 void BluetoothTestWinrt::SimulateGattConnectionError(
     BluetoothDevice* device,
     BluetoothDevice::ConnectErrorCode error_code) {

device/bluetooth/test/bluetooth_test_win.h

@@ -142,6 +142,8 @@ class BluetoothTestWinrt : public BluetoothTestWin,
       BluetoothDevice::ConnectErrorCode error_code) override;
   void SimulateGattDisconnection(BluetoothDevice* device) override;
   void SimulateDeviceBreaksConnection(BluetoothDevice* device) override;
+  void SimulateGattNameChange(BluetoothDevice* device,
+                              const std::string& new_name) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
       const std::vector<std::string>& uuids) override;

device/bluetooth/test/fake_bluetooth_le_device_winrt.cc

@@ -13,6 +13,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/win/async_operation.h"
+#include "base/win/scoped_hstring.h"
 #include "device/bluetooth/bluetooth_remote_gatt_service_winrt.h"
 #include "device/bluetooth/test/bluetooth_test_win.h"
 #include "device/bluetooth/test/fake_device_information_pairing_winrt.h"
@@ -68,7 +69,11 @@ HRESULT FakeBluetoothLEDeviceWinrt::get_DeviceId(HSTRING* value) {
 }
 
 HRESULT FakeBluetoothLEDeviceWinrt::get_Name(HSTRING* value) {
-  return E_NOTIMPL;
+  if (!name_)
+    return E_FAIL;
+
+  *value = base::win::ScopedHString::Create(*name_).release();
+  return S_OK;
 }
 
 HRESULT FakeBluetoothLEDeviceWinrt::get_GattServices(
@@ -95,12 +100,14 @@ HRESULT FakeBluetoothLEDeviceWinrt::GetGattService(
 HRESULT FakeBluetoothLEDeviceWinrt::add_NameChanged(
     ITypedEventHandler<BluetoothLEDevice*, IInspectable*>* handler,
     EventRegistrationToken* token) {
-  return E_NOTIMPL;
+  name_changed_handler_ = handler;
+  return S_OK;
 }
 
 HRESULT FakeBluetoothLEDeviceWinrt::remove_NameChanged(
     EventRegistrationToken token) {
-  return E_NOTIMPL;
+  name_changed_handler_ = nullptr;
+  return S_OK;
 }
 
 HRESULT FakeBluetoothLEDeviceWinrt::add_GattServicesChanged(
@@ -253,6 +260,12 @@ void FakeBluetoothLEDeviceWinrt::SimulateDeviceBreaksConnection() {
   connection_status_changed_handler_->Invoke(this, nullptr);
 }
 
+void FakeBluetoothLEDeviceWinrt::SimulateGattNameChange(
+    const std::string& new_name) {
+  name_ = new_name;
+  name_changed_handler_->Invoke(this, nullptr);
+}
+
 void FakeBluetoothLEDeviceWinrt::SimulateGattServicesDiscovered(
     const std::vector<std::string>& uuids) {
   for (const auto& uuid : uuids) {

device/bluetooth/test/fake_bluetooth_le_device_winrt.h

@@ -127,6 +127,7 @@ class FakeBluetoothLEDeviceWinrt
       BluetoothDevice::ConnectErrorCode error_code);
   void SimulateGattDisconnection();
   void SimulateDeviceBreaksConnection();
+  void SimulateGattNameChange(const std::string& new_name);
   void SimulateGattServicesDiscovered(const std::vector<std::string>& uuids);
   void SimulateGattServicesChanged();
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service);
@@ -140,6 +141,7 @@ class FakeBluetoothLEDeviceWinrt
  private:
   BluetoothTestWinrt* bluetooth_test_winrt_ = nullptr;
   uint32_t reference_count_ = 1u;
+  base::Optional<std::string> name_;
 
   ABI::Windows::Devices::Bluetooth::BluetoothConnectionStatus status_ =
       ABI::Windows::Devices::Bluetooth::BluetoothConnectionStatus_Disconnected;
@@ -167,6 +169,11 @@ class FakeBluetoothLEDeviceWinrt
       fake_services_;
   uint16_t service_attribute_handle_ = 0;
 
+  Microsoft::WRL::ComPtr<ABI::Windows::Foundation::ITypedEventHandler<
+      ABI::Windows::Devices::Bluetooth::BluetoothLEDevice*,
+      IInspectable*>>
+      name_changed_handler_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeBluetoothLEDeviceWinrt);
 };