Reland "bluetooth: Query for devices every second in TrayBluetoothHelper"

This is a reland of e45a6ef0

The original CL was reverted because a DCHECK was being hit. The
TrayBluetoothHelperLegacy implementation would sometimes call
OnBluetoothSystemStateChanged without the state actually changing
which would cause the DCHECK to be hit.

A separate CL (https://crrev.com/c/1352123) fixed
TrayBluetoothHelperLegacy so now the DCHECK shouldn't be hit.

Original change's description:
> 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}

TBR=tetsui@chromium.org

Bug: 882346
Change-Id: Ief9ce3d77a317dfde2a5fcc49cb0c598aaa9012d
Reviewed-on: https://chromium-review.googlesource.com/c/1352078Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611949}

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

@@ -48,7 +48,7 @@ class ASH_EXPORT 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 ASH_EXPORT 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,7 +114,9 @@ void TrayBluetoothHelperLegacy::InitializeOnAdapterReady(
   adapter_ = adapter;
   CHECK(adapter_);
   adapter_->AddObserver(this);
+
   last_state_ = GetBluetoothState();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::Initialize() {
@@ -123,20 +125,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.";
@@ -210,6 +198,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:
 
@@ -221,6 +223,7 @@ void TrayBluetoothHelperLegacy::AdapterPresentChanged(
 
   last_state_ = GetBluetoothState();
   NotifyBluetoothSystemStateChanged();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::AdapterPoweredChanged(
@@ -231,6 +234,7 @@ void TrayBluetoothHelperLegacy::AdapterPoweredChanged(
 
   last_state_ = GetBluetoothState();
   NotifyBluetoothSystemStateChanged();
+  StartOrStopRefreshingDeviceList();
 }
 
 void TrayBluetoothHelperLegacy::AdapterDiscoveringChanged(
@@ -239,21 +243,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"
@@ -78,6 +79,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*>(
@@ -94,10 +99,11 @@ TEST_F(TrayBluetoothHelperLegacyTest, Basics) {
   TrayBluetoothHelperLegacy helper;
   helper.Initialize();
   RunAllPendingInMessageLoop();
-  EXPECT_EQ(BluetoothSystem::State::kPoweredOff, helper.GetBluetoothState());
+  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(
@@ -254,6 +260,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*>(
@@ -270,7 +280,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);
 };