bluetooth: Query for devices every second in TrayBluetoothHelper

Before TrayBluetoothHelper would get notified of any changes to devices.
This caused problems in busy environments because TrayBluetoothHelper
would get notified hundreds or even thousands of times per second of
devices changes.

This CL changes to a "pull" pattern where TrayBluetootHelper queries
for devices every second. The devices are then cached so that clients
of TrayBluetoothHelper can access them synchronously.

TrayBluetoothHelper queries for devices as long as the state is
kPoweredOn. If the state changes, the device list is cleared.

Bug: 882346

TEST=The following actions should be performed:

1. Turn bluetooth on and off in system tray.
2. Open device list in system menu.
3. Connect to a device.
4. See that the pod feature button in the system tray indicates a
   device is connected when connected to a device.

Change-Id: Id9028546a627f260527aa737f51c032f74a447ec
Reviewed-on: https://chromium-review.googlesource.com/c/1347638
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611069}

ash/system/bluetooth/bluetooth_feature_pod_controller.cc

@@ -99,7 +99,7 @@ void BluetoothFeaturePodController::UpdateButton() {
        Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices()) {
     if (device->connection_state ==
         BluetoothDeviceInfo::ConnectionState::kConnected) {
-      connected_devices.push_back(std::move(device));
+      connected_devices.push_back(device->Clone());
     }
   }
 

ash/system/bluetooth/tray_bluetooth_helper.cc

@@ -3,11 +3,19 @@
 // found in the LICENSE file.
 
 #include "ash/system/bluetooth/tray_bluetooth_helper.h"
+#include "base/time/time.h"
 
 using device::mojom::BluetoothSystem;
 
 namespace ash {
 
+namespace {
+
+constexpr base::TimeDelta kUpdateFrequencyMs =
+    base::TimeDelta::FromMilliseconds(1000);
+
+}  // namespace
+
 TrayBluetoothHelper::TrayBluetoothHelper() = default;
 
 TrayBluetoothHelper::~TrayBluetoothHelper() = default;
@@ -20,6 +28,11 @@ void TrayBluetoothHelper::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+const BluetoothDeviceList& TrayBluetoothHelper::GetAvailableBluetoothDevices()
+    const {
+  return cached_devices_;
+}
+
 bool TrayBluetoothHelper::IsBluetoothStateAvailable() {
   switch (GetBluetoothState()) {
     case BluetoothSystem::State::kUnsupported:
@@ -32,6 +45,36 @@ bool TrayBluetoothHelper::IsBluetoothStateAvailable() {
   }
 }
 
+void TrayBluetoothHelper::StartOrStopRefreshingDeviceList() {
+  if (GetBluetoothState() == BluetoothSystem::State::kPoweredOn) {
+    DCHECK(!timer_.IsRunning());
+    UpdateDeviceCache();
+    timer_.Start(FROM_HERE, kUpdateFrequencyMs, this,
+                 &TrayBluetoothHelper::UpdateDeviceCache);
+    return;
+  }
+
+  timer_.Stop();
+  cached_devices_.clear();
+  NotifyBluetoothDeviceListChanged();
+}
+
+void TrayBluetoothHelper::UpdateDeviceCache() {
+  GetBluetoothDevices(
+      base::BindOnce(&TrayBluetoothHelper::OnGetBluetoothDevices,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void TrayBluetoothHelper::OnGetBluetoothDevices(BluetoothDeviceList devices) {
+  cached_devices_ = std::move(devices);
+  NotifyBluetoothDeviceListChanged();
+}
+
+void TrayBluetoothHelper::NotifyBluetoothDeviceListChanged() {
+  for (auto& observer : observers_)
+    observer.OnBluetoothDeviceListChanged();
+}
+
 void TrayBluetoothHelper::NotifyBluetoothSystemStateChanged() {
   for (auto& observer : observers_)
     observer.OnBluetoothSystemStateChanged();
@@ -42,9 +85,4 @@ void TrayBluetoothHelper::NotifyBluetoothScanStateChanged() {
     observer.OnBluetoothScanStateChanged();
 }
 
-void TrayBluetoothHelper::NotifyBluetoothDeviceListChanged() {
-  for (auto& observer : observers_)
-    observer.OnBluetoothDeviceListChanged();
-}
-
 }  // namespace ash

ash/system/bluetooth/tray_bluetooth_helper.h

@@ -24,7 +24,7 @@ using BluetoothDeviceList = std::vector<device::mojom::BluetoothDeviceInfoPtr>;
 // This is a temporary virtual class used during the migration to the new
 // BluetoothSystem Mojo interface. Once the migration is over, we'll
 // de-virtualize this class and remove its legacy implementation.
-class TrayBluetoothHelper {
+class ASH_EXPORT TrayBluetoothHelper {
  public:
   class Observer : public base::CheckedObserver {
    public:
@@ -48,7 +48,7 @@ class TrayBluetoothHelper {
   virtual void Initialize() = 0;
 
   // Returns a list of available bluetooth devices.
-  virtual BluetoothDeviceList GetAvailableBluetoothDevices() const = 0;
+  const BluetoothDeviceList& GetAvailableBluetoothDevices() const;
 
   // Requests bluetooth start discovering devices, which happens asynchronously.
   virtual void StartBluetoothDiscovering() = 0;
@@ -75,13 +75,40 @@ class TrayBluetoothHelper {
   virtual bool HasBluetoothDiscoverySession() = 0;
 
  protected:
+  using GetBluetoothDevicesCallback =
+      base::OnceCallback<void(BluetoothDeviceList)>;
+
+  // Using a "push" pattern where the underlying API notifies of device changes
+  // is undesireable because there are hundreds or sometimes thousands of
+  // changes per second. This could result in significantly slowing down the UI.
+  // To avoid this we use a pull pattern where we retrieve the device list every
+  // second and notify observers.
+  //
+  // Implementations of TrayBluetoothHelper should call this whenever the state
+  // changes.
+  void StartOrStopRefreshingDeviceList();
+
   void NotifyBluetoothSystemStateChanged();
   void NotifyBluetoothScanStateChanged();
-  void NotifyBluetoothDeviceListChanged();
+
+  virtual void GetBluetoothDevices(
+      GetBluetoothDevicesCallback callback) const = 0;
 
   base::ObserverList<Observer> observers_;
 
  private:
+  void UpdateDeviceCache();
+  void OnGetBluetoothDevices(BluetoothDeviceList devices);
+  void NotifyBluetoothDeviceListChanged();
+
+  // List of cached devices. Updated every second.
+  BluetoothDeviceList cached_devices_;
+
+  // Timer used to update |cached_devices_|.
+  base::RepeatingTimer timer_;
+
+  base::WeakPtrFactory<TrayBluetoothHelper> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(TrayBluetoothHelper);
 };
 

ash/system/bluetooth/tray_bluetooth_helper_experimental.cc

@@ -48,12 +48,6 @@ void TrayBluetoothHelperExperimental::Initialize() {
                      base::Unretained(this)));
 }
 
-BluetoothDeviceList
-TrayBluetoothHelperExperimental::GetAvailableBluetoothDevices() const {
-  NOTIMPLEMENTED();
-  return BluetoothDeviceList();
-}
-
 void TrayBluetoothHelperExperimental::StartBluetoothDiscovering() {
   bluetooth_system_ptr_->StartScan(base::DoNothing());
 }
@@ -81,10 +75,19 @@ bool TrayBluetoothHelperExperimental::HasBluetoothDiscoverySession() {
          device::mojom::BluetoothSystem::ScanState::kScanning;
 }
 
+void TrayBluetoothHelperExperimental::GetBluetoothDevices(
+    GetBluetoothDevicesCallback callback) const {
+  NOTIMPLEMENTED();
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), BluetoothDeviceList()));
+}
+
 void TrayBluetoothHelperExperimental::OnStateChanged(
     device::mojom::BluetoothSystem::State state) {
   cached_state_ = state;
+
   NotifyBluetoothSystemStateChanged();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperExperimental::OnScanStateChanged(

ash/system/bluetooth/tray_bluetooth_helper_experimental.h

@@ -31,13 +31,13 @@ class TrayBluetoothHelperExperimental
 
   // TrayBluetoothHelper:
   void Initialize() override;
-  BluetoothDeviceList GetAvailableBluetoothDevices() const override;
   void StartBluetoothDiscovering() override;
   void StopBluetoothDiscovering() override;
   void ConnectToBluetoothDevice(const std::string& address) override;
   device::mojom::BluetoothSystem::State GetBluetoothState() override;
   void SetBluetoothEnabled(bool enabled) override;
   bool HasBluetoothDiscoverySession() override;
+  void GetBluetoothDevices(GetBluetoothDevicesCallback callback) const override;
 
   // device::mojom::BluetoothSystemClient
   void OnStateChanged(device::mojom::BluetoothSystem::State state) override;

ash/system/bluetooth/tray_bluetooth_helper_legacy.cc

@@ -114,6 +114,8 @@ void TrayBluetoothHelperLegacy::InitializeOnAdapterReady(
   adapter_ = adapter;
   CHECK(adapter_);
   adapter_->AddObserver(this);
+
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::Initialize() {
@@ -122,20 +124,6 @@ void TrayBluetoothHelperLegacy::Initialize() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-BluetoothDeviceList TrayBluetoothHelperLegacy::GetAvailableBluetoothDevices()
-    const {
-  device::BluetoothAdapter::DeviceList devices =
-      device::FilterBluetoothDeviceList(adapter_->GetDevices(),
-                                        device::BluetoothFilterType::KNOWN,
-                                        kMaximumDevicesShown);
-
-  BluetoothDeviceList device_list;
-  for (device::BluetoothDevice* device : devices)
-    device_list.push_back(GetBluetoothDeviceInfo(device));
-
-  return device_list;
-}
-
 void TrayBluetoothHelperLegacy::StartBluetoothDiscovering() {
   if (HasBluetoothDiscoverySession()) {
     LOG(WARNING) << "Already have active Bluetooth device discovery session.";
@@ -209,6 +197,20 @@ bool TrayBluetoothHelperLegacy::HasBluetoothDiscoverySession() {
   return discovery_session_ && discovery_session_->IsActive();
 }
 
+void TrayBluetoothHelperLegacy::GetBluetoothDevices(
+    GetBluetoothDevicesCallback callback) const {
+  BluetoothDeviceList device_list;
+  device::BluetoothAdapter::DeviceList devices =
+      device::FilterBluetoothDeviceList(adapter_->GetDevices(),
+                                        device::BluetoothFilterType::KNOWN,
+                                        kMaximumDevicesShown);
+  for (device::BluetoothDevice* device : devices)
+    device_list.push_back(GetBluetoothDeviceInfo(device));
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(device_list)));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BluetoothAdapter::Observer:
 
@@ -216,12 +218,14 @@ void TrayBluetoothHelperLegacy::AdapterPresentChanged(
     device::BluetoothAdapter* adapter,
     bool present) {
   NotifyBluetoothSystemStateChanged();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::AdapterPoweredChanged(
     device::BluetoothAdapter* adapter,
     bool powered) {
   NotifyBluetoothSystemStateChanged();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::AdapterDiscoveringChanged(
@@ -230,21 +234,6 @@ void TrayBluetoothHelperLegacy::AdapterDiscoveringChanged(
   NotifyBluetoothScanStateChanged();
 }
 
-void TrayBluetoothHelperLegacy::DeviceAdded(device::BluetoothAdapter* adapter,
-                                            device::BluetoothDevice* device) {
-  NotifyBluetoothDeviceListChanged();
-}
-
-void TrayBluetoothHelperLegacy::DeviceChanged(device::BluetoothAdapter* adapter,
-                                              device::BluetoothDevice* device) {
-  NotifyBluetoothDeviceListChanged();
-}
-
-void TrayBluetoothHelperLegacy::DeviceRemoved(device::BluetoothAdapter* adapter,
-                                              device::BluetoothDevice* device) {
-  NotifyBluetoothDeviceListChanged();
-}
-
 void TrayBluetoothHelperLegacy::OnStartDiscoverySession(
     std::unique_ptr<device::BluetoothDiscoverySession> discovery_session) {
   // If the discovery session was returned after a request to stop discovery

ash/system/bluetooth/tray_bluetooth_helper_legacy.h

@@ -38,13 +38,13 @@ class ASH_EXPORT TrayBluetoothHelperLegacy
 
   // TrayBluetoothHelper:
   void Initialize() override;
-  BluetoothDeviceList GetAvailableBluetoothDevices() const override;
   void StartBluetoothDiscovering() override;
   void StopBluetoothDiscovering() override;
   void ConnectToBluetoothDevice(const std::string& address) override;
   device::mojom::BluetoothSystem::State GetBluetoothState() override;
   void SetBluetoothEnabled(bool enabled) override;
   bool HasBluetoothDiscoverySession() override;
+  void GetBluetoothDevices(GetBluetoothDevicesCallback callback) const override;
 
   // BluetoothAdapter::Observer:
   void AdapterPresentChanged(device::BluetoothAdapter* adapter,
@@ -53,12 +53,6 @@ class ASH_EXPORT TrayBluetoothHelperLegacy
                              bool powered) override;
   void AdapterDiscoveringChanged(device::BluetoothAdapter* adapter,
                                  bool discovering) override;
-  void DeviceAdded(device::BluetoothAdapter* adapter,
-                   device::BluetoothDevice* device) override;
-  void DeviceChanged(device::BluetoothAdapter* adapter,
-                     device::BluetoothDevice* device) override;
-  void DeviceRemoved(device::BluetoothAdapter* adapter,
-                     device::BluetoothDevice* device) override;
 
  private:
   void OnStartDiscoverySession(

ash/system/bluetooth/tray_bluetooth_helper_legacy_unittest.cc

@@ -7,6 +7,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/system/bluetooth/tray_bluetooth_helper.h"
 #include "ash/test/ash_test_base.h"
 #include "base/test/scoped_feature_list.h"
 #include "dbus/object_path.h"
@@ -43,6 +44,10 @@ TEST_F(TrayBluetoothHelperLegacyTest, Basics) {
       static_cast<FakeBluetoothAdapterClient*>(
           BluezDBusManager::Get()->GetBluetoothAdapterClient());
   adapter_client->SetSimulationIntervalMs(0);
+  adapter_client
+      ->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath))
+      ->powered.ReplaceValue(true);
 
   FakeBluetoothDeviceClient* device_client =
       static_cast<FakeBluetoothDeviceClient*>(
@@ -59,11 +64,11 @@ TEST_F(TrayBluetoothHelperLegacyTest, Basics) {
   TrayBluetoothHelperLegacy helper;
   helper.Initialize();
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(device::mojom::BluetoothSystem::State::kPoweredOff,
+  EXPECT_EQ(device::mojom::BluetoothSystem::State::kPoweredOn,
             helper.GetBluetoothState());
   EXPECT_FALSE(helper.HasBluetoothDiscoverySession());
 
-  BluetoothDeviceList devices = helper.GetAvailableBluetoothDevices();
+  const BluetoothDeviceList& devices = helper.GetAvailableBluetoothDevices();
   // The devices are fake in tests, so don't assume any particular number.
   EXPECT_FALSE(devices.empty());
   EXPECT_TRUE(ExistInFilteredDevices(
@@ -134,6 +139,10 @@ TEST_F(TrayBluetoothHelperLegacyTest, UnfilteredBluetoothDevices) {
       static_cast<FakeBluetoothAdapterClient*>(
           BluezDBusManager::Get()->GetBluetoothAdapterClient());
   adapter_client->SetSimulationIntervalMs(0);
+  adapter_client
+      ->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath))
+      ->powered.ReplaceValue(true);
 
   FakeBluetoothDeviceClient* device_client =
       static_cast<FakeBluetoothDeviceClient*>(
@@ -150,7 +159,7 @@ TEST_F(TrayBluetoothHelperLegacyTest, UnfilteredBluetoothDevices) {
   helper.Initialize();
   base::RunLoop().RunUntilIdle();
 
-  BluetoothDeviceList devices = helper.GetAvailableBluetoothDevices();
+  const BluetoothDeviceList& devices = helper.GetAvailableBluetoothDevices();
   // The devices are fake in tests, so don't assume any particular number.
   EXPECT_TRUE(ExistInFilteredDevices(
       FakeBluetoothDeviceClient::kDisplayPinCodeAddress, devices));

ash/system/bluetooth/unified_bluetooth_detailed_view_controller.cc

@@ -22,8 +22,6 @@ using device::mojom::BluetoothDeviceInfoPtr;
 
 namespace ash {
 
-const int kUpdateFrequencyMs = 1000;
-
 namespace {
 
 // Updates bluetooth device |device| in the |list|. If it is new, append to the
@@ -91,27 +89,17 @@ void UnifiedBluetoothDetailedViewController::OnBluetoothSystemStateChanged() {
   if (bluetooth_state == BluetoothSystem::State::kPoweredOn) {
     // If Bluetooth was just turned on, start discovering.
     Shell::Get()->tray_bluetooth_helper()->StartBluetoothDiscovering();
-  } else {
-    // Otherwise stop updating the list of devices.
-    timer_.Stop();
   }
 
   UpdateDeviceListAndUI();
 }
 
 void UnifiedBluetoothDetailedViewController::OnBluetoothScanStateChanged() {
-  // To avoid delaying showing devices, update the device list and UI
-  // immediately.
   UpdateDeviceListAndUI();
+}
 
-  if (Shell::Get()->tray_bluetooth_helper()->HasBluetoothDiscoverySession()) {
-    // Update the device list and UI every |kUpdateFrequencyMs|.
-    timer_.Start(
-        FROM_HERE, base::TimeDelta::FromMilliseconds(kUpdateFrequencyMs), this,
-        &UnifiedBluetoothDetailedViewController::UpdateDeviceListAndUI);
-    return;
-  }
-  timer_.Stop();
+void UnifiedBluetoothDetailedViewController::OnBluetoothDeviceListChanged() {
+  UpdateDeviceListAndUI();
 }
 
 void UnifiedBluetoothDetailedViewController::UpdateDeviceListAndUI() {
@@ -151,25 +139,28 @@ void UnifiedBluetoothDetailedViewController::UpdateBluetoothDeviceList() {
   std::set<std::string> new_paired_not_connected_devices;
   std::set<std::string> new_discovered_not_paired_devices;
 
-  for (auto& device :
+  for (const auto& device :
        Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices()) {
+    auto device_clone = device->Clone();
     if (device->connection_state ==
         BluetoothDeviceInfo::ConnectionState::kConnecting) {
       new_connecting_devices.insert(device->address);
-      UpdateBluetoothDeviceListHelper(&connecting_devices_, std::move(device));
+      UpdateBluetoothDeviceListHelper(&connecting_devices_,
+                                      std::move(device_clone));
     } else if (device->connection_state ==
                    BluetoothDeviceInfo::ConnectionState::kConnected &&
                device->is_paired) {
       new_connected_devices.insert(device->address);
-      UpdateBluetoothDeviceListHelper(&connected_devices_, std::move(device));
+      UpdateBluetoothDeviceListHelper(&connected_devices_,
+                                      std::move(device_clone));
     } else if (device->is_paired) {
       new_paired_not_connected_devices.insert(device->address);
       UpdateBluetoothDeviceListHelper(&paired_not_connected_devices_,
-                                      std::move(device));
+                                      std::move(device_clone));
     } else {
       new_discovered_not_paired_devices.insert(device->address);
       UpdateBluetoothDeviceListHelper(&discovered_not_paired_devices_,
-                                      std::move(device));
+                                      std::move(device_clone));
     }
   }
   RemoveObsoleteBluetoothDevicesFromList(&connecting_devices_,

ash/system/bluetooth/unified_bluetooth_detailed_view_controller.h

@@ -36,6 +36,7 @@ class UnifiedBluetoothDetailedViewController
   // BluetoothObserver:
   void OnBluetoothSystemStateChanged() override;
   void OnBluetoothScanStateChanged() override;
+  void OnBluetoothDeviceListChanged() override;
 
  private:
   void UpdateDeviceListAndUI();
@@ -50,9 +51,6 @@ class UnifiedBluetoothDetailedViewController
   BluetoothDeviceList paired_not_connected_devices_;
   BluetoothDeviceList discovered_not_paired_devices_;
 
-  // Timer used to limit the update frequency.
-  base::RepeatingTimer timer_;
-
   DISALLOW_COPY_AND_ASSIGN(UnifiedBluetoothDetailedViewController);
 };