Introduce AssistantServiceManager::State::STARTING state.

Until now the |AssistantServiceManager::State| enum did not distinguish
between the state in which libassistant was started (but not ready yet)
and in which it is ready.
That distinction is needed to be able to add tests using libassistant,
as they have to wait until it is ready.

So now there are 4 states:
    - STOPPED: It has not started/it has been stopped (this is the same
    as the old STOPPED state).
    - STARTING: |AssistantServiceManager::Start| has been called and the
    libassistant |AssistantManager::Start| call has not finished yet (this
    is the same as the old STARTED state).
    - STARTED: the libassistant |AssistantManager::Start| call has
    finished, but libassistant has not signalled it is ready yet (this
    is the same as the old RUNNING state).
    - RUNNING: libassistant signalled it is ready (this state is new).

As this CL also introduces the first unittests for
|AssistantServiceManagerImpl|, it also contains a bunch of fake/dummy
classes used for the unittests.

This CL increases test coverage in //chromeos/services/assistant from 28%
to 41%.

Change-Id: If38a6b70e22e6392537b6658e5f39eebf6dd9099
BUG: b/141264108
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1810019
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700410}

chromeos/services/assistant/BUILD.gn

@@ -68,6 +68,9 @@ component("lib") {
 
   if (enable_cros_libassistant) {
     sources += [
+      "assistant_manager_service_delegate.h",
+      "assistant_manager_service_delegate_impl.cc",
+      "assistant_manager_service_delegate_impl.h",
       "assistant_manager_service_impl.cc",
       "assistant_manager_service_impl.h",
       "assistant_settings_manager_impl.cc",
@@ -76,6 +79,7 @@ component("lib") {
       "chromium_api_delegate.h",
       "chromium_http_connection.cc",
       "chromium_http_connection.h",
+      "cros_platform_api.h",
       "media_session/assistant_media_session.cc",
       "media_session/assistant_media_session.h",
       "platform/audio_device_owner.cc",
@@ -161,21 +165,32 @@ source_set("tests") {
     "fake_client.cc",
     "fake_client.h",
     "service_unittest.cc",
+    "test_support/fully_initialized_assistant_state.cc",
+    "test_support/fully_initialized_assistant_state.h",
   ]
 
   if (enable_cros_libassistant) {
     sources += [
+      "assistant_manager_service_impl_unittest.cc",
       "platform/audio_input_impl_unittest.cc",
       "platform/audio_output_provider_impl_unittest.cc",
       "platform/network_provider_impl_unittest.cc",
       "platform/power_manager_provider_impl_unittest.cc",
       "platform/system_provider_impl_unittest.cc",
+      "test_support/fake_assistant_manager_service_delegate.cc",
+      "test_support/fake_assistant_manager_service_delegate.h",
+      "test_support/fake_platform_api.cc",
+      "test_support/fake_platform_api.h",
+      "test_support/mock_media_manager.cc",
+      "test_support/mock_media_manager.h",
     ]
 
     deps += [
+      "//chromeos/assistant/internal",
+      "//chromeos/assistant/internal:test_support",
+      "//chromeos/assistant/internal/proto/google3",
       "//chromeos/dbus",
       "//chromeos/services/network_config/public/mojom",
-      "//libassistant/shared/public",
       "//services/audio/public/cpp:test_support",
       "//services/device/public/cpp:test_support",
     ]

chromeos/services/assistant/assistant_manager_service.h

@@ -26,10 +26,14 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerService
   enum State {
     // Initial state, the service is created but not started yet.
     STOPPED = 0,
-    // The service is started, it takes a little time to be fully running.
-    STARTED = 1,
+    // Start has been called but libassistant creation is still in progress.
+    // Calling |assistant_manager()| will still return a nullptr.
+    STARTING = 1,
+    // The service is started, libassistant has been created, but libassistant
+    // is not ready yet to take requests.
+    STARTED = 2,
     // The service is fully running and ready to take requests.
-    RUNNING = 2
+    RUNNING = 3
   };
 
   ~AssistantManagerService() override = default;

chromeos/services/assistant/assistant_manager_service_delegate.h

+// Copyright 2019 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_MANAGER_SERVICE_DELEGATE_H_
+#define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_DELEGATE_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/single_thread_task_runner.h"
+
+namespace assistant_client {
+class AssistantManager;
+class AssistantManagerInternal;
+class PlatformApi;
+}  // namespace assistant_client
+
+namespace chromeos {
+namespace assistant {
+
+class AssistantMediaSession;
+class CrosPlatformApi;
+
+// Interface class that provides factory methods for assistant internal
+// functionality.
+class AssistantManagerServiceDelegate {
+ public:
+  AssistantManagerServiceDelegate() = default;
+  virtual ~AssistantManagerServiceDelegate() = default;
+
+  virtual std::unique_ptr<CrosPlatformApi> CreatePlatformApi(
+      AssistantMediaSession* media_session,
+      scoped_refptr<base::SingleThreadTaskRunner>
+          background_thread_task_runner) = 0;
+
+  virtual std::unique_ptr<assistant_client::AssistantManager>
+  CreateAssistantManager(assistant_client::PlatformApi* platform_api,
+                         const std::string& lib_assistant_config) = 0;
+
+  virtual assistant_client::AssistantManagerInternal*
+  UnwrapAssistantManagerInternal(
+      assistant_client::AssistantManager* assistant_manager) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AssistantManagerServiceDelegate);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_DELEGATE_H_

chromeos/services/assistant/assistant_manager_service_delegate_impl.cc

+// Copyright 2019 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/services/assistant/assistant_manager_service_delegate_impl.h"
+
+#include "ash/public/cpp/assistant/assistant_state_base.h"
+#include "chromeos/services/assistant/platform_api_impl.h"
+#include "chromeos/services/assistant/service_context.h"
+#include "libassistant/shared/internal_api/assistant_manager_internal.h"
+#include "libassistant/shared/public/assistant_manager.h"
+
+namespace chromeos {
+namespace assistant {
+
+AssistantManagerServiceDelegateImpl::AssistantManagerServiceDelegateImpl(
+    mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor,
+    mojom::Client* client,
+    ServiceContext* context)
+    : battery_monitor_(std::move(battery_monitor)),
+      client_(client),
+      context_(context) {}
+
+AssistantManagerServiceDelegateImpl::~AssistantManagerServiceDelegateImpl() =
+    default;
+
+std::unique_ptr<CrosPlatformApi>
+AssistantManagerServiceDelegateImpl::CreatePlatformApi(
+    AssistantMediaSession* media_session,
+    scoped_refptr<base::SingleThreadTaskRunner> background_thread_task_runner) {
+  return std::make_unique<PlatformApiImpl>(
+      client_, media_session, context_->power_manager_client(),
+      context_->cras_audio_handler(), std::move(battery_monitor_),
+      context_->main_task_runner(), background_thread_task_runner,
+      context_->assistant_state()->locale().value());
+}
+
+std::unique_ptr<assistant_client::AssistantManager>
+AssistantManagerServiceDelegateImpl::CreateAssistantManager(
+    assistant_client::PlatformApi* platform_api,
+    const std::string& lib_assistant_config) {
+  // This circumnvent way of creating the unique_ptr is required because
+  // |AssistantManager::Create| returns a raw pointer, and passing that in the
+  // constructor of unique_ptr is blocked by our presubmit checks that try to
+  // force us to use make_unique, which we can't use here.
+  std::unique_ptr<assistant_client::AssistantManager> result;
+  result.reset(assistant_client::AssistantManager::Create(
+      platform_api, lib_assistant_config));
+  return result;
+}
+
+assistant_client::AssistantManagerInternal*
+AssistantManagerServiceDelegateImpl::UnwrapAssistantManagerInternal(
+    assistant_client::AssistantManager* assistant_manager) {
+  return assistant_client::UnwrapAssistantManagerInternal(assistant_manager);
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/assistant_manager_service_delegate_impl.h

+// Copyright 2019 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_MANAGER_SERVICE_DELEGATE_IMPL_H_
+#define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_DELEGATE_IMPL_H_
+
+#include "chromeos/services/assistant/assistant_manager_service_delegate.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "services/device/public/mojom/battery_monitor.mojom.h"
+
+namespace chromeos {
+namespace assistant {
+namespace mojom {
+class Client;
+}  // namespace mojom
+}  // namespace assistant
+}  // namespace chromeos
+
+namespace chromeos {
+namespace assistant {
+
+class ServiceContext;
+
+class AssistantManagerServiceDelegateImpl
+    : public AssistantManagerServiceDelegate {
+ public:
+  AssistantManagerServiceDelegateImpl(
+      mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor,
+      mojom::Client* client,
+      ServiceContext* context);
+  ~AssistantManagerServiceDelegateImpl() override;
+
+  std::unique_ptr<CrosPlatformApi> CreatePlatformApi(
+      AssistantMediaSession* media_session,
+      scoped_refptr<base::SingleThreadTaskRunner> background_thread_task_runner)
+      override;
+
+  std::unique_ptr<assistant_client::AssistantManager> CreateAssistantManager(
+      assistant_client::PlatformApi* platform_api,
+      const std::string& lib_assistant_config) override;
+
+  assistant_client::AssistantManagerInternal* UnwrapAssistantManagerInternal(
+      assistant_client::AssistantManager* assistant_manager) override;
+
+ private:
+  mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor_;
+  // Owned by the parent |Service| which will destroy |this| before |context_|.
+  mojom::Client* client_;
+  ServiceContext* context_;
+
+  DISALLOW_COPY_AND_ASSIGN(AssistantManagerServiceDelegateImpl);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_DELEGATE_IMPL_H_

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -20,7 +20,6 @@
 #include "chromeos/services/assistant/assistant_manager_service.h"
 #include "chromeos/services/assistant/assistant_settings_manager_impl.h"
 #include "chromeos/services/assistant/chromium_api_delegate.h"
-#include "chromeos/services/assistant/platform_api_impl.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "libassistant/shared/internal_api/assistant_manager_delegate.h"
 #include "libassistant/shared/public/conversation_state_listener.h"
@@ -53,7 +52,9 @@ namespace chromeos {
 namespace assistant {
 
 class AssistantMediaSession;
+class CrosPlatformApi;
 class ServiceContext;
+class AssistantManagerServiceDelegate;
 
 // Enumeration of Assistant query response type, also recorded in histograms.
 // These values are persisted to logs. Entries should not be renumbered and
@@ -83,7 +84,7 @@ enum class AssistantQueryResponseType {
 // Since LibAssistant is a standalone library, all callbacks come from it
 // running on threads not owned by Chrome. Thus we need to post the callbacks
 // onto the main thread.
-class AssistantManagerServiceImpl
+class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
     : public AssistantManagerService,
       public ::chromeos::assistant::action::AssistantActionObserver,
       public AssistantEventObserver,
@@ -97,8 +98,8 @@ class AssistantManagerServiceImpl
   // |service| owns this class and must outlive this class.
   AssistantManagerServiceImpl(
       mojom::Client* client,
-      mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor,
       ServiceContext* context,
+      std::unique_ptr<AssistantManagerServiceDelegate> delegate,
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info,
       bool is_signed_out_mode);
@@ -222,6 +223,10 @@ class AssistantManagerServiceImpl
   void UpdateInternalMediaPlayerStatus(
       media_session::mojom::MediaSessionAction action);
 
+  // The start runs in the background. This will wait until the background
+  // thread is finished.
+  void WaitUntilStartIsFinishedForTesting();
+
  private:
   void StartAssistantInternal(const base::Optional<std::string>& access_token);
   void PostInitAssistant(base::OnceClosure post_init_callback);
@@ -309,10 +314,13 @@ class AssistantManagerServiceImpl
   mojom::DeviceActions* device_actions();
   scoped_refptr<base::SequencedTaskRunner> main_task_runner();
 
+  bool HasStartFinished() const;
+  void SetStartFinished();
+
   mojom::Client* const client_;
   State state_ = State::STOPPED;
   std::unique_ptr<AssistantMediaSession> media_session_;
-  std::unique_ptr<PlatformApiImpl> platform_api_;
+  std::unique_ptr<CrosPlatformApi> platform_api_;
   std::unique_ptr<action::CrosActionModule> action_module_;
   ChromiumApiDelegate chromium_api_delegate_;
   // NOTE: |display_connection_| is used by |assistant_manager_| and must be
@@ -320,11 +328,14 @@ class AssistantManagerServiceImpl
   std::unique_ptr<CrosDisplayConnection> display_connection_;
   std::unique_ptr<assistant_client::AssistantManager> assistant_manager_;
   std::unique_ptr<AssistantSettingsManagerImpl> assistant_settings_manager_;
-  // |new_asssistant_manager_| is created on |background_thread_| then posted to
+  // |new_assistant_manager_| is created on |background_thread_| then posted to
   // main thread to finish initialization then move to |assistant_manager_|.
   std::unique_ptr<assistant_client::AssistantManager> new_assistant_manager_;
+  // Same ownership as |new_assistant_manager_|.
+  assistant_client::AssistantManagerInternal* new_assistant_manager_internal_ =
+      nullptr;
   base::Lock new_assistant_manager_lock_;
-  // same ownership as assistant_manager_.
+  // same ownership as |assistant_manager_|.
   assistant_client::AssistantManagerInternal* assistant_manager_internal_ =
       nullptr;
   mojo::RemoteSet<mojom::AssistantInteractionSubscriber>
@@ -334,6 +345,8 @@ class AssistantManagerServiceImpl
   // Owned by the parent |Service| which will destroy |this| before |context_|.
   ServiceContext* const context_;
 
+  std::unique_ptr<AssistantManagerServiceDelegate> delegate_;
+
   bool spoken_feedback_enabled_ = false;
 
   ax::mojom::AssistantExtraPtr assistant_extra_;
@@ -368,8 +381,6 @@ class AssistantManagerServiceImpl
   base::UnguessableToken media_session_audio_focus_id_ =
       base::UnguessableToken::Null();
 
-  bool start_finished_ = false;
-
   mojo::Binding<mojom::AppListEventSubscriber> app_list_subscriber_binding_;
 
   base::ObserverList<AssistantCommunicationErrorObserver> error_observers_;

chromeos/services/assistant/assistant_settings_manager_impl.cc

@@ -24,6 +24,17 @@ using SpeakerIdEnrollmentState =
 namespace chromeos {
 namespace assistant {
 
+namespace {
+
+bool HasStarted(const AssistantManagerService* assistant_manager_service) {
+  return (assistant_manager_service->GetState() ==
+              AssistantManagerService::STARTED ||
+          assistant_manager_service->GetState() ==
+              AssistantManagerService::RUNNING);
+}
+
+}  // namespace
+
 AssistantSettingsManagerImpl::AssistantSettingsManagerImpl(
     ServiceContext* context,
     AssistantManagerServiceImpl* assistant_manager_service)
@@ -40,8 +51,7 @@ void AssistantSettingsManagerImpl::BindRequest(
 
 void AssistantSettingsManagerImpl::GetSettings(const std::string& selector,
                                                GetSettingsCallback callback) {
-  DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
+  DCHECK(HasStarted(assistant_manager_service_));
   DCHECK(main_task_runner()->RunsTasksInCurrentSequence());
 
   // TODO(xiaohuic): libassistant could be restarting for various reasons. In
@@ -79,8 +89,7 @@ void AssistantSettingsManagerImpl::GetSettings(const std::string& selector,
 void AssistantSettingsManagerImpl::UpdateSettings(
     const std::string& update,
     GetSettingsCallback callback) {
-  DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
+  DCHECK(HasStarted(assistant_manager_service_));
   DCHECK(main_task_runner()->RunsTasksInCurrentSequence());
 
   if (!assistant_manager_service_->assistant_manager_internal()) {
@@ -114,8 +123,7 @@ void AssistantSettingsManagerImpl::UpdateSettings(
 void AssistantSettingsManagerImpl::StartSpeakerIdEnrollment(
     bool skip_cloud_enrollment,
     mojom::SpeakerIdEnrollmentClientPtr client) {
-  DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
+  DCHECK(HasStarted(assistant_manager_service_));
   DCHECK(main_task_runner()->RunsTasksInCurrentSequence());
 
   if (!assistant_manager_service_->assistant_manager_internal())
@@ -142,8 +150,7 @@ void AssistantSettingsManagerImpl::StartSpeakerIdEnrollment(
 
 void AssistantSettingsManagerImpl::StopSpeakerIdEnrollment(
     StopSpeakerIdEnrollmentCallback callback) {
-  DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
+  DCHECK(HasStarted(assistant_manager_service_));
   DCHECK(main_task_runner()->RunsTasksInCurrentSequence());
 
   if (!assistant_manager_service_->assistant_manager_internal()) {

chromeos/services/assistant/constants.h

@@ -5,9 +5,12 @@
 #ifndef CHROMEOS_SERVICES_ASSISTANT_CONSTANTS_H_
 #define CHROMEOS_SERVICES_ASSISTANT_CONSTANTS_H_
 
+#include "base/component_export.h"
+
 namespace chromeos {
 namespace assistant {
 
+COMPONENT_EXPORT(ASSISTANT_SERVICE)
 extern const char kUserID[];
 
 }  // namespace assistant

chromeos/services/assistant/cros_platform_api.h

+// Copyright 2019 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_CROS_PLATFORM_API_H_
+#define CHROMEOS_SERVICES_ASSISTANT_CROS_PLATFORM_API_H_
+
+#include "base/macros.h"
+#include "libassistant/shared/public/platform_api.h"
+
+namespace chromeos {
+namespace assistant {
+
+// Platform API required by the voice assistant, extended with some methods used
+// when ChromeOS needs to make changes to the platform state.
+class CrosPlatformApi : public assistant_client::PlatformApi {
+ public:
+  CrosPlatformApi() = default;
+  ~CrosPlatformApi() override = default;
+
+  // Called when the mic state associated with the interaction is changed.
+  virtual void SetMicState(bool mic_open) = 0;
+
+  virtual void OnConversationTurnStarted() = 0;
+  virtual void OnConversationTurnFinished() = 0;
+
+  // Called when hotword enabled status changed.
+  virtual void OnHotwordEnabled(bool enable) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CrosPlatformApi);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_CROS_PLATFORM_API_H_

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -23,7 +23,7 @@ void FakeAssistantManagerServiceImpl::Start(
     const base::Optional<std::string>& access_token,
     bool enable_hotword,
     base::OnceClosure callback) {
-  state_ = State::STARTED;
+  state_ = State::STARTING;
   start_callback_ = std::move(callback);
 }
 

chromeos/services/assistant/platform_api_impl.cc

@@ -184,6 +184,14 @@ void PlatformApiImpl::SetMicState(bool mic_open) {
   audio_input_provider_.SetMicState(mic_open);
 }
 
+void PlatformApiImpl::OnConversationTurnStarted() {
+  audio_input_provider_.GetAudioInput().OnConversationTurnStarted();
+}
+
+void PlatformApiImpl::OnConversationTurnFinished() {
+  audio_input_provider_.GetAudioInput().OnConversationTurnFinished();
+}
+
 void PlatformApiImpl::OnHotwordEnabled(bool enable) {
   audio_input_provider_.OnHotwordEnabled(enable);
 }

chromeos/services/assistant/platform_api_impl.h

@@ -11,13 +11,13 @@
 #include <vector>
 
 #include "chromeos/audio/cras_audio_handler.h"
+#include "chromeos/services/assistant/cros_platform_api.h"
 #include "chromeos/services/assistant/platform/audio_input_provider_impl.h"
 #include "chromeos/services/assistant/platform/audio_output_provider_impl.h"
 #include "chromeos/services/assistant/platform/file_provider_impl.h"
 #include "chromeos/services/assistant/platform/network_provider_impl.h"
 #include "chromeos/services/assistant/platform/system_provider_impl.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
-#include "libassistant/shared/public/platform_api.h"
 #include "libassistant/shared/public/platform_auth.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "services/device/public/mojom/battery_monitor.mojom.h"
@@ -30,7 +30,7 @@ namespace assistant {
 class AssistantMediaSession;
 
 // Platform API required by the voice assistant.
-class PlatformApiImpl : public assistant_client::PlatformApi,
+class PlatformApiImpl : public CrosPlatformApi,
                         CrasAudioHandler::AudioObserver {
  public:
   PlatformApiImpl(
@@ -56,10 +56,13 @@ class PlatformApiImpl : public assistant_client::PlatformApi,
   void OnAudioNodesChanged() override;
 
   // Called when the mic state associated with the interaction is changed.
-  void SetMicState(bool mic_open);
+  void SetMicState(bool mic_open) override;
+
+  void OnConversationTurnStarted() override;
+  void OnConversationTurnFinished() override;
 
   // Called when hotword enabled status changed.
-  void OnHotwordEnabled(bool enable);
+  void OnHotwordEnabled(bool enable) override;
 
  private:
   // ChromeOS does not use auth manager, so we don't yet need to implement a

chromeos/services/assistant/service.cc

@@ -24,6 +24,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
 #include "chromeos/services/assistant/assistant_manager_service.h"
+#include "chromeos/services/assistant/assistant_manager_service_delegate_impl.h"
 #include "chromeos/services/assistant/assistant_settings_manager.h"
 #include "chromeos/services/assistant/fake_assistant_manager_service_impl.h"
 #include "chromeos/services/assistant/fake_assistant_settings_manager_impl.h"
@@ -314,6 +315,7 @@ void Service::UpdateAssistantManagerState() {
         DVLOG(1) << "Request Assistant start";
       }
       break;
+    case AssistantManagerService::State::STARTING:
     case AssistantManagerService::State::STARTED:
       // Wait if |assistant_manager_service_| is not at a stable state.
       update_assistant_manager_callback_.Cancel();
@@ -435,10 +437,13 @@ void Service::CreateAssistantManagerService() {
   client_->RequestBatteryMonitor(
       battery_monitor.InitWithNewPipeAndPassReceiver());
 
+  auto delegate = std::make_unique<AssistantManagerServiceDelegateImpl>(
+      std::move(battery_monitor), client_.get(), context());
+
   // |assistant_manager_service_| is only created once.
   DCHECK(url_loader_factory_info_);
   assistant_manager_service_ = std::make_unique<AssistantManagerServiceImpl>(
-      client_.get(), std::move(battery_monitor), context(),
+      client_.get(), context(), std::move(delegate),
       std::move(url_loader_factory_info_), is_signed_out_mode_);
 #else
   assistant_manager_service_ =
@@ -450,9 +455,10 @@ void Service::CreateAssistantManagerService() {
 
 void Service::FinalizeAssistantManagerService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
+             AssistantManagerService::STARTED ||
+         assistant_manager_service_->GetState() ==
+             AssistantManagerService::RUNNING);
 
   // Using session_observer_binding_ as a flag to control onetime initialization
   if (!observing_ash_session_) {

chromeos/services/assistant/service_unittest.cc

@@ -8,7 +8,6 @@
 #include <utility>
 #include <vector>
 
-#include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/mojom/assistant_state_controller.mojom.h"
 #include "base/bind.h"
 #include "base/logging.h"
@@ -27,6 +26,7 @@
 #include "chromeos/services/assistant/fake_client.h"
 #include "chromeos/services/assistant/pref_connection_delegate.h"
 #include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
+#include "chromeos/services/assistant/test_support/fully_initialized_assistant_state.h"
 #include "components/prefs/testing_pref_service.h"
 #include "mojo/public/cpp/bindings/interface_ptr_set.h"
 #include "services/identity/public/mojom/identity_accessor.mojom.h"
@@ -131,12 +131,6 @@ class FakeAssistantClient : public FakeClient {
   explicit FakeAssistantClient(ash::AssistantState* assistant_state)
       : assistant_state_(assistant_state) {}
 
-  mojom::ClientPtr CreatePendingRemoteAndBind() {
-    mojom::ClientPtr ptr;
-    binding_.Bind(mojo::MakeRequest(&ptr));
-    return ptr;
-  }
-
  private:
   // FakeClient:
   void RequestAssistantStateController(
@@ -146,7 +140,6 @@ class FakeAssistantClient : public FakeClient {
   }
 
   ash::AssistantState* const assistant_state_;
-  mojo::Binding<mojom::Client> binding_{this};
 
   DISALLOW_COPY_AND_ASSIGN(FakeAssistantClient);
 };
@@ -201,11 +194,6 @@ class AssistantServiceTest : public testing::Test {
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             &url_loader_factory_);
 
-    assistant_state()->NotifyArcPlayStoreEnabledChanged(true);
-    assistant_state()->NotifyFeatureAllowed(
-        ash::mojom::AssistantAllowedState::ALLOWED);
-    assistant_state()->NotifyLocaleChanged("en_US");
-
     auto fake_pref_connection = std::make_unique<FakePrefConnectionDelegate>();
     pref_service_ = fake_pref_connection->pref_service();
 
@@ -265,7 +253,7 @@ class AssistantServiceTest : public testing::Test {
   std::unique_ptr<Service> service_;
   mojo::Remote<mojom::AssistantService> remote_service_;
 
-  ash::AssistantState assistant_state_;
+  FullyInitializedAssistantState assistant_state_;
 
   FakeIdentityAccessor fake_identity_accessor_;
   FakeAssistantClient fake_assistant_client_{&assistant_state_};
@@ -342,19 +330,19 @@ TEST_F(AssistantServiceTest, StopImmediatelyIfAssistantIsRunning) {
 }
 
 TEST_F(AssistantServiceTest, StopDelayedIfAssistantNotFinishedStarting) {
-  // Test is set up as |State::STARTED|, turning settings off will trigger
+  // Test is set up as |State::STARTING|, turning settings off will trigger
   // logic to try to stop it.
   pref_service()->SetBoolean(prefs::kAssistantEnabled, false);
 
   EXPECT_EQ(assistant_manager()->GetState(),
-            AssistantManagerService::State::STARTED);
+            AssistantManagerService::State::STARTING);
 
   mock_task_runner()->FastForwardBy(kUpdateAssistantManagerDelay);
   base::RunLoop().RunUntilIdle();
 
   // No change of state because it is still starting.
   EXPECT_EQ(assistant_manager()->GetState(),
-            AssistantManagerService::State::STARTED);
+            AssistantManagerService::State::STARTING);
 
   assistant_manager()->FinishStart();
 

chromeos/services/assistant/test_support/fake_assistant_manager_service_delegate.cc

+// Copyright 2019 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/services/assistant/test_support/fake_assistant_manager_service_delegate.h"
+
+#include "chromeos/assistant/internal/test_support/fake_assistant_manager.h"
+#include "chromeos/assistant/internal/test_support/fake_assistant_manager_internal.h"
+#include "chromeos/services/assistant/test_support/fake_platform_api.h"
+
+namespace chromeos {
+namespace assistant {
+
+FakeAssistantManagerServiceDelegate::FakeAssistantManagerServiceDelegate()
+    : assistant_manager_(std::make_unique<FakeAssistantManager>()),
+      assistant_manager_internal_(
+          std::make_unique<FakeAssistantManagerInternal>()),
+      assistant_manager_ptr_(assistant_manager_.get()) {}
+
+FakeAssistantManagerServiceDelegate::~FakeAssistantManagerServiceDelegate() =
+    default;
+
+std::unique_ptr<CrosPlatformApi>
+FakeAssistantManagerServiceDelegate::CreatePlatformApi(
+    AssistantMediaSession* media_session,
+    scoped_refptr<base::SingleThreadTaskRunner> background_thread_task_runner) {
+  return std::make_unique<FakePlatformApi>();
+}
+
+std::unique_ptr<assistant_client::AssistantManager>
+FakeAssistantManagerServiceDelegate::CreateAssistantManager(
+    assistant_client::PlatformApi* platform_api,
+    const std::string& lib_assistant_config) {
+  DCHECK(assistant_manager_);
+  return std::move(assistant_manager_);
+}
+
+assistant_client::AssistantManagerInternal*
+FakeAssistantManagerServiceDelegate::UnwrapAssistantManagerInternal(
+    assistant_client::AssistantManager* assistant_manager) {
+  return assistant_manager_internal_.get();
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/fake_assistant_manager_service_delegate.h

+// Copyright 2019 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_ASSISTANT_MANAGER_SERVICE_DELEGATE_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_ASSISTANT_MANAGER_SERVICE_DELEGATE_H_
+
+#include "chromeos/services/assistant/assistant_manager_service_delegate.h"
+
+namespace chromeos {
+namespace assistant {
+
+class FakeAssistantManager;
+class FakeAssistantManagerInternal;
+
+// Implementation of |AssistantManagerServiceDelegate| that returns fake
+// instances for all of the member methods. Used during unittests.
+class FakeAssistantManagerServiceDelegate
+    : public AssistantManagerServiceDelegate {
+ public:
+  FakeAssistantManagerServiceDelegate();
+  ~FakeAssistantManagerServiceDelegate() override;
+
+  FakeAssistantManager* assistant_manager() { return assistant_manager_ptr_; }
+
+  FakeAssistantManagerInternal* assistant_manager_internal() {
+    return assistant_manager_internal_.get();
+  }
+
+  // AssistantManagerServiceDelegate:
+  std::unique_ptr<CrosPlatformApi> CreatePlatformApi(
+      AssistantMediaSession* media_session,
+      scoped_refptr<base::SingleThreadTaskRunner> background_thread_task_runner)
+      override;
+  std::unique_ptr<assistant_client::AssistantManager> CreateAssistantManager(
+      assistant_client::PlatformApi* platform_api,
+      const std::string& lib_assistant_config) override;
+  assistant_client::AssistantManagerInternal* UnwrapAssistantManagerInternal(
+      assistant_client::AssistantManager* assistant_manager) override;
+
+ private:
+  // Will be initialized in the constructor and moved out when
+  // |CreateAssistantManager| is called.
+  std::unique_ptr<FakeAssistantManager> assistant_manager_;
+  std::unique_ptr<FakeAssistantManagerInternal> assistant_manager_internal_;
+  FakeAssistantManager* assistant_manager_ptr_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeAssistantManagerServiceDelegate);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_ASSISTANT_MANAGER_SERVICE_DELEGATE_H_

chromeos/services/assistant/test_support/fake_platform_api.cc

+// Copyright 2019 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/services/assistant/test_support/fake_platform_api.h"
+#include "base/logging.h"
+
+namespace chromeos {
+namespace assistant {
+
+assistant_client::AudioInputProvider& FakePlatformApi::GetAudioInputProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+assistant_client::AudioOutputProvider&
+FakePlatformApi::GetAudioOutputProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+assistant_client::AuthProvider& FakePlatformApi::GetAuthProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+assistant_client::FileProvider& FakePlatformApi::GetFileProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+assistant_client::NetworkProvider& FakePlatformApi::GetNetworkProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+assistant_client::SystemProvider& FakePlatformApi::GetSystemProvider() {
+  NOTIMPLEMENTED();
+  abort();
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/fake_platform_api.h

+// Copyright 2019 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_PLATFORM_API_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_PLATFORM_API_H_
+
+#include "base/macros.h"
+#include "chromeos/services/assistant/cros_platform_api.h"
+
+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.
+class FakePlatformApi : public CrosPlatformApi {
+ public:
+  FakePlatformApi() = default;
+  ~FakePlatformApi() override = default;
+
+  // CrosPlatformApi overrides
+  assistant_client::AudioInputProvider& GetAudioInputProvider() override;
+  assistant_client::AudioOutputProvider& GetAudioOutputProvider() override;
+  assistant_client::AuthProvider& GetAuthProvider() override;
+  assistant_client::FileProvider& GetFileProvider() override;
+  assistant_client::NetworkProvider& GetNetworkProvider() override;
+  assistant_client::SystemProvider& GetSystemProvider() override;
+  void SetMicState(bool mic_open) override {}
+  void OnHotwordEnabled(bool enable) override {}
+  void OnConversationTurnStarted() override {}
+  void OnConversationTurnFinished() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakePlatformApi);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FAKE_PLATFORM_API_H_

chromeos/services/assistant/test_support/fully_initialized_assistant_state.cc

+// Copyright 2019 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/services/assistant/test_support/fully_initialized_assistant_state.h"
+
+namespace chromeos {
+namespace assistant {
+
+FullyInitializedAssistantState::FullyInitializedAssistantState() {
+  InitializeAllValues();
+}
+
+void FullyInitializedAssistantState::InitializeAllValues() {
+  settings_enabled_ = true;
+  consent_status_ =
+      chromeos::assistant::prefs::ConsentStatus::kActivityControlAccepted;
+  context_enabled_ = true;
+  hotword_enabled_ = true;
+  hotword_always_on_ = true;
+  launch_with_mic_open_ = true;
+  notification_enabled_ = true;
+  allowed_state_ = ash::mojom::AssistantAllowedState::ALLOWED;
+  locale_ = "en_US";
+  arc_play_store_enabled_ = true;
+  locked_full_screen_enabled_ = true;
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/fully_initialized_assistant_state.h

+// Copyright 2019 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_FULLY_INITIALIZED_ASSISTANT_STATE_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FULLY_INITIALIZED_ASSISTANT_STATE_H_
+
+#include "ash/public/cpp/assistant/assistant_state.h"
+
+namespace chromeos {
+namespace assistant {
+
+// Instance of |AssistantState| where every base::Optional value has a non-null
+// value. All values will be set to their equivalent of enabled.
+class FullyInitializedAssistantState : public ash::AssistantState {
+ public:
+  FullyInitializedAssistantState();
+  ~FullyInitializedAssistantState() override = default;
+
+ private:
+  void InitializeAllValues();
+
+  DISALLOW_COPY_AND_ASSIGN(FullyInitializedAssistantState);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_FULLY_INITIALIZED_ASSISTANT_STATE_H_

chromeos/services/assistant/test_support/mock_media_manager.cc

+// Copyright 2019 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/services/assistant/test_support/mock_media_manager.h"
+
+namespace chromeos {
+namespace assistant {
+
+MockMediaManager::MockMediaManager() = default;
+MockMediaManager::~MockMediaManager() = default;
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/mock_media_manager.h

+// Copyright 2019 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_MOCK_MEDIA_MANAGER_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_MOCK_MEDIA_MANAGER_H_
+
+#include "libassistant/shared/public/media_manager.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace chromeos {
+namespace assistant {
+
+class MockMediaManager : public assistant_client::MediaManager {
+ public:
+  MockMediaManager();
+  ~MockMediaManager() override;
+
+  // MediaManager:
+  MOCK_METHOD1(AddListener, void(Listener*));
+  MOCK_METHOD0(Next, void());
+  MOCK_METHOD0(Previous, void());
+  MOCK_METHOD0(Pause, void());
+  MOCK_METHOD0(Resume, void());
+  MOCK_METHOD0(StopAndClearPlaylist, void());
+  MOCK_METHOD0(PlayPause, void());
+  MOCK_METHOD1(SetExternalPlaybackState,
+               void(const assistant_client::MediaStatus&));
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_MOCK_MEDIA_MANAGER_H_