Wire up the prototyping ambient UI.

This CL:
1. Added a new class AmbientModeState to store states related to this
mode.
2. Wired up AssistantManagerServiceImpl to observe the new state change in
order to update action module as necessary.

Misc:
Fixed linter warnings.

Bug: b/139698911
Test: run unittests added in this change
Change-Id: I4dd078c19cc2ce73a8262fe3cfc0c39308949e82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906932
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721894}

ash/ambient/ambient_controller.cc

@@ -10,6 +10,7 @@
 #include "ash/ambient/util/ambient_util.h"
 #include "ash/assistant/assistant_controller.h"
 #include "ash/login/ui/lock_screen.h"
+#include "ash/public/cpp/ambient/ambient_mode_state.h"
 #include "ash/public/cpp/ambient/photo_controller.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "ui/views/widget/widget.h"
@@ -26,9 +27,13 @@ bool CanStartAmbientMode() {
 }  // namespace
 
 AmbientController::AmbientController(AssistantController* assistant_controller)
-    : assistant_controller_(assistant_controller) {}
+    : assistant_controller_(assistant_controller) {
+  ambient_state_.AddObserver(this);
+}
 
 AmbientController::~AmbientController() {
+  ambient_state_.RemoveObserver(this);
+
   DestroyContainerView();
 }
 
@@ -41,6 +46,21 @@ void AmbientController::OnWidgetDestroying(views::Widget* widget) {
   // If Assistant UI was already closed, this is a no-op.
   assistant_controller_->ui_controller()->CloseUi(
       AssistantExitPoint::kUnspecified);
+
+  // We need to update the mode when the widget gets destroyed as this may have
+  // caused by AmbientContainerView directly closed the widget without calling
+  // Stop() after an outside press.
+  ambient_state_.SetAmbientModeEnabled(false);
+}
+
+void AmbientController::OnAmbientModeEnabled(bool enabled) {
+  if (enabled) {
+    CreateContainerView();
+    container_view_->GetWidget()->Show();
+    RefreshImage();
+  } else {
+    DestroyContainerView();
+  }
 }
 
 void AmbientController::Toggle() {
@@ -64,20 +84,18 @@ void AmbientController::Start() {
     return;
   }
 
-  // CloseUi to ensure standalone Assistant Ui doesn't exist when entering
-  // Ambient mode to avoid strange behavior caused by standalone Ui was
-  // only hidden at that time. This will be a no-op if Ui was already closed.
-  // TODO(meilinw): Handle embedded Ui.
+  // CloseUi to ensure standalone Assistant UI doesn't exist when entering
+  // Ambient mode to avoid strange behavior caused by standalone UI was
+  // only hidden at that time. This will be a no-op if UI was already closed.
+  // TODO(meilinw): Handle embedded UI.
   assistant_controller_->ui_controller()->CloseUi(
       AssistantExitPoint::kUnspecified);
 
-  CreateContainerView();
-  container_view_->GetWidget()->Show();
-  RefreshImage();
+  ambient_state_.SetAmbientModeEnabled(true);
 }
 
 void AmbientController::Stop() {
-  DestroyContainerView();
+  ambient_state_.SetAmbientModeEnabled(false);
 }
 
 void AmbientController::CreateContainerView() {

ash/ambient/ambient_controller.h

@@ -7,6 +7,7 @@
 
 #include "ash/ambient/model/photo_model.h"
 #include "ash/ash_export.h"
+#include "ash/public/cpp/ambient/ambient_mode_state.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
@@ -23,7 +24,8 @@ class AssistantController;
 class PhotoModelObserver;
 
 // Class to handle all ambient mode functionalities.
-class ASH_EXPORT AmbientController : views::WidgetObserver {
+class ASH_EXPORT AmbientController : public views::WidgetObserver,
+                                     public AmbientModeStateObserver {
  public:
   explicit AmbientController(AssistantController* assistant_controller);
   ~AmbientController() override;
@@ -31,10 +33,12 @@ class ASH_EXPORT AmbientController : views::WidgetObserver {
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
+  // AmbientModeStateObserver:
+  void OnAmbientModeEnabled(bool enabled) override;
+
   void Toggle();
 
   void AddPhotoModelObserver(PhotoModelObserver* observer);
-
   void RemovePhotoModelObserver(PhotoModelObserver* observer);
 
   const PhotoModel& model() const { return model_; }
@@ -64,6 +68,7 @@ class ASH_EXPORT AmbientController : views::WidgetObserver {
   AssistantController* const assistant_controller_;  // Owned by Shell.
   AmbientContainerView* container_view_ = nullptr;   // Owned by view hierarchy.
   PhotoModel model_;
+  AmbientModeState ambient_state_;
   base::OneShotTimer refresh_timer_;
   base::WeakPtrFactory<AmbientController> weak_factory_{this};
 

ash/ambient/ui/ambient_container_view_unittest.cc

@@ -24,26 +24,6 @@
 
 namespace ash {
 
-namespace {
-
-class TestPhotoController : public PhotoController {
- public:
-  TestPhotoController() = default;
-  ~TestPhotoController() override = default;
-
-  // PhotoController:
-  void GetNextImage(PhotoController::PhotoDownloadCallback callback) override {
-    gfx::ImageSkia image =
-        gfx::test::CreateImageSkia(/*width=*/10, /*height=*/10);
-    std::move(callback).Run(image);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestPhotoController);
-};
-
-}  // namespace
-
 class AmbientContainerViewTest : public AshTestBase {
  public:
   AmbientContainerViewTest()
@@ -53,7 +33,6 @@ class AmbientContainerViewTest : public AshTestBase {
   void SetUp() override {
     scoped_feature_list_.InitAndEnableFeature(
         chromeos::features::kAmbientModeFeature);
-    photo_controller_ = std::make_unique<TestPhotoController>();
     AshTestBase::SetUp();
   }
 
@@ -73,7 +52,6 @@ class AmbientContainerViewTest : public AshTestBase {
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;
-  std::unique_ptr<TestPhotoController> photo_controller_;
 
   DISALLOW_COPY_AND_ASSIGN(AmbientContainerViewTest);
 };

ash/public/cpp/BUILD.gn

@@ -17,6 +17,8 @@ component("cpp") {
     "accessibility_focus_ring_controller.h",
     "accessibility_focus_ring_info.cc",
     "accessibility_focus_ring_info.h",
+    "ambient/ambient_mode_state.cc",
+    "ambient/ambient_mode_state.h",
     "ambient/photo_controller.cc",
     "ambient/photo_controller.h",
     "android_intent_helper.cc",
@@ -318,6 +320,8 @@ source_set("test_support") {
     "test/test_keyboard_controller_observer.h",
     "test/test_new_window_delegate.cc",
     "test/test_new_window_delegate.h",
+    "test/test_photo_controller.cc",
+    "test/test_photo_controller.h",
     "test/test_system_tray_client.cc",
     "test/test_system_tray_client.h",
   ]
@@ -329,6 +333,7 @@ source_set("test_support") {
     "//ui/aura",
     "//ui/aura:test_support",
     "//ui/gfx",
+    "//ui/gfx:test_support",
     "//ui/views",
   ]
 }

ash/public/cpp/ambient/ambient_mode_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 "ash/public/cpp/ambient/ambient_mode_state.h"
+
+namespace ash {
+
+namespace {
+
+AmbientModeState* g_ambient_mode_state = nullptr;
+
+}  // namespace
+
+// static
+AmbientModeState* AmbientModeState::Get() {
+  return g_ambient_mode_state;
+}
+
+AmbientModeState::AmbientModeState() {
+  DCHECK(!g_ambient_mode_state);
+  g_ambient_mode_state = this;
+}
+
+AmbientModeState::~AmbientModeState() {
+  DCHECK_EQ(g_ambient_mode_state, this);
+  g_ambient_mode_state = nullptr;
+}
+
+void AmbientModeState::AddObserver(AmbientModeStateObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void AmbientModeState::RemoveObserver(AmbientModeStateObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void AmbientModeState::SetAmbientModeEnabled(bool enabled) {
+  if (enabled_ == enabled)
+    return;
+
+  enabled_ = enabled;
+  NotifyAmbientModeEnabled();
+}
+
+void AmbientModeState::NotifyAmbientModeEnabled() {
+  for (auto& observer : observers_)
+    observer.OnAmbientModeEnabled(enabled_);
+}
+
+}  // namespace ash

ash/public/cpp/ambient/ambient_mode_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 ASH_PUBLIC_CPP_AMBIENT_AMBIENT_MODE_STATE_H_
+#define ASH_PUBLIC_CPP_AMBIENT_AMBIENT_MODE_STATE_H_
+
+#include "ash/public/cpp/ash_public_export.h"
+#include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
+
+namespace ash {
+
+// An interface implemented by classes who want to be notified about ambient
+// state change event.
+class ASH_PUBLIC_EXPORT AmbientModeStateObserver
+    : public base::CheckedObserver {
+ public:
+  // Invoked when the Ambient Mode has been enabled/disabled by user
+  // entering/exiting this mode.
+  virtual void OnAmbientModeEnabled(bool enabled) = 0;
+};
+
+// A class that stores Ambient Mode related states.
+class ASH_PUBLIC_EXPORT AmbientModeState {
+ public:
+  static AmbientModeState* Get();
+
+  AmbientModeState();
+  AmbientModeState(const AmbientModeState&) = delete;
+  AmbientModeState& operator=(AmbientModeState&) = delete;
+  ~AmbientModeState();
+
+  void AddObserver(AmbientModeStateObserver* observer);
+  void RemoveObserver(AmbientModeStateObserver* observer);
+
+  // Sets the enabled/disabled state for Ambient Mode.
+  void SetAmbientModeEnabled(bool enabled);
+
+  // Returns the enabled/disabled state for Ambient Mode.
+  bool enabled() const { return enabled_; }
+
+ private:
+  void NotifyAmbientModeEnabled();
+
+  // Whether Ambient Mode is enabled.
+  bool enabled_ = false;
+
+  base::ObserverList<AmbientModeStateObserver> observers_;
+};
+
+}  // namespace ash
+
+#endif  // ASH_PUBLIC_CPP_AMBIENT_AMBIENT_MODE_STATE_H_

ash/public/cpp/assistant/assistant_state_base.h

@@ -5,6 +5,7 @@
 #ifndef ASH_PUBLIC_CPP_ASSISTANT_ASSISTANT_STATE_BASE_H_
 #define ASH_PUBLIC_CPP_ASSISTANT_ASSISTANT_STATE_BASE_H_
 
+#include <memory>
 #include <string>
 
 #include "ash/public/mojom/assistant_state_controller.mojom.h"

ash/public/cpp/test/test_photo_controller.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 "ash/public/cpp/test/test_photo_controller.h"
+
+#include <utility>
+
+#include "ash/public/cpp/ambient/photo_controller.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_unittest_util.h"
+
+namespace ash {
+
+TestPhotoController::TestPhotoController() = default;
+TestPhotoController::~TestPhotoController() = default;
+
+void TestPhotoController::GetNextImage(
+    PhotoController::PhotoDownloadCallback callback) {
+  gfx::ImageSkia image =
+      gfx::test::CreateImageSkia(/*width=*/10, /*height=*/10);
+  std::move(callback).Run(image);
+}
+
+}  // namespace ash

ash/public/cpp/test/test_photo_controller.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 ASH_PUBLIC_CPP_TEST_TEST_PHOTO_CONTROLLER_H_
+#define ASH_PUBLIC_CPP_TEST_TEST_PHOTO_CONTROLLER_H_
+
+#include "ash/public/cpp/ambient/photo_controller.h"
+#include "base/macros.h"
+
+namespace ash {
+
+class ASH_PUBLIC_EXPORT TestPhotoController : public PhotoController {
+ public:
+  TestPhotoController();
+  TestPhotoController(const TestPhotoController&) = delete;
+  TestPhotoController& operator=(TestPhotoController&) = delete;
+  ~TestPhotoController() override;
+
+  // PhotoController:
+  void GetNextImage(PhotoController::PhotoDownloadCallback callback) override;
+};
+
+}  // namespace ash
+
+#endif  // ASH_PUBLIC_CPP_TEST_TEST_PHOTO_CONTROLLER_H_

ash/test/ash_test_helper.cc

@@ -20,6 +20,7 @@
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/public/cpp/test/test_keyboard_controller_observer.h"
 #include "ash/public/cpp/test/test_new_window_delegate.h"
+#include "ash/public/cpp/test/test_photo_controller.h"
 #include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/session/test_pref_service_provider.h"
 #include "ash/session/test_session_controller_client.h"
@@ -179,6 +180,8 @@ void AshTestHelper::SetUp(const InitParams& init_params,
   system_tray_client_ = std::make_unique<TestSystemTrayClient>();
   shell->system_tray_model()->SetClient(system_tray_client_.get());
 
+  photo_controller_ = std::make_unique<TestPhotoController>();
+
   if (init_params.start_session)
     session_controller_client_->CreatePredefinedUserSessions(1);
 

ash/test/ash_test_helper.h

@@ -52,6 +52,7 @@ class TestNotifierSettingsController;
 class TestPrefServiceProvider;
 class TestShellDelegate;
 class TestSystemTrayClient;
+class TestPhotoController;
 
 // A helper class that does common initialization required for Ash. Creates a
 // root window and an ash::Shell instance with a test delegate.
@@ -160,6 +161,7 @@ class AshTestHelper {
   std::unique_ptr<TestPrefServiceProvider> prefs_provider_;
   std::unique_ptr<TestAssistantService> assistant_service_;
   std::unique_ptr<ui::TestContextFactories> context_factories_;
+  std::unique_ptr<TestPhotoController> photo_controller_;
 
   std::unique_ptr<base::test::ScopedCommandLine> command_line_;
 

chrome/browser/ui/ash/assistant/assistant_state_client_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/ash/assistant/assistant_state_client.h"
 
+#include <memory>
+
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/mojom/assistant_state_controller.mojom.h"
 #include "base/bind.h"

chromeos/services/assistant/assistant_manager_service.h

@@ -55,7 +55,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerService
   // Stop the assistant.
   virtual void Stop() = 0;
 
-  // Returns the current state.
+  // Return the current state.
   virtual State GetState() const = 0;
 
   // Set access token for assistant.
@@ -67,13 +67,14 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerService
   // Turn on / off hotword listening.
   virtual void EnableHotword(bool enable) = 0;
 
+  // Enable/disable ARC play store.
   virtual void SetArcPlayStoreEnabled(bool enabled) = 0;
 
-  // Returns a pointer of AssistantSettingsManager.
+  // Return a pointer of AssistantSettingsManager.
   virtual AssistantSettingsManager* GetAssistantSettingsManager() = 0;
 
-  // Add/Remove an observer that is invoked when there is a communucation error
-  // with the Assistant service.
+  // Add/Remove an observer that is invoked when there is a communication
+  // error with the Assistant service.
   virtual void AddCommunicationErrorObserver(
       CommunicationErrorObserver* observer) = 0;
   virtual void RemoveCommunicationErrorObserver(

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -25,6 +25,7 @@
 #include "chromeos/assistant/internal/internal_util.h"
 #include "chromeos/assistant/internal/proto/google3/assistant/api/client_input/warmer_welcome_input.pb.h"
 #include "chromeos/assistant/internal/proto/google3/assistant/api/client_op/device_args.pb.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/dbus/util/version_loader.h"
 #include "chromeos/services/assistant/assistant_manager_service_delegate.h"
 #include "chromeos/services/assistant/constants.h"
@@ -177,6 +178,10 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
 }
 
 AssistantManagerServiceImpl::~AssistantManagerServiceImpl() {
+  auto* ambient_state = ash::AmbientModeState::Get();
+  if (ambient_state)
+    ambient_state->RemoveObserver(this);
+
   background_thread_.Stop();
 }
 
@@ -193,6 +198,17 @@ void AssistantManagerServiceImpl::Start(
 
   EnableHotword(enable_hotword);
 
+  if (chromeos::features::IsAmbientModeEnabled()) {
+    auto* ambient_state = ash::AmbientModeState::Get();
+    DCHECK(ambient_state);
+
+    // Update the support action list in action module when system enters/exits
+    // the Ambient Mode. Some actions such as open URL in the browser will be
+    // disabled in this mode.
+    action_module_->SetAmbientModeEnabled(ambient_state->enabled());
+    ambient_state->AddObserver(this);
+  }
+
   // LibAssistant creation will make file IO and sync wait. Post the creation to
   // background thread to avoid DCHECK.
   background_thread_.task_runner()->PostTaskAndReply(
@@ -209,6 +225,12 @@ void AssistantManagerServiceImpl::Stop() {
 
   SetStateAndInformObservers(State::STOPPED);
 
+  if (chromeos::features::IsAmbientModeEnabled()) {
+    auto* ambient_state = ash::AmbientModeState::Get();
+    DCHECK(ambient_state);
+    ambient_state->RemoveObserver(this);
+  }
+
   // When user disables the feature, we also deletes all data.
   if (!assistant_state()->settings_enabled().value() && assistant_manager_)
     assistant_manager_->ResetAllDataAndShutdown();
@@ -1315,6 +1337,10 @@ void AssistantManagerServiceImpl::OnAndroidAppListRefreshed(
   display_connection_->OnAndroidAppListRefreshed(android_apps_info);
 }
 
+void AssistantManagerServiceImpl::OnAmbientModeEnabled(bool enabled) {
+  action_module_->SetAmbientModeEnabled(enabled);
+}
+
 void AssistantManagerServiceImpl::UpdateInternalOptions(
     assistant_client::AssistantManagerInternal* assistant_manager_internal) {
   // Build internal options

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/public/cpp/ambient/ambient_mode_state.h"
 #include "ash/public/mojom/assistant_controller.mojom.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/synchronization/lock.h"
@@ -94,7 +95,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
       public assistant_client::DeviceStateListener,
       public assistant_client::MediaManager::Listener,
       public media_session::mojom::MediaControllerObserver,
-      public mojom::AppListEventSubscriber {
+      public mojom::AppListEventSubscriber,
+      public ash::AmbientModeStateObserver {
  public:
   // |service| owns this class and must outlive this class.
   AssistantManagerServiceImpl(
@@ -107,7 +109,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
 
   ~AssistantManagerServiceImpl() override;
 
-  // assistant::AssistantManagerService overrides
+  // assistant::AssistantManagerService overrides:
   void Start(const base::Optional<std::string>& access_token,
              bool enable_hotword) override;
   void Stop() override;
@@ -181,7 +183,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
           recognition_result) override;
   void OnRespondingStarted(bool is_error_response) override;
 
-  // AssistantManagerDelegate overrides
+  // AssistantManagerDelegate overrides:
   assistant_client::ActionModule::Result HandleModifySettingClientOp(
       const std::string& modify_setting_args_proto) override;
   bool IsSettingSupported(const std::string& setting_id) override;
@@ -200,6 +202,9 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   void OnAndroidAppListRefreshed(
       std::vector<mojom::AndroidAppInfoPtr> apps_info) override;
 
+  // ash::AmbientModeStateObserver overrides:
+  void OnAmbientModeEnabled(bool enabled) override;
+
   void UpdateInternalOptions(
       assistant_client::AssistantManagerInternal* assistant_manager_internal);
 

chromeos/services/assistant/assistant_state_proxy.h

@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_STATE_PROXY_H_
 #define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_STATE_PROXY_H_
 
+#include <memory>
 #include <string>
 
 #include "ash/public/cpp/assistant/assistant_state_base.h"

chromeos/services/assistant/service_unittest.cc

@@ -236,7 +236,6 @@ class AssistantServiceTest : public testing::Test {
   mojo::Remote<mojom::AssistantService> remote_service_;
 
   FullyInitializedAssistantState assistant_state_;
-
   FakeIdentityAccessor fake_identity_accessor_;
   FakeAssistantClient fake_assistant_client_{&assistant_state_};
   FakeDeviceActions fake_device_actions_;