Add RequestDevices to FirmwareUpdateManager.

This function requests the list of currently connected devices
from the fwupd DBus client.

Bug: 1252981
Test: ash_unittests
Change-Id: I16d9b2f47595fce76c82ec35fe686a322b77b913
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3234593
Commit-Queue: Anton Swifton <swifton@google.com>
Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/main@{#935320}

ash/components/fwupd/BUILD.gn

@@ -9,7 +9,11 @@ assert(is_chromeos_ash, "Non-Chrome-OS builds must not depend on //ash")
 component("fwupd") {
   defines = [ "IS_ASH_FIRMWARE_UPDATE_MANAGER_IMPL" ]
 
-  deps = [ "//base:base" ]
+  deps = [
+    "//base:base",
+    "//chromeos/dbus/fwupd",
+    "//dbus",
+  ]
 
   sources = [
     "firmware_update_manager.cc",
@@ -22,6 +26,8 @@ source_set("unit_tests") {
 
   deps = [
     ":fwupd",
+    "//chromeos/dbus/fwupd",
+    "//dbus:test_support",
     "//testing/gtest",
   ]
 

ash/components/fwupd/firmware_update_manager.cc

@@ -5,6 +5,8 @@
 #include "ash/components/fwupd/firmware_update_manager.h"
 
 #include "base/check_op.h"
+#include "chromeos/dbus/fwupd/fwupd_client.h"
+#include "dbus/message.h"
 
 namespace ash {
 
@@ -15,12 +17,17 @@ FirmwareUpdateManager* g_instance = nullptr;
 }  // namespace
 
 FirmwareUpdateManager::FirmwareUpdateManager() {
+  DCHECK(chromeos::FwupdClient::Get());
+  chromeos::FwupdClient::Get()->AddObserver(this);
+
   DCHECK_EQ(nullptr, g_instance);
   g_instance = this;
+  g_instance->RequestDevices();
 }
 
 FirmwareUpdateManager::~FirmwareUpdateManager() {
   DCHECK_EQ(this, g_instance);
+  chromeos::FwupdClient::Get()->RemoveObserver(this);
   g_instance = nullptr;
 }
 
@@ -30,4 +37,15 @@ FirmwareUpdateManager* FirmwareUpdateManager::Get() {
   return g_instance;
 }
 
+void FirmwareUpdateManager::RequestDevices() {
+  chromeos::FwupdClient::Get()->GetDevices();
+}
+
+void FirmwareUpdateManager::OnDeviceListResponse(
+    chromeos::FwupdDeviceList* devices) {
+  DCHECK(devices);
+  // TODO(swifton): This is a stub implementation.
+  ++on_device_list_response_count_for_testing_;
+}
+
 }  // namespace ash

ash/components/fwupd/firmware_update_manager.h

@@ -6,18 +6,36 @@
 #define ASH_COMPONENTS_FWUPD_FIRMWARE_UPDATE_MANAGER_H_
 
 #include "base/component_export.h"
+#include "chromeos/dbus/fwupd/fwupd_client.h"
+#include "chromeos/dbus/fwupd/fwupd_device.h"
+#include "dbus/message.h"
 
 namespace ash {
 // FirmwareUpdateManager contains all logic that runs the firmware update SWA.
-class COMPONENT_EXPORT(ASH_FIRMWARE_UPDATE_MANAGER) FirmwareUpdateManager {
+class COMPONENT_EXPORT(ASH_FIRMWARE_UPDATE_MANAGER) FirmwareUpdateManager
+    : public chromeos::FwupdClient::Observer {
  public:
+  // Query the fwupd DBus client for currently connected devices.
+  void RequestDevices();
+
+  // FwupdClient::Observer:
+  // When the fwupd DBus client gets a response with devices from fwupd,
+  // it calls this function and passes the response.
+  void OnDeviceListResponse(chromeos::FwupdDeviceList* devices) override;
+
   FirmwareUpdateManager();
   FirmwareUpdateManager(const FirmwareUpdateManager&) = delete;
   FirmwareUpdateManager& operator=(const FirmwareUpdateManager&) = delete;
-  ~FirmwareUpdateManager();
+  ~FirmwareUpdateManager() override;
 
   // Gets the global instance pointer.
   static FirmwareUpdateManager* Get();
+
+ protected:
+  friend class FirmwareUpdateManagerTest;
+  // Temporary auxiliary variable for testing.
+  // TODO(swifton): Replace with mock observers.
+  int on_device_list_response_count_for_testing_ = 0;
 };
 }  // namespace ash
 

ash/components/fwupd/firmware_update_manager_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "ash/components/fwupd/firmware_update_manager.h"
 
+#include <memory>
+
+#include "chromeos/dbus/fwupd/fake_fwupd_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace ash {
@@ -15,6 +18,21 @@ class FirmwareUpdateManagerTest : public testing::Test {
   FirmwareUpdateManagerTest& operator=(const FirmwareUpdateManagerTest&) =
       delete;
   ~FirmwareUpdateManagerTest() override = default;
+
+  int GetOnDevicesResponseCallbackCallCountForTesting() {
+    return firmware_update_manager_.on_device_list_response_count_for_testing_;
+  }
+
+  chromeos::FakeFwupdClient dbus_client_;
+  FirmwareUpdateManager firmware_update_manager_;
 };
 
+// TODO(swifton): Rewrite this test with an observer.
+TEST_F(FirmwareUpdateManagerTest, RequestDeviceList) {
+  // FirmwareUpdateManager requests devices when it is created.
+  EXPECT_EQ(1, GetOnDevicesResponseCallbackCallCountForTesting());
+  firmware_update_manager_.RequestDevices();
+  EXPECT_EQ(2, GetOnDevicesResponseCallbackCallCountForTesting());
+}
+
 }  // namespace ash

chromeos/dbus/fwupd/BUILD.gn

@@ -19,6 +19,8 @@ component("fwupd") {
     "fake_fwupd_client.h",
     "fwupd_client.cc",
     "fwupd_client.h",
+    "fwupd_device.cc",
+    "fwupd_device.h",
   ]
 }
 

chromeos/dbus/fwupd/fake_fwupd_client.cc

@@ -4,6 +4,8 @@
 
 #include "chromeos/dbus/fwupd/fake_fwupd_client.h"
 
+#include "chromeos/dbus/fwupd/fwupd_device.h"
+
 #include <string>
 
 namespace chromeos {
@@ -12,6 +14,12 @@ FakeFwupdClient::FakeFwupdClient() = default;
 FakeFwupdClient::~FakeFwupdClient() = default;
 void FakeFwupdClient::Init(dbus::Bus* bus) {}
 void FakeFwupdClient::GetUpgrades(std::string device_id) {}
-void FakeFwupdClient::GetDevices() {}
+
+void FakeFwupdClient::GetDevices() {
+  // TODO(swifton): This is a stub.
+  auto devices = std::make_unique<FwupdDeviceList>();
+  for (auto& observer : observers_)
+    observer.OnDeviceListResponse(devices.get());
+}
 
 }  // namespace chromeos

chromeos/dbus/fwupd/fwupd_client.cc

@@ -91,9 +91,12 @@ class FwupdClientImpl : public FwupdClient {
       return;
     }
 
-    // TODO(swifton): This is a stub implementation. Replace this with a
-    // callback call for FirmwareUpdateHandler when it's implemented.
-    ++get_devices_callback_call_count_for_testing_;
+    // TODO(swifton): Get the devices from the response. This just sends an
+    // empty list right now.
+    auto devices = std::make_unique<FwupdDeviceList>();
+
+    for (auto& observer : observers_)
+      observer.OnDeviceListResponse(devices.get());
   }
 
   void OnSignalConnected(const std::string& interface_name,
@@ -119,16 +122,28 @@ class FwupdClientImpl : public FwupdClient {
   base::WeakPtrFactory<FwupdClientImpl> weak_ptr_factory_{this};
 };
 
-FwupdClientImpl::FwupdClientImpl() {
+void FwupdClient::AddObserver(FwupdClient::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void FwupdClient::RemoveObserver(FwupdClient::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+FwupdClient::FwupdClient() {
   DCHECK(!g_instance);
   g_instance = this;
 }
 
-FwupdClientImpl::~FwupdClientImpl() {
+FwupdClient::~FwupdClient() {
   DCHECK_EQ(g_instance, this);
   g_instance = nullptr;
 }
 
+FwupdClientImpl::FwupdClientImpl() = default;
+
+FwupdClientImpl::~FwupdClientImpl() = default;
+
 // static
 std::unique_ptr<FwupdClient> FwupdClient::Create() {
   return std::make_unique<FwupdClientImpl>();

chromeos/dbus/fwupd/fwupd_client.h

@@ -8,17 +8,29 @@
 #include <memory>
 
 #include "base/component_export.h"
+#include "base/observer_list.h"
 #include "chromeos/dbus/dbus_client.h"
+#include "chromeos/dbus/fwupd/fwupd_device.h"
+#include "dbus/message.h"
 
 namespace chromeos {
 // FwupdClient is used for handling signals from the fwupd daemon.
 class COMPONENT_EXPORT(CHROMEOS_DBUS_FUWPD) FwupdClient : public DBusClient {
  public:
+  class Observer : public base::CheckedObserver {
+   public:
+    ~Observer() override = default;
+    virtual void OnDeviceListResponse(FwupdDeviceList* devices) = 0;
+  };
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
   // Create() should be used instead.
-  FwupdClient() = default;
+  FwupdClient();
   FwupdClient(const FwupdClient&) = delete;
   FwupdClient& operator=(const FwupdClient&) = delete;
-  ~FwupdClient() override = default;
+  ~FwupdClient() override;
 
   // Factory function.
   static std::unique_ptr<FwupdClient> Create();
@@ -40,7 +52,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_FUWPD) FwupdClient : public DBusClient {
   bool client_is_in_testing_mode_ = false;
   int device_signal_call_count_for_testing_ = 0;
   int get_upgrades_callback_call_count_for_testing_ = 0;
-  int get_devices_callback_call_count_for_testing_ = 0;
+
+  base::ObserverList<Observer> observers_;
 };
 }  // namespace chromeos
 

chromeos/dbus/fwupd/fwupd_client_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "dbus/message.h"
 #include "dbus/mock_bus.h"
 #include "dbus/mock_object_proxy.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -71,10 +72,6 @@ class FwupdClientTest : public testing::Test {
     return fwupd_client_->get_upgrades_callback_call_count_for_testing_;
   }
 
-  int GetGetDevicesCallbackCallCount() {
-    return fwupd_client_->get_devices_callback_call_count_for_testing_;
-  }
-
   void OnMethodCalled(dbus::MethodCall* method_call,
                       int timeout_ms,
                       dbus::ObjectProxy::ResponseOrErrorCallback* callback) {
@@ -141,6 +138,14 @@ class FwupdClientTest : public testing::Test {
   std::deque<MethodCallResult> dbus_method_call_simulated_results_;
 };
 
+class MockObserver : public FwupdClient::Observer {
+ public:
+  MOCK_METHOD(void,
+              OnDeviceListResponse,
+              (chromeos::FwupdDeviceList * devices),
+              ());
+};
+
 // TODO (swifton): Rewrite this test with an observer when it's available.
 TEST_F(FwupdClientTest, AddOneDevice) {
   EmitSignal(kFwupdDeviceAddedSignalName);
@@ -162,8 +167,11 @@ TEST_F(FwupdClientTest, RequestUpgrades) {
   EXPECT_EQ(1, GetGetUpgradesCallbackCallCount());
 }
 
-// TODO (swifton): Rewrite this test with an observer when it's available.
 TEST_F(FwupdClientTest, RequestDevices) {
+  MockObserver observer;
+  EXPECT_CALL(observer, OnDeviceListResponse(_)).Times(1);
+  fwupd_client_->AddObserver(&observer);
+
   EXPECT_CALL(*proxy_, DoCallMethodWithErrorResponse(_, _, _))
       .WillRepeatedly(Invoke(this, &FwupdClientTest::OnMethodCalled));
 
@@ -173,8 +181,6 @@ TEST_F(FwupdClientTest, RequestDevices) {
   fwupd_client_->GetDevices();
 
   base::RunLoop().RunUntilIdle();
-
-  EXPECT_EQ(1, GetGetDevicesCallbackCallCount());
 }
 
 }  // namespace chromeos

chromeos/dbus/fwupd/fwupd_device.cc

@@ -0,0 +1,19 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/dbus/fwupd/fwupd_device.h"
+
+namespace chromeos {
+
+FwupdDevice::FwupdDevice() = default;
+
+FwupdDevice::FwupdDevice(const std::string& id,
+                         const std::u16string& device_name)
+    : id(id), device_name(device_name) {}
+
+FwupdDevice::FwupdDevice(const FwupdDevice& other) = default;
+
+FwupdDevice::~FwupdDevice() = default;
+
+}  // namespace chromeos

chromeos/dbus/fwupd/fwupd_device.h

@@ -0,0 +1,29 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_
+#define CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_
+
+#include <string>
+
+#include "base/component_export.h"
+
+namespace chromeos {
+
+// Structure to hold FwupdDevice data received from fwupd.
+struct COMPONENT_EXPORT(CHROMEOS_DBUS_FWUPD) FwupdDevice {
+  FwupdDevice();
+  FwupdDevice(const std::string& id, const std::u16string& device_name);
+  FwupdDevice(const FwupdDevice& other);
+  ~FwupdDevice();
+
+  std::string id;
+  std::u16string device_name;
+};
+
+typedef std::vector<FwupdDevice> FwupdDeviceList;
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_