Cleanup handling of |device.MODIFY_SETTING| requests

Previously this was all done in the |AssistantManagerServiceImpl|,
but that class is already huge.

To solve this I made the following changes:
   * split it out in a separate |AssistantDeviceSettingsDelegate| class.
   * Introduce helper |Setting| classes, 1 for each supported setting.
   * Add unittests for each of the supported settings.

Bug: N/A
Change-Id: Ia32aecb15e3200a183c72ad8e5bcfcd3b2b4dfbc
Tests: new |chromeos_unittests| with filter |AssistantDeviceSettingsDelegateTest.*|
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2147693
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761041}

chromeos/services/assistant/BUILD.gn

@@ -60,6 +60,8 @@ component("lib") {
 
   if (enable_cros_libassistant) {
     sources += [
+      "assistant_device_settings_delegate.cc",
+      "assistant_device_settings_delegate.h",
       "assistant_manager_service_delegate.h",
       "assistant_manager_service_delegate_impl.cc",
       "assistant_manager_service_delegate_impl.h",
@@ -163,6 +165,7 @@ source_set("tests") {
 
   if (enable_cros_libassistant) {
     sources += [
+      "assistant_device_settings_delegate_unittest.cc",
       "assistant_manager_service_impl_unittest.cc",
       "media_session/assistant_media_session_unittest.cc",
       "platform/audio_input_impl_unittest.cc",
@@ -174,6 +177,8 @@ source_set("tests") {
       "test_support/fake_assistant_manager_service_delegate.h",
       "test_support/fake_platform_api.cc",
       "test_support/fake_platform_api.h",
+      "test_support/fake_service_context.cc",
+      "test_support/fake_service_context.h",
       "test_support/mock_assistant_interaction_subscriber.cc",
       "test_support/mock_assistant_interaction_subscriber.h",
       "test_support/mock_media_manager.cc",

chromeos/services/assistant/assistant_device_settings_delegate.h

+// Copyright 2020 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_SERVICES_ASSISTANT_ASSISTANT_DEVICE_SETTINGS_DELEGATE_H_
+#define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_DEVICE_SETTINGS_DELEGATE_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "ash/public/mojom/assistant_controller.mojom-forward.h"
+#include "chromeos/services/assistant/public/mojom/assistant.mojom-forward.h"
+
+namespace assistant_client {
+class VolumeControl;
+}  // namespace assistant_client
+
+namespace assistant {
+namespace api {
+namespace client_op {
+class ModifySettingArgs;
+class GetDeviceSettingsArgs;
+}  // namespace client_op
+}  // namespace api
+}  // namespace assistant
+
+namespace chromeos {
+namespace assistant {
+
+struct DeviceSetting;
+class ServiceContext;
+class Setting;
+
+// Delegate that handles Assistant actions related to retrieving/modifying
+// the device settings, like Bluetooth or WiFi.
+class AssistantDeviceSettingsDelegate {
+ public:
+  explicit AssistantDeviceSettingsDelegate(
+      ServiceContext* context,
+      assistant_client::VolumeControl* volume_control);
+  AssistantDeviceSettingsDelegate(AssistantDeviceSettingsDelegate&) = delete;
+  AssistantDeviceSettingsDelegate& operator=(AssistantDeviceSettingsDelegate&) =
+      delete;
+  ~AssistantDeviceSettingsDelegate();
+
+  bool IsSettingSupported(const std::string& setting_id) const;
+
+  void HandleModifyDeviceSetting(
+      const ::assistant::api::client_op::ModifySettingArgs& args);
+
+  // Return which of the given device settings are supported or not.
+  std::vector<DeviceSetting> GetDeviceSettings(
+      const ::assistant::api::client_op::GetDeviceSettingsArgs& args) const;
+
+ private:
+  void AddSetting(std::unique_ptr<Setting> setting);
+
+  std::vector<std::unique_ptr<Setting>> settings_;
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_DEVICE_SETTINGS_DELEGATE_H_

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -57,6 +57,7 @@ class AssistantMediaSession;
 class CrosPlatformApi;
 class ServiceContext;
 class AssistantManagerServiceDelegate;
+class AssistantDeviceSettingsDelegate;
 
 // Enumeration of Assistant query response type, also recorded in histograms.
 // These values are persisted to logs. Entries should not be renumbered and
@@ -350,6 +351,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   ServiceContext* const context_;
 
   std::unique_ptr<AssistantManagerServiceDelegate> delegate_;
+  std::unique_ptr<AssistantDeviceSettingsDelegate> settings_delegate_;
 
   bool spoken_feedback_enabled_ = false;
 

chromeos/services/assistant/assistant_manager_service_impl_unittest.cc

@@ -23,6 +23,7 @@
 #include "chromeos/services/assistant/service_context.h"
 #include "chromeos/services/assistant/test_support/fake_assistant_manager_service_delegate.h"
 #include "chromeos/services/assistant/test_support/fake_client.h"
+#include "chromeos/services/assistant/test_support/fake_service_context.h"
 #include "chromeos/services/assistant/test_support/fully_initialized_assistant_state.h"
 #include "chromeos/services/assistant/test_support/mock_assistant_interaction_subscriber.h"
 #include "chromeos/services/assistant/test_support/mock_media_manager.h"
@@ -46,7 +47,6 @@ using UserInfo = AssistantManagerService::UserInfo;
 
 namespace {
 
-const char* kGaiaId = "<fake-gaia-id>";
 const char* kNoValue = FakeAssistantManager::kNoValue;
 
 // Action CloneArg<k>(pointer) clones the k-th (0-based) argument of the mock
@@ -116,80 +116,6 @@ class FakeAssistantClient : public FakeClient {
   DISALLOW_COPY_AND_ASSIGN(FakeAssistantClient);
 };
 
-class FakeServiceContext : public ServiceContext {
- public:
-  FakeServiceContext(
-      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
-      ash::AssistantState* assistant_state,
-      PowerManagerClient* power_manager_client)
-      : main_task_runner_(main_task_runner),
-        assistant_state_(assistant_state),
-        power_manager_client_(power_manager_client) {}
-  ~FakeServiceContext() override = default;
-
-  void set_assistant_alarm_timer_controller(
-      ash::mojom::AssistantAlarmTimerController*
-          assistant_alarm_timer_controller) {
-    assistant_alarm_timer_controller_ = assistant_alarm_timer_controller;
-  }
-
-  ash::mojom::AssistantAlarmTimerController* assistant_alarm_timer_controller()
-      override {
-    return assistant_alarm_timer_controller_;
-  }
-
-  mojom::AssistantController* assistant_controller() override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  ash::mojom::AssistantNotificationController*
-  assistant_notification_controller() override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  ash::mojom::AssistantScreenContextController*
-  assistant_screen_context_controller() override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  ash::AssistantStateBase* assistant_state() override {
-    return assistant_state_;
-  }
-
-  CrasAudioHandler* cras_audio_handler() override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  mojom::DeviceActions* device_actions() override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  scoped_refptr<base::SequencedTaskRunner> main_task_runner() override {
-    return main_task_runner_;
-  }
-
-  PowerManagerClient* power_manager_client() override {
-    return power_manager_client_;
-  }
-
-  std::string primary_account_gaia_id() override { return gaia_id; }
-
- private:
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
-  ash::AssistantState* const assistant_state_;
-  PowerManagerClient* const power_manager_client_;
-  std::string gaia_id = kGaiaId;
-  ash::mojom::AssistantAlarmTimerController* assistant_alarm_timer_controller_ =
-      nullptr;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeServiceContext);
-};
-
 class AssistantAlarmTimerControllerMock
     : public ash::mojom::AssistantAlarmTimerController {
  public:
@@ -250,9 +176,11 @@ class AssistantManagerServiceImplTest : public testing::Test {
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             &url_loader_factory_);
 
-    service_context_ = std::make_unique<FakeServiceContext>(
-        task_environment.GetMainThreadTaskRunner(), &assistant_state_,
-        PowerManagerClient::Get());
+    service_context_ = std::make_unique<FakeServiceContext>();
+    service_context_
+        ->set_main_task_runner(task_environment.GetMainThreadTaskRunner())
+        .set_power_manager_client(PowerManagerClient::Get())
+        .set_assistant_state(&assistant_state_);
 
     CreateAssistantManagerServiceImpl(/*libassistant_config=*/{});
   }

chromeos/services/assistant/test_support/fake_platform_api.cc

@@ -2,12 +2,71 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/services/assistant/test_support/fake_platform_api.h"
+#include <vector>
+
 #include "base/logging.h"
+#include "chromeos/services/assistant/test_support/fake_platform_api.h"
+#include "libassistant/shared/public/platform_audio_output.h"
 
 namespace chromeos {
 namespace assistant {
 
+namespace {
+
+class FakeVolumeControl : public assistant_client::VolumeControl {
+ public:
+  // assistant_client::VolumeControl implementation:
+  void SetAudioFocus(assistant_client::OutputStreamType) override {}
+  float GetSystemVolume() override { return 0.5; }
+  void SetSystemVolume(float new_volume, bool user_initiated) override {}
+  float GetAlarmVolume() override { return 0.5; }
+  void SetAlarmVolume(float new_volume, bool user_initiated) override {}
+  bool IsSystemMuted() override { return false; }
+  void SetSystemMuted(bool muted) override {}
+};
+
+class FakeAudioOutputProvider : public assistant_client::AudioOutputProvider {
+ public:
+  FakeAudioOutputProvider() = default;
+  FakeAudioOutputProvider(FakeAudioOutputProvider&) = delete;
+  FakeAudioOutputProvider& operator=(FakeAudioOutputProvider&) = delete;
+  ~FakeAudioOutputProvider() override = default;
+
+  // assistant_client::AudioOutputProvider implementation:
+  assistant_client::AudioOutput* CreateAudioOutput(
+      assistant_client::OutputStreamType type,
+      const assistant_client::OutputStreamFormat& stream_format) override {
+    NOTIMPLEMENTED();
+    abort();
+  }
+
+  std::vector<assistant_client::OutputStreamEncoding>
+  GetSupportedStreamEncodings() override {
+    return {};
+  }
+
+  assistant_client::AudioInput* GetReferenceInput() override {
+    NOTIMPLEMENTED();
+    abort();
+  }
+
+  bool SupportsPlaybackTimestamp() const override { return false; }
+
+  assistant_client::VolumeControl& GetVolumeControl() override {
+    return volume_control_;
+  }
+
+ private:
+  FakeVolumeControl volume_control_;
+};
+
+}  // namespace
+
+FakePlatformApi::FakePlatformApi()
+    : audio_output_provider_(std::make_unique<FakeAudioOutputProvider>()) {}
+
+FakePlatformApi::~FakePlatformApi() = default;
+
 assistant_client::AudioInputProvider& FakePlatformApi::GetAudioInputProvider() {
   NOTIMPLEMENTED();
   abort();
@@ -15,8 +74,7 @@ assistant_client::AudioInputProvider& FakePlatformApi::GetAudioInputProvider() {
 
 assistant_client::AudioOutputProvider&
 FakePlatformApi::GetAudioOutputProvider() {
-  NOTIMPLEMENTED();
-  abort();
+  return *audio_output_provider_.get();
 }
 
 assistant_client::AuthProvider& FakePlatformApi::GetAuthProvider() {

chromeos/services/assistant/test_support/fake_platform_api.h

@@ -5,6 +5,8 @@
 #ifndef CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_PLATFORM_API_H_
 #define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_PLATFORM_API_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "chromeos/services/assistant/cros_platform_api.h"
 
@@ -12,12 +14,12 @@ namespace chromeos {
 namespace assistant {
 
 // Fake implementation of the |CrosPlatformApi| used during the unittests.
-// As of now the |assistant_client::PlatformApi| methods are not implemented
-// and will assert when called.
+// As of now most of the |assistant_client::PlatformApi| methods are not
+// implemented and will assert when called.
 class FakePlatformApi : public CrosPlatformApi {
  public:
-  FakePlatformApi() = default;
-  ~FakePlatformApi() override = default;
+  FakePlatformApi();
+  ~FakePlatformApi() override;
 
   // CrosPlatformApi overrides
   assistant_client::AudioInputProvider& GetAudioInputProvider() override;
@@ -33,6 +35,8 @@ class FakePlatformApi : public CrosPlatformApi {
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FakePlatformApi);
+
+  std::unique_ptr<assistant_client::AudioOutputProvider> audio_output_provider_;
 };
 
 }  // namespace assistant

chromeos/services/assistant/test_support/fake_service_context.cc

+// Copyright 2020 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 <string>
+
+#include "chromeos/services/assistant/test_support/fake_service_context.h"
+
+namespace chromeos {
+namespace assistant {
+
+/*static*/
+constexpr const char* FakeServiceContext::kGaiaId;
+
+FakeServiceContext::FakeServiceContext() = default;
+
+FakeServiceContext::~FakeServiceContext() = default;
+
+FakeServiceContext& FakeServiceContext::set_assistant_alarm_timer_controller(
+    ash::mojom::AssistantAlarmTimerController* value) {
+  assistant_alarm_timer_controller_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_main_task_runner(
+    scoped_refptr<base::SingleThreadTaskRunner> value) {
+  main_task_runner_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_power_manager_client(
+    PowerManagerClient* value) {
+  power_manager_client_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_primary_account_gaia_id(
+    std::string value) {
+  gaia_id_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_assistant_state(
+    ash::AssistantStateBase* value) {
+  assistant_state_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_device_actions(
+    mojom::DeviceActions* value) {
+  device_actions_ = value;
+  return *this;
+}
+
+FakeServiceContext& FakeServiceContext::set_assistant_notification_controller(
+    ash::mojom::AssistantNotificationController* value) {
+  assistant_notification_controller_ = value;
+  return *this;
+}
+
+ash::mojom::AssistantAlarmTimerController*
+FakeServiceContext::assistant_alarm_timer_controller() {
+  DCHECK(assistant_alarm_timer_controller_ != nullptr);
+  return assistant_alarm_timer_controller_;
+}
+
+mojom::AssistantController* FakeServiceContext::assistant_controller() {
+  NOTIMPLEMENTED();
+  return nullptr;
+}
+
+ash::mojom::AssistantNotificationController*
+FakeServiceContext::assistant_notification_controller() {
+  DCHECK(assistant_notification_controller_ != nullptr);
+  return assistant_notification_controller_;
+}
+
+ash::mojom::AssistantScreenContextController*
+FakeServiceContext::assistant_screen_context_controller() {
+  NOTIMPLEMENTED();
+  return nullptr;
+}
+
+ash::AssistantStateBase* FakeServiceContext::assistant_state() {
+  DCHECK(assistant_state_ != nullptr);
+  return assistant_state_;
+}
+
+CrasAudioHandler* FakeServiceContext::cras_audio_handler() {
+  NOTIMPLEMENTED();
+  return nullptr;
+}
+
+mojom::DeviceActions* FakeServiceContext::device_actions() {
+  DCHECK(device_actions_ != nullptr);
+  return device_actions_;
+}
+
+scoped_refptr<base::SequencedTaskRunner>
+FakeServiceContext::main_task_runner() {
+  DCHECK(main_task_runner_ != nullptr);
+  return main_task_runner_;
+}
+
+PowerManagerClient* FakeServiceContext::power_manager_client() {
+  DCHECK(power_manager_client_ != nullptr);
+  return power_manager_client_;
+}
+
+std::string FakeServiceContext::primary_account_gaia_id() {
+  return gaia_id_;
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/fake_service_context.h

+// Copyright 2020 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_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_SERVICE_CONTEXT_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_SERVICE_CONTEXT_H_
+
+#include <string>
+
+#include "base/single_thread_task_runner.h"
+#include "chromeos/services/assistant/service_context.h"
+
+namespace chromeos {
+namespace assistant {
+
+// Fake implementation of the |ServiceContext| used during the unittests.
+// Every method will assert when called,
+// unless you've provided the object using one of the setter methods.
+class FakeServiceContext : public ServiceContext {
+ public:
+  // Gaia ID returned by primary_account_gaia_id() (unless overridden).
+  static constexpr const char* kGaiaId = "<fake-gaia-id>";
+
+  FakeServiceContext();
+  FakeServiceContext(const FakeServiceContext&) = delete;
+  FakeServiceContext& operator=(const FakeServiceContext&) = delete;
+  ~FakeServiceContext() override;
+
+  FakeServiceContext& set_assistant_alarm_timer_controller(
+      ash::mojom::AssistantAlarmTimerController*);
+  FakeServiceContext& set_main_task_runner(
+      scoped_refptr<base::SingleThreadTaskRunner>);
+  FakeServiceContext& set_power_manager_client(PowerManagerClient*);
+  FakeServiceContext& set_primary_account_gaia_id(std::string);
+  FakeServiceContext& set_assistant_state(ash::AssistantStateBase*);
+  FakeServiceContext& set_device_actions(mojom::DeviceActions*);
+  FakeServiceContext& set_assistant_notification_controller(
+      ash::mojom::AssistantNotificationController*);
+
+  // ServiceContext implementation:
+  ash::mojom::AssistantAlarmTimerController* assistant_alarm_timer_controller()
+      override;
+  mojom::AssistantController* assistant_controller() override;
+  ash::mojom::AssistantNotificationController*
+  assistant_notification_controller() override;
+  ash::mojom::AssistantScreenContextController*
+  assistant_screen_context_controller() override;
+  ash::AssistantStateBase* assistant_state() override;
+  CrasAudioHandler* cras_audio_handler() override;
+  mojom::DeviceActions* device_actions() override;
+  scoped_refptr<base::SequencedTaskRunner> main_task_runner() override;
+  PowerManagerClient* power_manager_client() override;
+  std::string primary_account_gaia_id() override;
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  ash::AssistantStateBase* assistant_state_ = nullptr;
+  PowerManagerClient* power_manager_client_ = nullptr;
+  std::string gaia_id_ = kGaiaId;
+  ash::mojom::AssistantAlarmTimerController* assistant_alarm_timer_controller_ =
+      nullptr;
+  mojom::DeviceActions* device_actions_ = nullptr;
+  ash::mojom::AssistantNotificationController*
+      assistant_notification_controller_ = nullptr;
+};
+}  // namespace assistant
+}  // namespace chromeos
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_SERVICE_CONTEXT_H_