ambient: Enable ambient mode by default

Bug: b/171071314 Test: Added new test helper and fixes tests. Change-Id: I22c9fe099025a51c999f2d8fbfa12d983f0aa5d1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505670 Commit-Queue: Tao Wu <wutao@chromium.org> Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org> Cr-Commit-Position: refs/heads/master@{#825527}
FairyWorld · Nov 9, 2020 · 92a8f5c · 92a8f5c
1 parent 87903f0
commit 92a8f5c
Show file tree

Hide file tree

Showing 17 changed files with 110 additions and 57 deletions.
diff --git a/ash/BUILD.gn b/ash/BUILD.gn
@@ -2509,6 +2509,8 @@ static_library("test_support") {
     "accessibility/test_accessibility_controller_client.h",
     "ambient/test/ambient_ash_test_base.cc",
     "ambient/test/ambient_ash_test_base.h",
+    "ambient/test/ambient_ash_test_helper.cc",
+    "ambient/test/ambient_ash_test_helper.h",
     "ambient/test/test_ambient_client.cc",
     "ambient/test/test_ambient_client.h",
     "app_list/app_list_test_api.cc",

diff --git a/ash/ambient/ambient_controller.cc b/ash/ambient/ambient_controller.cc
@@ -172,11 +172,9 @@ AmbientController::AmbientController(
   // |SessionController| is initialized before |this| in Shell.
   session_observer_.Add(Shell::Get()->session_controller());
 
-  // Checks the current lid state on initialization.
   auto* power_manager_client = chromeos::PowerManagerClient::Get();
   DCHECK(power_manager_client);
   power_manager_client_observer_.Add(power_manager_client);
-  power_manager_client->RequestStatusUpdate();
 
   ambient_backend_model_observer_.Add(
       ambient_photo_controller_.ambient_backend_model());
@@ -194,7 +192,6 @@ void AmbientController::OnAmbientUiVisibilityChanged(
     AmbientUiVisibility visibility) {
   switch (visibility) {
     case AmbientUiVisibility::kShown:
-
       // Record metrics on ambient mode usage.
       ambient::RecordAmbientModeActivation(
           /*ui_mode=*/LockScreen::HasInstance() ? AmbientUiMode::kLockScreenUi

diff --git a/ash/ambient/test/ambient_ash_test_base.cc b/ash/ambient/test/ambient_ash_test_base.cc
@@ -11,6 +11,7 @@
 #include "ash/ambient/ambient_access_token_controller.h"
 #include "ash/ambient/ambient_constants.h"
 #include "ash/ambient/ambient_photo_controller.h"
+#include "ash/ambient/test/ambient_ash_test_helper.h"
 #include "ash/ambient/ui/ambient_background_image_view.h"
 #include "ash/ambient/ui/ambient_container_view.h"
 #include "ash/ambient/ui/ambient_view_ids.h"
@@ -29,7 +30,6 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/dbus/power_manager/idle.pb.h"
@@ -136,14 +136,6 @@ AmbientAshTestBase::AmbientAshTestBase()
 AmbientAshTestBase::~AmbientAshTestBase() = default;
 
 void AmbientAshTestBase::SetUp() {
-  scoped_feature_list_.InitAndEnableFeatureWithParameters(
-      chromeos::features::kAmbientModeFeature,
-      {{"GeoPhotosEnabled", "true"},
-       {"CapturedOnPixelPhotosEnabled", "false"}});
-  image_downloader_ = std::make_unique<TestImageDownloader>();
-  ambient_client_ = std::make_unique<TestAmbientClient>(&wake_lock_provider_);
-  chromeos::PowerManagerClient::InitializeFake();
-
   AshTestBase::SetUp();
 
   // Need to reset first and then assign the TestPhotoClient because can only
@@ -162,9 +154,6 @@ void AmbientAshTestBase::SetUp() {
 }
 
 void AmbientAshTestBase::TearDown() {
-  ambient_client_.reset();
-  image_downloader_.reset();
-
   AshTestBase::TearDown();
 }
 
@@ -346,7 +335,7 @@ int AmbientAshTestBase::GetNumOfActiveWakeLocks(
     device::mojom::WakeLockType type) {
   base::RunLoop run_loop;
   int result_count = 0;
-  wake_lock_provider_.GetActiveWakeLocksForTests(
+  GetAmbientAshTestHelper()->wake_lock_provider()->GetActiveWakeLocksForTests(
       type, base::BindOnce(
                 [](base::RunLoop* run_loop, int* result_count, int32_t count) {
                   *result_count = count;
@@ -359,11 +348,11 @@ int AmbientAshTestBase::GetNumOfActiveWakeLocks(
 
 void AmbientAshTestBase::IssueAccessToken(const std::string& token,
                                           bool with_error) {
-  ambient_client_->IssueAccessToken(token, with_error);
+  GetAmbientAshTestHelper()->IssueAccessToken(token, with_error);
 }
 
-bool AmbientAshTestBase::IsAccessTokenRequestPending() const {
-  return ambient_client_->IsAccessTokenRequestPending();
+bool AmbientAshTestBase::IsAccessTokenRequestPending() {
+  return GetAmbientAshTestHelper()->IsAccessTokenRequestPending();
 }
 
 base::TimeDelta AmbientAshTestBase::GetRefreshTokenDelay() {

diff --git a/ash/ambient/test/ambient_ash_test_base.h b/ash/ambient/test/ambient_ash_test_base.h
@@ -12,10 +12,7 @@
 #include "ash/ambient/ambient_controller.h"
 #include "ash/ambient/test/test_ambient_client.h"
 #include "ash/ambient/ui/ambient_background_image_view.h"
-#include "ash/public/cpp/test/test_image_downloader.h"
 #include "ash/test/ash_test_base.h"
-#include "base/test/scoped_feature_list.h"
-#include "services/device/public/cpp/test/test_wake_lock_provider.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 #include "ui/views/widget/widget.h"
 
@@ -119,7 +116,7 @@ class AmbientAshTestBase : public AshTestBase {
   // If |with_error| is true, will return an empty access token.
   void IssueAccessToken(const std::string& access_token, bool with_error);
 
-  bool IsAccessTokenRequestPending() const;
+  bool IsAccessTokenRequestPending();
 
   base::TimeDelta GetRefreshTokenDelay();
 
@@ -150,11 +147,6 @@ class AmbientAshTestBase : public AshTestBase {
   void SetImageDecoderImage(const gfx::ImageSkia& image);
 
  private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  std::unique_ptr<TestImageDownloader> image_downloader_;
-
-  device::TestWakeLockProvider wake_lock_provider_;
-  std::unique_ptr<TestAmbientClient> ambient_client_;
   std::unique_ptr<views::Widget> widget_;
 };
 

diff --git a/ash/ambient/test/ambient_ash_test_helper.cc b/ash/ambient/test/ambient_ash_test_helper.cc
@@ -0,0 +1,28 @@
+// Copyright 2020 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/ambient/test/ambient_ash_test_helper.h"
+
+#include "ash/ambient/test/test_ambient_client.h"
+#include "ash/public/cpp/test/test_image_downloader.h"
+
+namespace ash {
+
+AmbientAshTestHelper::AmbientAshTestHelper() {
+  image_downloader_ = std::make_unique<TestImageDownloader>();
+  ambient_client_ = std::make_unique<TestAmbientClient>(&wake_lock_provider_);
+}
+
+AmbientAshTestHelper::~AmbientAshTestHelper() = default;
+
+void AmbientAshTestHelper::IssueAccessToken(const std::string& token,
+                                            bool with_error) {
+  ambient_client_->IssueAccessToken(token, with_error);
+}
+
+bool AmbientAshTestHelper::IsAccessTokenRequestPending() const {
+  return ambient_client_->IsAccessTokenRequestPending();
+}
+
+}  // namespace ash
diff --git a/ash/ambient/test/ambient_ash_test_helper.h b/ash/ambient/test/ambient_ash_test_helper.h
@@ -0,0 +1,41 @@
+// Copyright 2020 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_AMBIENT_TEST_AMBIENT_ASH_TEST_HELPER_H_
+#define ASH_AMBIENT_TEST_AMBIENT_ASH_TEST_HELPER_H_
+
+#include <memory>
+
+#include "services/device/public/cpp/test/test_wake_lock_provider.h"
+
+namespace ash {
+
+class TestImageDownloader;
+class TestAmbientClient;
+
+// The helper class to test the Ambient Mode in Ash.
+class AmbientAshTestHelper {
+ public:
+  AmbientAshTestHelper();
+  ~AmbientAshTestHelper();
+
+  // Simulate to issue an |access_token|.
+  // If |with_error| is true, will return an empty access token.
+  void IssueAccessToken(const std::string& access_token, bool with_error);
+
+  bool IsAccessTokenRequestPending() const;
+
+  device::TestWakeLockProvider* wake_lock_provider() {
+    return &wake_lock_provider_;
+  }
+
+ private:
+  std::unique_ptr<TestImageDownloader> image_downloader_;
+  device::TestWakeLockProvider wake_lock_provider_;
+  std::unique_ptr<TestAmbientClient> ambient_client_;
+};
+
+}  // namespace ash
+
+#endif  // ASH_AMBIENT_TEST_AMBIENT_ASH_TEST_HELPER_H_
diff --git a/ash/assistant/test/assistant_ash_test_base.cc b/ash/assistant/test/assistant_ash_test_base.cc
@@ -18,7 +18,6 @@
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/cpp/assistant/controller/assistant_ui_controller.h"
 #include "ash/public/cpp/test/assistant_test_api.h"
-#include "ash/public/cpp/test/test_image_downloader.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_helper.h"
@@ -105,7 +104,6 @@ AssistantAshTestBase::AssistantAshTestBase(
       test_api_(AssistantTestApi::Create()),
       test_setup_(std::make_unique<TestAssistantSetup>()),
       test_web_view_factory_(std::make_unique<TestAssistantWebViewFactory>()),
-      test_image_downloader_(std::make_unique<TestImageDownloader>()),
       assistant_client_(std::make_unique<TestAssistantClient>()) {}
 
 AssistantAshTestBase::~AssistantAshTestBase() = default;

diff --git a/ash/assistant/test/assistant_ash_test_base.h b/ash/assistant/test/assistant_ash_test_base.h
@@ -35,7 +35,6 @@ class TestAssistantClient;
 class TestAssistantService;
 class TestAssistantSetup;
 class TestAssistantWebViewFactory;
-class TestImageDownloader;
 
 // Helper class to make testing the Assistant Ash UI easier.
 class AssistantAshTestBase : public AshTestBase {
@@ -209,7 +208,6 @@ class AssistantAshTestBase : public AshTestBase {
   std::unique_ptr<AssistantTestApi> test_api_;
   std::unique_ptr<TestAssistantSetup> test_setup_;
   std::unique_ptr<TestAssistantWebViewFactory> test_web_view_factory_;
-  std::unique_ptr<TestImageDownloader> test_image_downloader_;
 
   std::vector<std::unique_ptr<aura::Window>> windows_;
   std::vector<std::unique_ptr<views::Widget>> widgets_;

diff --git a/ash/test/ash_test_base.cc b/ash/test/ash_test_base.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "ash/accessibility/accessibility_panel_layout_manager.h"
+#include "ash/ambient/test/ambient_ash_test_helper.h"
 #include "ash/app_list/test/app_list_test_helper.h"
 #include "ash/display/extended_mouse_warp_controller.h"
 #include "ash/display/mouse_cursor_event_filter.h"
@@ -382,6 +383,10 @@ AppListTestHelper* AshTestBase::GetAppListTestHelper() {
   return ash_test_helper_->app_list_test_helper();
 }
 
+AmbientAshTestHelper* AshTestBase::GetAmbientAshTestHelper() {
+  return ash_test_helper_->ambient_ash_test_helper();
+}
+
 void AshTestBase::CreateUserSessions(int n) {
   GetSessionControllerClient()->CreatePredefinedUserSessions(n);
 }

diff --git a/ash/test/ash_test_base.h b/ash/test/ash_test_base.h
@@ -65,6 +65,7 @@ class WidgetDelegate;
 
 namespace ash {
 
+class AmbientAshTestHelper;
 class AppListTestHelper;
 class AshTestHelper;
 class Shelf;
@@ -235,6 +236,8 @@ class AshTestBase : public testing::Test {
 
   AppListTestHelper* GetAppListTestHelper();
 
+  AmbientAshTestHelper* GetAmbientAshTestHelper();
+
   // Emulates an ash session that have |session_count| user sessions running.
   // Note that existing user sessions will be cleared.
   void CreateUserSessions(int session_count);

diff --git a/ash/test/ash_test_helper.cc b/ash/test/ash_test_helper.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "ash/accelerometer/accelerometer_reader.h"
+#include "ash/ambient/test/ambient_ash_test_helper.h"
 #include "ash/app_list/test/app_list_test_helper.h"
 #include "ash/assistant/assistant_controller_impl.h"
 #include "ash/assistant/test/test_assistant_service.h"
@@ -141,6 +142,8 @@ void AshTestHelper::SetUp() {
 }
 
 void AshTestHelper::TearDown() {
+  ambient_ash_test_helper_.reset();
+
   // The AppListTestHelper holds a pointer to the AppListController the Shell
   // owns, so shut the test helper down first.
   app_list_test_helper_.reset();
@@ -225,6 +228,8 @@ void AshTestHelper::SetUp(InitParams init_params) {
   if (!views::ViewsDelegate::GetInstance())
     test_views_delegate_ = MakeTestViewsDelegate();
 
+  ambient_ash_test_helper_ = std::make_unique<AmbientAshTestHelper>();
+
   ShellInitParams shell_init_params;
   shell_init_params.delegate = std::move(init_params.delegate);
   if (!shell_init_params.delegate)

diff --git a/ash/test/ash_test_helper.h b/ash/test/ash_test_helper.h
@@ -11,7 +11,6 @@
 #include <utility>
 
 #include "ash/assistant/test/test_assistant_service.h"
-#include "ash/public/cpp/test/test_image_downloader.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"
@@ -44,6 +43,7 @@ class TestViewsDelegate;
 namespace ash {
 
 class AppListTestHelper;
+class AmbientAshTestHelper;
 class TestKeyboardControllerObserver;
 class TestNewWindowDelegate;
 
@@ -130,6 +130,10 @@ class AshTestHelper : public aura::test::AuraTestHelper {
     return assistant_service_.get();
   }
 
+  AmbientAshTestHelper* ambient_ash_test_helper() {
+    return ambient_ash_test_helper_.get();
+  }
+
  private:
   // Scoping objects to manage init/teardown of services.
   class BluezDBusManagerInitializer;
@@ -159,6 +163,7 @@ class AshTestHelper : public aura::test::AuraTestHelper {
   std::unique_ptr<TestSessionControllerClient> session_controller_client_;
   std::unique_ptr<TestKeyboardControllerObserver>
       test_keyboard_controller_observer_;
+  std::unique_ptr<AmbientAshTestHelper> ambient_ash_test_helper_;
 
   DISALLOW_COPY_AND_ASSIGN(AshTestHelper);
 };

diff --git a/chrome/browser/ui/ash/ambient/ambient_client_impl_unittest.cc b/chrome/browser/ui/ash/ambient/ambient_client_impl_unittest.cc
@@ -6,33 +6,27 @@
 
 #include <memory>
 
-#include "ash/public/cpp/ambient/ambient_client.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
-#include "chrome/test/base/chrome_ash_test_base.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "components/user_manager/scoped_user_manager.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 constexpr char kTestProfileName[] = "user@gmail.com";
 constexpr char kTestGaiaId[] = "1234567890";
 
-class AmbientClientImplTest : public ChromeAshTestBase {
+class AmbientClientImplTest : public testing::Test {
  public:
   AmbientClientImplTest() = default;
   ~AmbientClientImplTest() override = default;
 
   void SetUp() override {
-    scoped_feature_list_.InitAndEnableFeature(
-        chromeos::features::kAmbientModeFeature);
-    // Needed by ash.
     ambient_client_ = std::make_unique<AmbientClientImpl>();
-
     ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
     profile_manager_ = std::make_unique<TestingProfileManager>(
         TestingBrowserProcess::GetGlobal());
@@ -47,8 +41,6 @@ class AmbientClientImplTest : public ChromeAshTestBase {
         std::make_unique<IdentityTestEnvironmentProfileAdaptor>(profile_);
     user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
         std::make_unique<chromeos::FakeChromeUserManager>());
-
-    AshTestBase::SetUp();
   }
 
   void TearDown() override {
@@ -58,7 +50,6 @@ class AmbientClientImplTest : public ChromeAshTestBase {
     profile_ = nullptr;
     profile_manager_->DeleteTestingProfile(kTestProfileName);
     profile_manager_.reset();
-    AshTestBase::TearDown();
   }
 
  protected:
@@ -89,7 +80,7 @@ class AmbientClientImplTest : public ChromeAshTestBase {
     }
   }
 
-  base::test::ScopedFeatureList scoped_feature_list_;
+  content::BrowserTaskEnvironment task_environment_;
   base::ScopedTempDir data_dir_;
   std::unique_ptr<TestingProfileManager> profile_manager_;
   // Owned by |profile_manager_|

diff --git a/chrome/browser/ui/webui/settings/chromeos/personalization_section.cc b/chrome/browser/ui/webui/settings/chromeos/personalization_section.cc
@@ -123,7 +123,9 @@ const std::vector<SearchConcept>& GetAmbientModeOffSearchConcepts() {
 }
 
 bool IsAmbientModeAllowed() {
+  // TODO(b/172029925): Set up to test this code.
   return chromeos::features::IsAmbientModeEnabled() &&
+         ash::AmbientClient::Get() &&
          ash::AmbientClient::Get()->IsAmbientModeAllowed();
 }