Hook up "Enable Assitant" settings

- Stop/Restart Assitant when settings changed;
- Enable Assistant at start of optin flow.
- Hide UI when Assitant is disabled.

Bug: b/110991778, b/111720990
Change-Id: I675ec2b257fdbf8fd7ff2c9bcf9484cc3ea4beb1
Reviewed-on: https://chromium-review.googlesource.com/1150923
Commit-Queue: Yue Li <updowndota@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579937}

ash/assistant/assistant_controller.cc

@@ -13,6 +13,7 @@
 #include "ash/new_window_controller.h"
 #include "ash/session/session_controller.h"
 #include "ash/shell.h"
+#include "ash/voice_interaction/voice_interaction_controller.h"
 #include "base/bind.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/unguessable_token.h"
@@ -27,7 +28,12 @@ AssistantController::AssistantController()
       assistant_screen_context_controller_(
           std::make_unique<AssistantScreenContextController>(this)),
       assistant_ui_controller_(std::make_unique<AssistantUiController>(this)),
+      voice_interaction_binding_(this),
       weak_factory_(this) {
+  mojom::VoiceInteractionObserverPtr ptr;
+  voice_interaction_binding_.Bind(mojo::MakeRequest(&ptr));
+  Shell::Get()->voice_interaction_controller()->AddObserver(std::move(ptr));
+
   AddObserver(this);
   NotifyConstructed();
 }
@@ -220,6 +226,12 @@ void AssistantController::NotifyUrlOpened(const GURL& url) {
     observer.OnUrlOpened(url);
 }
 
+void AssistantController::OnVoiceInteractionStatusChanged(
+    mojom::VoiceInteractionState state) {
+  if (state == mojom::VoiceInteractionState::STOPPED)
+    assistant_ui_controller_->HideUi(AssistantSource::kUnspecified);
+}
+
 base::WeakPtr<AssistantController> AssistantController::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

ash/assistant/assistant_controller.h

@@ -14,6 +14,7 @@
 #include "ash/public/interfaces/assistant_controller.mojom.h"
 #include "ash/public/interfaces/assistant_image_downloader.mojom.h"
 #include "ash/public/interfaces/assistant_setup.mojom.h"
+#include "ash/public/interfaces/voice_interaction_controller.mojom.h"
 #include "ash/public/interfaces/web_contents_manager.mojom.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -36,7 +37,8 @@ class AssistantUiController;
 class ASH_EXPORT AssistantController
     : public mojom::AssistantController,
       public AssistantControllerObserver,
-      public mojom::ManagedWebContentsOpenUrlDelegate {
+      public mojom::ManagedWebContentsOpenUrlDelegate,
+      public mojom::VoiceInteractionObserver {
  public:
   AssistantController();
   ~AssistantController() override;
@@ -125,6 +127,16 @@ class ASH_EXPORT AssistantController
   void NotifyDeepLinkReceived(const GURL& deep_link);
   void NotifyUrlOpened(const GURL& url);
 
+  // mojom::VoiceInteractionObserver:
+  void OnVoiceInteractionStatusChanged(
+      mojom::VoiceInteractionState state) override;
+  void OnVoiceInteractionSettingsEnabled(bool enabled) override {}
+  void OnVoiceInteractionContextEnabled(bool enabled) override {}
+  void OnVoiceInteractionHotwordEnabled(bool enabled) override {}
+  void OnVoiceInteractionSetupCompleted(bool completed) override {}
+  void OnAssistantFeatureAllowedChanged(
+      mojom::AssistantAllowedState state) override {}
+
   // The observer list should be initialized early so that sub-controllers may
   // register as observers during their construction.
   base::ObserverList<AssistantControllerObserver> observers_;
@@ -153,6 +165,8 @@ class ASH_EXPORT AssistantController
 
   std::unique_ptr<AssistantUiController> assistant_ui_controller_;
 
+  mojo::Binding<mojom::VoiceInteractionObserver> voice_interaction_binding_;
+
   base::WeakPtrFactory<AssistantController> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AssistantController);

ash/assistant/assistant_interaction_controller.cc

@@ -11,7 +11,9 @@
 #include "ash/assistant/model/assistant_response.h"
 #include "ash/assistant/model/assistant_ui_element.h"
 #include "ash/assistant/util/deep_link_util.h"
+#include "ash/public/interfaces/voice_interaction_controller.mojom.h"
 #include "ash/shell.h"
+#include "ash/voice_interaction/voice_interaction_controller.h"
 #include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 
@@ -327,6 +329,10 @@ void AssistantInteractionController::StartTextInteraction(
     const std::string text) {
   StopActiveInteraction();
 
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
+
   assistant_interaction_model_.SetPendingQuery(
       std::make_unique<AssistantTextQuery>(text));
 
@@ -336,6 +342,10 @@ void AssistantInteractionController::StartTextInteraction(
 void AssistantInteractionController::StartVoiceInteraction() {
   StopActiveInteraction();
 
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
+
   assistant_interaction_model_.SetPendingQuery(
       std::make_unique<AssistantVoiceQuery>());
 
@@ -349,6 +359,10 @@ void AssistantInteractionController::StopActiveInteraction() {
   // events belonging to the interaction being stopped.
   assistant_interaction_model_.SetInteractionState(InteractionState::kInactive);
   assistant_interaction_model_.ClearPendingQuery();
+
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
   assistant_->StopActiveInteraction();
 
   // Because we are stopping an interaction in progress, we discard any pending

ash/assistant/assistant_notification_controller.cc

@@ -6,6 +6,7 @@
 
 #include "ash/assistant/assistant_controller.h"
 #include "ash/new_window_controller.h"
+#include "ash/public/interfaces/voice_interaction_controller.mojom.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -110,11 +111,17 @@ void AssistantNotificationController::SetAssistant(
 void AssistantNotificationController::RetrieveNotification(
     AssistantNotificationPtr notification,
     int action_index) {
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
   assistant_->RetrieveNotification(std::move(notification), action_index);
 }
 
 void AssistantNotificationController::DismissNotification(
     AssistantNotificationPtr notification) {
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
   assistant_->DismissNotification(std::move(notification));
 }
 

ash/assistant/assistant_screen_context_controller.cc

@@ -9,7 +9,9 @@
 #include "ash/assistant/assistant_ui_controller.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/window_properties.h"
+#include "ash/public/interfaces/voice_interaction_controller.mojom.h"
 #include "ash/shell.h"
+#include "ash/voice_interaction/voice_interaction_controller.h"
 #include "ash/wm/mru_window_tracker.h"
 #include "base/bind.h"
 #include "base/memory/scoped_refptr.h"
@@ -170,6 +172,10 @@ void AssistantScreenContextController::RequestScreenshot(
 
 void AssistantScreenContextController::RequestScreenContext(
     const gfx::Rect& rect) {
+  if (Shell::Get()->voice_interaction_controller()->voice_interaction_state() !=
+      mojom::VoiceInteractionState::RUNNING)
+    return;
+
   // Abort any request in progress and update request state.
   screen_context_request_factory_.InvalidateWeakPtrs();
   assistant_screen_context_model_.SetRequestState(

chrome/browser/ui/webui/chromeos/assistant_optin/assistant_optin_handler.cc

@@ -263,7 +263,6 @@ void AssistantOptInHandler::OnActivityControlOptInResult(bool opted_in) {
     PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();
     prefs->SetBoolean(arc::prefs::kVoiceInteractionActivityControlAccepted,
                       false);
-    prefs->SetBoolean(arc::prefs::kVoiceInteractionEnabled, true);
     CallJSOrDefer("closeDialog");
   }
 }
@@ -336,7 +335,6 @@ void AssistantOptInHandler::OnGetSettingsResponse(const std::string& settings) {
     PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();
     prefs->SetBoolean(arc::prefs::kVoiceInteractionActivityControlAccepted,
                       true);
-    prefs->SetBoolean(arc::prefs::kVoiceInteractionEnabled, true);
     ShowNextScreen();
   } else {
     AddSettingZippy("settings",
@@ -372,7 +370,6 @@ void AssistantOptInHandler::OnUpdateSettingsResponse(
       PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();
       prefs->SetBoolean(arc::prefs::kVoiceInteractionActivityControlAccepted,
                         true);
-      prefs->SetBoolean(arc::prefs::kVoiceInteractionEnabled, true);
     }
   }
 

chrome/browser/ui/webui/chromeos/assistant_optin/assistant_optin_ui.cc

@@ -68,6 +68,10 @@ AssistantOptInUI::AssistantOptInUI(content::WebUI* web_ui)
   source->AddResourcePath("assistant_logo.png", IDR_ASSISTANT_LOGO_PNG);
   source->SetDefaultResource(IDR_ASSISTANT_OPTIN_HTML);
   content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), source);
+
+  // Make sure enable Assistant service since we need it during the flow.
+  PrefService* prefs = Profile::FromWebUI(web_ui)->GetPrefs();
+  prefs->SetBoolean(arc::prefs::kVoiceInteractionEnabled, true);
 }
 
 AssistantOptInUI::~AssistantOptInUI() = default;

chromeos/services/assistant/assistant_manager_service.h

@@ -35,6 +35,9 @@ class AssistantManagerService : public mojom::Assistant {
   virtual void Start(const std::string& access_token,
                      base::OnceClosure callback) = 0;
 
+  // Stop the assistant.
+  virtual void Stop() = 0;
+
   // Returns the current state.
   virtual State GetState() const = 0;
 

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -49,6 +49,8 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
       enable_hotword_(enable_hotword),
       action_module_(std::make_unique<action::CrosActionModule>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      assistant_settings_manager_(
+          std::make_unique<AssistantSettingsManagerImpl>(this)),
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),
       voice_interaction_observer_binding_(this),
       service_(service),
@@ -81,6 +83,13 @@ void AssistantManagerServiceImpl::Start(const std::string& access_token,
                      base::Unretained(this), std::move(post_init_callback)));
 }
 
+void AssistantManagerServiceImpl::Stop() {
+  state_ = State::STOPPED;
+
+  assistant_manager_internal_ = nullptr;
+  assistant_manager_.reset(nullptr);
+}
+
 AssistantManagerService::State AssistantManagerServiceImpl::GetState() const {
   return state_;
 }
@@ -469,6 +478,12 @@ void AssistantManagerServiceImpl::OnVoiceInteractionContextEnabled(
   context_enabled_ = enabled;
 }
 
+void AssistantManagerServiceImpl::OnVoiceInteractionHotwordEnabled(
+    bool enabled) {
+  enable_hotword_ = enabled;
+  platform_api_.OnHotwordEnabled(enabled);
+}
+
 void AssistantManagerServiceImpl::OnVoiceInteractionSetupCompleted(
     bool completed) {
   UpdateDeviceLocale(completed);
@@ -508,10 +523,6 @@ void AssistantManagerServiceImpl::PostInitAssistant(
 
   state_ = State::RUNNING;
 
-  if (!assistant_settings_manager_) {
-    assistant_settings_manager_ =
-        std::make_unique<AssistantSettingsManagerImpl>(this);
-  }
   std::move(post_init_callback).Run();
   UpdateDeviceSettings();
 }

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -70,6 +70,7 @@ class AssistantManagerServiceImpl
   // assistant::AssistantManagerService overrides
   void Start(const std::string& access_token,
              base::OnceClosure callback) override;
+  void Stop() override;
   State GetState() const override;
   void SetAccessToken(const std::string& access_token) override;
   void EnableListening(bool enable) override;
@@ -133,7 +134,7 @@ class AssistantManagerServiceImpl
       ash::mojom::VoiceInteractionState state) override {}
   void OnVoiceInteractionSettingsEnabled(bool enabled) override;
   void OnVoiceInteractionContextEnabled(bool enabled) override;
-  void OnVoiceInteractionHotwordEnabled(bool enabled) override {}
+  void OnVoiceInteractionHotwordEnabled(bool enabled) override;
   void OnVoiceInteractionSetupCompleted(bool completed) override;
   void OnAssistantFeatureAllowedChanged(
       ash::mojom::AssistantAllowedState state) override {}

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -19,6 +19,10 @@ void FakeAssistantManagerServiceImpl::Start(const std::string& access_token,
     std::move(callback).Run();
 }
 
+void FakeAssistantManagerServiceImpl::Stop() {
+  state_ = State::STOPPED;
+}
+
 void FakeAssistantManagerServiceImpl::SetAccessToken(
     const std::string& access_token) {}
 

chromeos/services/assistant/fake_assistant_manager_service_impl.h

@@ -26,6 +26,7 @@ class FakeAssistantManagerServiceImpl : public AssistantManagerService {
   // assistant::AssistantManagerService overrides
   void Start(const std::string& access_token,
              base::OnceClosure callback) override;
+  void Stop() override;
   void SetAccessToken(const std::string& access_token) override;
   void EnableListening(bool enable) override;
   State GetState() const override;

chromeos/services/assistant/platform/audio_input_provider_impl.cc

@@ -150,6 +150,14 @@ void AudioInputImpl::SetMicState(bool mic_open) {
   }
 }
 
+void AudioInputImpl::OnHotwordEnabled(bool enable) {
+  default_on_ = enable;
+  if (default_on_ || mic_open)
+    source_->Start();
+  else
+    source_->Stop();
+}
+
 void AudioInputImpl::StartRecording() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   source_->Start();
@@ -180,5 +188,9 @@ void AudioInputProviderImpl::SetMicState(bool mic_open) {
   audio_input_.SetMicState(mic_open);
 }
 
+void AudioInputProviderImpl::OnHotwordEnabled(bool enable) {
+  audio_input_.OnHotwordEnabled(enable);
+}
+
 }  // namespace assistant
 }  // namespace chromeos

chromeos/services/assistant/platform/audio_input_provider_impl.h

@@ -72,6 +72,9 @@ class AudioInputImpl : public assistant_client::AudioInput,
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open);
 
+  // Called when hotword enabled status changed.
+  void OnHotwordEnabled(bool enable);
+
  private:
   void StartRecording();
   void StopRecording();
@@ -79,7 +82,7 @@ class AudioInputImpl : public assistant_client::AudioInput,
   scoped_refptr<media::AudioCapturerSource> source_;
 
   // Should audio input always recording actively.
-  const bool default_on_;
+  bool default_on_;
 
   // Guards observers_;
   base::Lock lock_;
@@ -109,6 +112,9 @@ class AudioInputProviderImpl : public assistant_client::AudioInputProvider {
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open);
 
+  // Called when hotword enabled status changed.
+  void OnHotwordEnabled(bool enable);
+
  private:
   AudioInputImpl audio_input_;
 

chromeos/services/assistant/platform_api_impl.cc

@@ -109,5 +109,9 @@ void PlatformApiImpl::SetMicState(bool mic_open) {
   audio_input_provider_.SetMicState(mic_open);
 }
 
+void PlatformApiImpl::OnHotwordEnabled(bool enable) {
+  audio_input_provider_.OnHotwordEnabled(enable);
+}
+
 }  // namespace assistant
 }  // namespace chromeos

chromeos/services/assistant/platform_api_impl.h

@@ -49,6 +49,9 @@ class PlatformApiImpl : public assistant_client::PlatformApi {
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open);
 
+  // Called when hotword enabled status changed.
+  void OnHotwordEnabled(bool enable);
+
  private:
   // ChromeOS does not use auth manager, so we don't yet need to implement a
   // real auth provider.

chromeos/services/assistant/service.cc

@@ -122,21 +122,53 @@ void Service::SuspendDone(const base::TimeDelta& sleep_duration) {
 void Service::OnSessionActivated(bool activated) {
   DCHECK(client_);
   session_active_ = activated;
-  client_->OnAssistantStatusChanged(activated);
+
+  if (assistant_manager_service_->GetState() !=
+      AssistantManagerService::State::RUNNING) {
+    return;
+  }
+
+  client_->OnAssistantStatusChanged(activated /* running */);
   UpdateListeningState();
 }
 
 void Service::OnLockStateChanged(bool locked) {
   locked_ = locked;
+
+  if (assistant_manager_service_->GetState() !=
+      AssistantManagerService::State::RUNNING) {
+    return;
+  }
+
   UpdateListeningState();
 }
 
+void Service::OnVoiceInteractionSettingsEnabled(bool enabled) {
+  settings_enabled_ = enabled;
+  if (enabled && assistant_manager_service_->GetState() ==
+                     AssistantManagerService::State::STOPPED) {
+    // This will eventually trigger the actual start of assistant services
+    // because they all depend on it.
+    RequestAccessToken();
+  } else if (!enabled && assistant_manager_service_->GetState() !=
+                             AssistantManagerService::State::STOPPED) {
+    assistant_manager_service_->Stop();
+    client_->OnAssistantStatusChanged(false /* running */);
+  }
+}
+
 void Service::OnVoiceInteractionHotwordEnabled(bool enabled) {
+  if (hotword_enabled_ == enabled)
+    return;
+  hotword_enabled_ = enabled;
+
   if (assistant_manager_service_->GetState() !=
       AssistantManagerService::State::RUNNING) {
     return;
   }
-  CreateAssistantManagerService(enabled);
+
+  assistant_manager_service_->Stop();
+  client_->OnAssistantStatusChanged(false /* running */);
   RequestAccessToken();
 }
 
@@ -188,9 +220,9 @@ void Service::Init(mojom::ClientPtr client,
       std::make_unique<FakeAssistantManagerServiceImpl>();
 #endif
 
-  // This will eventually trigger the actual start of assistant services because
-  // they all depend on it.
-  RequestAccessToken();
+  voice_interaction_controller_->IsSettingEnabled(
+      base::BindOnce(&Service::OnVoiceInteractionSettingsEnabled,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void Service::GetPrimaryAccountInfoCallback(
@@ -244,19 +276,14 @@ void Service::GetAccessTokenCallback(const base::Optional<std::string>& token,
 }
 
 void Service::CreateAssistantManagerService(bool enable_hotword) {
+  hotword_enabled_ = enable_hotword;
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
-  // TODO(updowndota): Check settings enabled pref when start Assistant.
   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), this, enable_hotword);
 #endif
-}
-
-void Service::FinalizeAssistantManagerService() {
-  DCHECK(assistant_manager_service_->GetState() ==
-         AssistantManagerService::State::RUNNING);
 
   // Bind to Assistant controller in ash.
   context()->connector()->BindInterface(ash::mojom::kServiceName,
@@ -273,9 +300,27 @@ void Service::FinalizeAssistantManagerService() {
       assistant_manager_service_.get()->GetAssistantSettingsManager();
   registry_.AddInterface<mojom::AssistantSettingsManager>(base::BindRepeating(
       &Service::BindAssistantSettingsManager, base::Unretained(this)));
+}
 
-  client_->OnAssistantStatusChanged(true);
+void Service::FinalizeAssistantManagerService() {
+  DCHECK(assistant_manager_service_->GetState() ==
+         AssistantManagerService::State::RUNNING);
+
+  // Double check settings enabled status to avoid racing issue.
+  if (!settings_enabled_) {
+    assistant_manager_service_->Stop();
+    client_->OnAssistantStatusChanged(false /* running */);
+    return;
+  }
+
+  client_->OnAssistantStatusChanged(true /* running */);
+  UpdateListeningState();
   DVLOG(1) << "Assistant is running";
+
+  // Double check hotword status to avoid racing issue.
+  voice_interaction_controller_->IsHotwordEnabled(
+      base::BindOnce(&Service::OnVoiceInteractionHotwordEnabled,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void Service::AddAshSessionObserver() {

chromeos/services/assistant/service.h

@@ -86,7 +86,7 @@ class Service : public service_manager::Service,
   // ash::mojom::VoiceInteractionObserver:
   void OnVoiceInteractionStatusChanged(
       ash::mojom::VoiceInteractionState state) override {}
-  void OnVoiceInteractionSettingsEnabled(bool enabled) override {}
+  void OnVoiceInteractionSettingsEnabled(bool enabled) override;
   void OnVoiceInteractionContextEnabled(bool enabled) override {}
   void OnVoiceInteractionHotwordEnabled(bool enabled) override;
   void OnVoiceInteractionSetupCompleted(bool completed) override {}
@@ -143,6 +143,10 @@ class Service : public service_manager::Service,
   bool session_active_ = false;
   // Whether the lock screen is on.
   bool locked_ = false;
+  // Whether the assistant has been enabled in settings.
+  bool settings_enabled_ = false;
+  // Whether the hotword has been enabled.
+  bool hotword_enabled_ = false;
 
   ash::mojom::AssistantControllerPtr assistant_controller_;
   ash::mojom::VoiceInteractionControllerPtr voice_interaction_controller_;