Always create the |ServiceController|

Previously it was only created when it was bound to a receiver,
which caused complications if other controllers are created first,
because those controllers need access to the Libassistant objects
created by the |ServiceController|.

This CL also removes unused accessor methods.

Bug: b/171748795
Test: chromeos_unittests --gtest_filter="ServiceController*"
Change-Id: I1eed788e51a108b6f2b70929f059daabe7b70d65
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2569847
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835916}

chromeos/services/libassistant/libassistant_service.cc

@@ -18,29 +18,15 @@ LibassistantService::LibassistantService(
     mojo::PendingReceiver<mojom::LibassistantService> receiver,
     assistant_client::PlatformApi* platform_api,
     assistant::AssistantManagerServiceDelegate* delegate)
-    : platform_api_(platform_api),
-      delegate_(delegate),
-      receiver_(this, std::move(receiver)) {}
+    : receiver_(this, std::move(receiver)),
+      service_controller_(
+          std::make_unique<ServiceController>(delegate, platform_api)) {}
 
 LibassistantService::~LibassistantService() = default;
 
-assistant_client::AssistantManager* LibassistantService::assistant_manager() {
-  DCHECK(service_controller_);
-  return service_controller_->assistant_manager();
-}
-
-assistant_client::AssistantManagerInternal*
-LibassistantService::assistant_manager_internal() {
-  DCHECK(service_controller_);
-  return service_controller_->assistant_manager_internal();
-}
-
 void LibassistantService::BindServiceController(
     mojo::PendingReceiver<mojom::ServiceController> receiver) {
-  // Cannot bind the service controller twice.
-  DCHECK(!service_controller_);
-  service_controller_ = std::make_unique<ServiceController>(
-      std::move(receiver), delegate_, platform_api_);
+  service_controller_->Bind(std::move(receiver));
 }
 
 }  // namespace libassistant

chromeos/services/libassistant/libassistant_service.h

@@ -12,8 +12,6 @@
 #include "mojo/public/cpp/bindings/receiver.h"
 
 namespace assistant_client {
-class AssistantManager;
-class AssistantManagerInternal;
 class PlatformApi;
 }  // namespace assistant_client
 
@@ -39,12 +37,6 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) LibassistantService
   LibassistantService& operator=(LibassistantService&) = delete;
   ~LibassistantService() override;
 
-  // Retrieve the |AssistantManager|. The pointer is valid as long as the
-  // |ServiceController| is in state |kStarted| (and this class is not
-  // destroyed).
-  assistant_client::AssistantManager* assistant_manager();
-  assistant_client::AssistantManagerInternal* assistant_manager_internal();
-
  private:
   // mojom::LibassistantService implementation:
   void BindServiceController(
@@ -53,11 +45,6 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) LibassistantService
   void BindAudioOutputController() override {}
   void BindInteractionController() override {}
 
-  // Owned by |AssistantManagerServiceImpl| which indirectly owns us.
-  assistant_client::PlatformApi* const platform_api_;
-  // Owned by |AssistantManagerServiceImpl| which indirectly owns us.
-  assistant::AssistantManagerServiceDelegate* const delegate_;
-
   mojo::Receiver<mojom::LibassistantService> receiver_;
   std::unique_ptr<ServiceController> service_controller_;
 };

chromeos/services/libassistant/service_controller.cc

@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <memory>
-
 #include "chromeos/services/libassistant/service_controller.h"
 
+#include <memory>
+
+#include "base/check.h"
 #include "chromeos/services/assistant/public/cpp/migration/assistant_manager_service_delegate.h"
 #include "chromeos/services/assistant/public/cpp/migration/libassistant_v1_api.h"
 
@@ -15,15 +16,18 @@ namespace libassistant {
 using mojom::ServiceState;
 
 ServiceController::ServiceController(
-    mojo::PendingReceiver<mojom::ServiceController> receiver,
     assistant::AssistantManagerServiceDelegate* delegate,
     assistant_client::PlatformApi* platform_api)
-    : delegate_(delegate),
-      platform_api_(platform_api),
-      receiver_(this, std::move(receiver)) {}
+    : delegate_(delegate), platform_api_(platform_api), receiver_(this) {}
 
 ServiceController::~ServiceController() = default;
 
+void ServiceController::Bind(
+    mojo::PendingReceiver<mojom::ServiceController> receiver) {
+  DCHECK(!receiver_.is_bound());
+  receiver_.Bind(std::move(receiver));
+}
+
 void ServiceController::Start(const std::string& libassistant_config) {
   if (state_ != ServiceState::kStopped)
     return;

chromeos/services/libassistant/service_controller.h

@@ -35,16 +35,19 @@ namespace libassistant {
 
 // Component managing the lifecycle of Libassistant,
 // exposing methods to start/stop and configure Libassistant.
+// Note: to access the Libassistant objects from //chromeos/services/assistant,
+// use the |LibassistantV1Api| singleton, which will be populated by this class.
 class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ServiceController
     : public mojom::ServiceController {
  public:
-  ServiceController(mojo::PendingReceiver<mojom::ServiceController> receiver,
-                    assistant::AssistantManagerServiceDelegate* delegate,
+  ServiceController(assistant::AssistantManagerServiceDelegate* delegate,
                     assistant_client::PlatformApi* platform_api);
   ServiceController(ServiceController&) = delete;
   ServiceController& operator=(ServiceController&) = delete;
   ~ServiceController() override;
 
+  void Bind(mojo::PendingReceiver<mojom::ServiceController> receiver);
+
   // mojom::ServiceController implementation:
   void Start(const std::string& libassistant_config) override;
   void Stop() override;
@@ -61,7 +64,7 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ServiceController
 
   mojom::ServiceState state_ = mojom::ServiceState::kStopped;
 
-  // Owned by |AssistantProxy| which owns us.
+  // Owned by |AssistantManagerServiceImpl| which indirectly owns us.
   assistant::AssistantManagerServiceDelegate* const delegate_;
   // Owned by |AssistantManagerServiceImpl| which indirectly owns us.
   assistant_client::PlatformApi* const platform_api_;

chromeos/services/libassistant/service_controller_unittest.cc

@@ -45,9 +45,10 @@ class StateObserverMock : public mojom::StateObserver {
 class ServiceControllerTest : public testing::Test {
  public:
   ServiceControllerTest()
-      : service_controller_(client_.BindNewPipeAndPassReceiver(),
-                            &delegate_,
-                            /*platform_api=*/nullptr) {}
+      : service_controller_(&delegate_,
+                            /*platform_api=*/nullptr) {
+    service_controller_.Bind(client_.BindNewPipeAndPassReceiver());
+  }
 
   mojo::Remote<mojom::ServiceController>& client() { return client_; }
   ServiceController& service_controller() { return service_controller_; }