Separate ServiceController Initialize() and Start()

A lot of things are configured between the creation of the
|AssistantManager| and the call of |AssistantManager::Start()|.

Right now this is done through an initialize callback (which circumvents
the Mojom API).
To get rid of this initialize callback (and configure things properly
through Mojom APIs), we need to separate the creation and start of
|AssistantManager| in 2 separate mojom calls.

This CL also renames all |ServiceController| tests to
|AssistantServiceControllerTest| so we can run all assistant tests using
the "Assistant*" test filter.

Bug: b/176851446
Test: chromeos_unittests --gtest_filter="Assistant*"
Change-Id: I37c11439446f52fcc209d0612fd73cf8b0478de0
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617119Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842124}

chromeos/services/assistant/proxy/service_controller_proxy.cc

@@ -159,7 +159,8 @@ void ServiceControllerProxy::Start(
       assistant::features::IsMediaSessionIntegrationEnabled());
 
   // The mojom service will create the |AssistantManager|.
-  service_controller_remote_->Start(libassistant_config);
+  service_controller_remote_->Initialize(libassistant_config);
+  service_controller_remote_->Start();
 
   // We need to initialize the |AssistantManager| once it's created and before
   // it's started, so we register a callback to do just that.

chromeos/services/assistant/test_support/fake_service_controller.cc

@@ -49,9 +49,11 @@ void FakeServiceController::UnblockStartCalls() {
   start_mutex_.unlock();
 }
 
-void FakeServiceController::Start(const std::string& libassistant_config) {
+void FakeServiceController::Initialize(const std::string& libassistant_config) {
   libassistant_config_ = libassistant_config;
+}
 
+void FakeServiceController::Start() {
   // Will block if |BlockStartCalls| was invoked.
   std::lock_guard<std::mutex> lock(start_mutex_);
 

chromeos/services/assistant/test_support/fake_service_controller.h

@@ -40,7 +40,7 @@ class FakeServiceController : public libassistant::mojom::ServiceController {
   void SetState(State new_state);
   State state() const { return state_; }
 
-  // Returns the Libassistant config that was passed to the last Start() call.
+  // Returns the Libassistant config that was passed to Initialize().
   std::string libassistant_config() { return libassistant_config_; }
 
   void Bind(mojo::PendingReceiver<libassistant::mojom::ServiceController>
@@ -57,7 +57,8 @@ class FakeServiceController : public libassistant::mojom::ServiceController {
   void UnblockStartCalls();
 
   // mojom::ServiceController implementation:
-  void Start(const std::string& libassistant_config) override;
+  void Initialize(const std::string& libassistant_config) override;
+  void Start() override;
   void Stop() override;
   void AddAndFireStateObserver(
       mojo::PendingRemote<libassistant::mojom::StateObserver> pending_observer)

chromeos/services/libassistant/assistant_manager_observer.h

@@ -25,12 +25,19 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) AssistantManagerObserver
   ~AssistantManagerObserver() override = default;
 
   // Called when the |AssistantManager| and |AssistantManagerInternal| have
-  // been created. The pointers are guaranteed to remain valid until after
+  // been created, but not started yet.
+  // The pointers are guaranteed to remain valid until after
   // OnDestroyingAssistantManager() is called.
   virtual void OnAssistantManagerCreated(
       assistant_client::AssistantManager* assistant_manager,
-      assistant_client::AssistantManagerInternal*
-          assistant_manager_internal) = 0;
+      assistant_client::AssistantManagerInternal* assistant_manager_internal) {}
+
+  // Called when Start() has been called on the |AssistantManager|.
+  // The pointers are guaranteed to remain valid until after
+  // OnDestroyingAssistantManager() is called.
+  virtual void OnAssistantManagerStarted(
+      assistant_client::AssistantManager* assistant_manager,
+      assistant_client::AssistantManagerInternal* assistant_manager_internal) {}
 
   // Called just before the |AssistantManager| and |AssistantManagerInternal|
   // will be destroyed. They should not be used anymore after this has been
@@ -39,8 +46,7 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) AssistantManagerObserver
   // for the implementer's convenience).
   virtual void OnDestroyingAssistantManager(
       assistant_client::AssistantManager* assistant_manager,
-      assistant_client::AssistantManagerInternal*
-          assistant_manager_internal) = 0;
+      assistant_client::AssistantManagerInternal* assistant_manager_internal) {}
 };
 
 }  // namespace libassistant

chromeos/services/libassistant/public/mojom/service_controller.mojom

@@ -16,13 +16,18 @@ enum ServiceState {
 // Interface managing the lifecycle of Libassistant,
 // exposing methods to start/stop and configure Libassistant.
 interface ServiceController {
-  // Start the service. Can be called multiple times, and will be a noop if 
+
+  // Initialize the service. This must be called before Start() and before
+  // restarting the service (so between all Stop() and Start() calls).
+  // Will be a noop if the service is started or running.
+  // Note that calling Initialize() will not cause any change in the service
+  // state, as the service will remain in state |kStopped| until Start() is
+  // called.
+  Initialize(string libassistant_config);
+
+  // Start the service. Can be called multiple times, and will be a noop if
   // the service is already started or running.
-  // TODO(jeroendh): I feel the libassistant config is something internal to
-  // the mojom service, so we should not pass that in here. Instead, have
-  // setters on the |ServiceController| for the  fields that are non-static
-  // like the s3 server URI.
-  Start(string libassistant_config);
+  Start();
 
   // Stop the service. Will be a noop if the service is already stopped.
   Stop();

chromeos/services/libassistant/service_controller.cc

@@ -36,16 +36,28 @@ void ServiceController::SetInitializeCallback(InitializeCallback callback) {
   initialize_callback_ = std::move(callback);
 }
 
-void ServiceController::Start(const std::string& libassistant_config) {
-  if (state_ != ServiceState::kStopped)
+void ServiceController::Initialize(const std::string& libassistant_config) {
+  if (assistant_manager_ != nullptr) {
+    LOG(ERROR) << "Initialize() should only be called once.";
     return;
+  }
 
   assistant_manager_ =
       delegate_->CreateAssistantManager(platform_api_, libassistant_config);
   assistant_manager_internal_ =
       delegate_->UnwrapAssistantManagerInternal(assistant_manager_.get());
-  libassistant_v1_api_ = std::make_unique<assistant::LibassistantV1Api>(
-      assistant_manager_.get(), assistant_manager_internal_);
+
+  for (auto& observer : assistant_manager_observers_) {
+    observer.OnAssistantManagerCreated(assistant_manager(),
+                                       assistant_manager_internal());
+  }
+}
+
+void ServiceController::Start() {
+  if (state_ != ServiceState::kStopped)
+    return;
+
+  DCHECK(IsInitialized()) << "Initialize() must be called before Start()";
 
   if (initialize_callback_) {
     std::move(initialize_callback_)
@@ -54,10 +66,13 @@ void ServiceController::Start(const std::string& libassistant_config) {
 
   assistant_manager()->Start();
 
+  libassistant_v1_api_ = std::make_unique<assistant::LibassistantV1Api>(
+      assistant_manager_.get(), assistant_manager_internal_);
+
   SetStateAndInformObservers(ServiceState::kStarted);
 
   for (auto& observer : assistant_manager_observers_) {
-    observer.OnAssistantManagerCreated(assistant_manager(),
+    observer.OnAssistantManagerStarted(assistant_manager(),
                                        assistant_manager_internal());
   }
 }
@@ -93,10 +108,14 @@ void ServiceController::AddAndFireAssistantManagerObserver(
 
   assistant_manager_observers_.AddObserver(observer);
 
-  if (IsStarted()) {
+  if (IsInitialized()) {
     observer->OnAssistantManagerCreated(assistant_manager(),
                                         assistant_manager_internal());
   }
+  if (IsStarted()) {
+    observer->OnAssistantManagerStarted(assistant_manager(),
+                                        assistant_manager_internal());
+  }
 }
 
 void ServiceController::RemoveAssistantManagerObserver(
@@ -108,6 +127,10 @@ bool ServiceController::IsStarted() const {
   return state_ != mojom::ServiceState::kStopped;
 }
 
+bool ServiceController::IsInitialized() const {
+  return assistant_manager_ != nullptr;
+}
+
 assistant_client::AssistantManager* ServiceController::assistant_manager() {
   return assistant_manager_.get();
 }

chromeos/services/libassistant/service_controller.h

@@ -62,7 +62,8 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ServiceController
   void SetInitializeCallback(InitializeCallback callback);
 
   // mojom::ServiceController implementation:
-  void Start(const std::string& libassistant_config) override;
+  void Initialize(const std::string& libassistant_config) override;
+  void Start() override;
   void Stop() override;
   void AddAndFireStateObserver(
       mojo::PendingRemote<mojom::StateObserver> observer) override;
@@ -70,6 +71,7 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ServiceController
   void AddAndFireAssistantManagerObserver(AssistantManagerObserver* observer);
   void RemoveAssistantManagerObserver(AssistantManagerObserver* observer);
 
+  bool IsInitialized() const;
   bool IsStarted() const;
 
   // Will return nullptr if the service is stopped.