Refactor constructor of AssistantManagerService

Pass a Service pointer with Getters instead of pointers for each interface

Bug: None
Test: Manual Test
Change-Id: I4b32aabbabda94b74f699cb7093708ead1bc6fa6
Reviewed-on: https://chromium-review.googlesource.com/1123371
Commit-Queue: Yue Li <updowndota@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572369}

chromeos/services/assistant/assistant_manager_service.h

@@ -47,10 +47,6 @@ class AssistantManagerService : public mojom::Assistant {
   // Returns a pointer of AssistantSettingsManager.
   virtual AssistantSettingsManager* GetAssistantSettingsManager() = 0;
 
-  // Sets assistant controller.
-  virtual void SetAssistantController(
-      ash::mojom::AssistantController* controller) = 0;
-
   using GetSettingsUiResponseCallback =
       base::OnceCallback<void(const std::string&)>;
   // Send request for getting settings ui.

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -42,17 +42,15 @@ const char kBluetoothDeviceSettingId[] = "BLUETOOTH";
 AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     service_manager::Connector* connector,
     device::mojom::BatteryMonitorPtr battery_monitor,
-    mojom::Client* client,
-    mojom::DeviceActionsPtr device_actions)
+    Service* service)
     : platform_api_(CreateLibAssistantConfig(),
                     connector,
                     std::move(battery_monitor)),
-      device_actions_(std::move(device_actions)),
       action_module_(std::make_unique<action::CrosActionModule>(this)),
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       voice_interaction_observer_binding_(this),
-      assistant_client_(client),
+      service_(service),
       background_thread_("background thread"),
       weak_factory_(this) {
   background_thread_.Start();
@@ -186,21 +184,16 @@ void AssistantManagerServiceImpl::RequestScreenContext(
       base::BindOnce(
           &AssistantManagerServiceImpl::SendContextQueryAndRunCallback,
           weak_factory_.GetWeakPtr(), std::move(callback)));
-  assistant_client_->RequestAssistantStructure(
+  service_->client()->RequestAssistantStructure(
       base::BindOnce(&AssistantManagerServiceImpl::OnAssistantStructureReceived,
                      weak_factory_.GetWeakPtr(), on_done));
   // TODO(muyuanli): handle metalayer and grab only part of the screen.
-  assistant_controller_->RequestScreenshot(
+  service_->assistant_controller()->RequestScreenshot(
       region, base::BindOnce(
                   &AssistantManagerServiceImpl::OnAssistantScreenshotReceived,
                   weak_factory_.GetWeakPtr(), on_done));
 }
 
-void AssistantManagerServiceImpl::SetAssistantController(
-    ash::mojom::AssistantController* controller) {
-  assistant_controller_ = controller;
-}
-
 void AssistantManagerServiceImpl::StartVoiceInteraction() {
   assistant_manager_->StartAssistantInteraction();
 }
@@ -305,10 +298,10 @@ void AssistantManagerServiceImpl::OnModifySettingsAction(
   if (modify_setting_args.setting_id() == kWiFiDeviceSettingId) {
     switch (modify_setting_args.change()) {
       case api::client_op::ModifySettingArgs_Change_ON:
-        device_actions_->SetWifiEnabled(true);
+        service_->device_actions()->SetWifiEnabled(true);
         return;
       case api::client_op::ModifySettingArgs_Change_OFF:
-        device_actions_->SetWifiEnabled(false);
+        service_->device_actions()->SetWifiEnabled(false);
         return;
 
       case api::client_op::ModifySettingArgs_Change_TOGGLE:

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -43,6 +43,8 @@ class Connector;
 namespace chromeos {
 namespace assistant {
 
+class Service;
+
 // Implementation of AssistantManagerService based on LibAssistant.
 // This is the main class that ineracts with LibAssistant.
 // Since LibAssistant is a standalone library, all callbacks come from it
@@ -57,10 +59,10 @@ class AssistantManagerServiceImpl
       public ash::mojom::VoiceInteractionObserver,
       public assistant_client::DeviceStateListener {
  public:
+  // |service| owns this class and must outlive this class.
   AssistantManagerServiceImpl(service_manager::Connector* connector,
                               device::mojom::BatteryMonitorPtr battery_monitor,
-                              mojom::Client* client,
-                              mojom::DeviceActionsPtr device_actions);
+                              Service* service);
 
   ~AssistantManagerServiceImpl() override;
 
@@ -77,8 +79,6 @@ class AssistantManagerServiceImpl
   void SendUpdateSettingsUiRequest(
       const std::string& update,
       UpdateSettingsUiResponseCallback callback) override;
-  void SetAssistantController(
-      ash::mojom::AssistantController* controller) override;
 
   // mojom::Assistant overrides:
   void StartVoiceInteraction() override;
@@ -183,7 +183,7 @@ class AssistantManagerServiceImpl
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
   std::unique_ptr<assistant_client::AssistantManager> assistant_manager_;
   std::unique_ptr<AssistantSettingsManagerImpl> assistant_settings_manager_;
-  mojom::DeviceActionsPtr device_actions_;
+  // same ownership as assistant_manager_.
   assistant_client::AssistantManagerInternal* assistant_manager_internal_;
   std::unique_ptr<CrosDisplayConnection> display_connection_;
   mojo::InterfacePtrSet<mojom::AssistantEventSubscriber> subscribers_;
@@ -191,8 +191,7 @@ class AssistantManagerServiceImpl
   mojo::Binding<ash::mojom::VoiceInteractionObserver>
       voice_interaction_observer_binding_;
 
-  ash::mojom::AssistantController* assistant_controller_;
-  mojom::Client* assistant_client_;
+  Service* service_;  // unowned.
 
   bool assistant_enabled_ = false;
   bool context_enabled_ = false;

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -55,8 +55,5 @@ void FakeAssistantManagerServiceImpl::SendTextQuery(const std::string& query) {}
 void FakeAssistantManagerServiceImpl::AddAssistantEventSubscriber(
     mojom::AssistantEventSubscriberPtr subscriber) {}
 
-void FakeAssistantManagerServiceImpl::SetAssistantController(
-    ash::mojom::AssistantController* controller) {}
-
 }  // namespace assistant
 }  // namespace chromeos

chromeos/services/assistant/fake_assistant_manager_service_impl.h

@@ -36,8 +36,6 @@ class FakeAssistantManagerServiceImpl : public AssistantManagerService {
   void SendUpdateSettingsUiRequest(
       const std::string& update,
       UpdateSettingsUiResponseCallback callback) override;
-  void SetAssistantController(
-      ash::mojom::AssistantController* controller) override;
 
   // mojom::Assistant overrides:
   void StartVoiceInteraction() override;

chromeos/services/assistant/service.cc

@@ -162,13 +162,13 @@ void Service::RetryRefreshToken() {
 void Service::Init(mojom::ClientPtr client,
                    mojom::DeviceActionsPtr device_actions) {
   client_ = std::move(client);
+  device_actions_ = std::move(device_actions);
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   device::mojom::BatteryMonitorPtr battery_monitor;
   context()->connector()->BindInterface(device::mojom::kServiceName,
                                         mojo::MakeRequest(&battery_monitor));
   assistant_manager_service_ = std::make_unique<AssistantManagerServiceImpl>(
-      context()->connector(), std::move(battery_monitor), client_.get(),
-      std::move(device_actions));
+      context()->connector(), std::move(battery_monitor), this);
 #else
   assistant_manager_service_ =
       std::make_unique<FakeAssistantManagerServiceImpl>();
@@ -246,8 +246,6 @@ void Service::FinalizeAssistantManagerService() {
 
   assistant_settings_manager_ =
       assistant_manager_service_.get()->GetAssistantSettingsManager();
-  assistant_manager_service_->SetAssistantController(
-      assistant_controller_.get());
   registry_.AddInterface<mojom::AssistantSettingsManager>(base::BindRepeating(
       &Service::BindAssistantSettingsManager, base::Unretained(this)));
 

chromeos/services/assistant/service.h

@@ -46,6 +46,12 @@ class Service : public service_manager::Service,
   Service();
   ~Service() override;
 
+  mojom::Client* client() { return client_.get(); }
+  mojom::DeviceActions* device_actions() { return device_actions_.get(); }
+  ash::mojom::AssistantController* assistant_controller() {
+    return assistant_controller_.get();
+  }
+
   void SetIdentityManagerForTesting(
       identity::mojom::IdentityManagerPtr identity_manager);
 
@@ -105,6 +111,7 @@ class Service : public service_manager::Service,
   mojo::Binding<ash::mojom::SessionActivationObserver>
       session_observer_binding_;
   mojom::ClientPtr client_;
+  mojom::DeviceActionsPtr device_actions_;
 
   identity::mojom::IdentityManagerPtr identity_manager_;