From 92a8f5c7763f9050ca285777583aae2a565f4e5f Mon Sep 17 00:00:00 2001 From: wutao Date: Mon, 9 Nov 2020 23:25:54 +0000 Subject: [PATCH] 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 Reviewed-by: Jimmy Gong Reviewed-by: Xiyuan Xia Reviewed-by: Xiaohui Chen Cr-Commit-Position: refs/heads/master@{#825527} --- ash/BUILD.gn | 2 + ash/ambient/ambient_controller.cc | 3 -- ash/ambient/test/ambient_ash_test_base.cc | 21 +++------- ash/ambient/test/ambient_ash_test_base.h | 10 +---- ash/ambient/test/ambient_ash_test_helper.cc | 28 +++++++++++++ ash/ambient/test/ambient_ash_test_helper.h | 41 +++++++++++++++++++ ash/assistant/test/assistant_ash_test_base.cc | 2 - ash/assistant/test/assistant_ash_test_base.h | 2 - ash/test/ash_test_base.cc | 5 +++ ash/test/ash_test_base.h | 3 ++ ash/test/ash_test_helper.cc | 5 +++ ash/test/ash_test_helper.h | 7 +++- .../ambient/ambient_client_impl_unittest.cc | 17 ++------ .../chromeos/personalization_section.cc | 2 + chromeos/constants/chromeos_features.cc | 2 +- .../assistant_manager_service_impl.cc | 9 ++-- .../services/assistant/service_unittest.cc | 8 +--- 17 files changed, 110 insertions(+), 57 deletions(-) create mode 100644 ash/ambient/test/ambient_ash_test_helper.cc create mode 100644 ash/ambient/test/ambient_ash_test_helper.h diff --git a/ash/BUILD.gn b/ash/BUILD.gn index 7a93fe9d142645..b8b155784cf3d5 100644 --- 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 index 3154aa9da940ea..94ae4b86bf392d 100644 --- 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 index 83196136e03690..531def4ef90a26 100644 --- 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(); - ambient_client_ = std::make_unique(&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 index 8ca2bec8282a38..8fe3112240146e 100644 --- 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 image_downloader_; - - device::TestWakeLockProvider wake_lock_provider_; - std::unique_ptr ambient_client_; std::unique_ptr widget_; }; diff --git a/ash/ambient/test/ambient_ash_test_helper.cc b/ash/ambient/test/ambient_ash_test_helper.cc new file mode 100644 index 00000000000000..c8ee5486b1bf34 --- /dev/null +++ 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(); + ambient_client_ = std::make_unique(&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 new file mode 100644 index 00000000000000..7ec665b32600ab --- /dev/null +++ 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 + +#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 image_downloader_; + device::TestWakeLockProvider wake_lock_provider_; + std::unique_ptr 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 index 794170867c23bc..53f984c10c336c 100644 --- 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()), test_web_view_factory_(std::make_unique()), - test_image_downloader_(std::make_unique()), assistant_client_(std::make_unique()) {} AssistantAshTestBase::~AssistantAshTestBase() = default; diff --git a/ash/assistant/test/assistant_ash_test_base.h b/ash/assistant/test/assistant_ash_test_base.h index 6c7c2ba90651da..e8b7ff0c87c2dc 100644 --- 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 test_api_; std::unique_ptr test_setup_; std::unique_ptr test_web_view_factory_; - std::unique_ptr test_image_downloader_; std::vector> windows_; std::vector> widgets_; diff --git a/ash/test/ash_test_base.cc b/ash/test/ash_test_base.cc index 6cae1bcffafb59..0a4c727ced6f43 100644 --- a/ash/test/ash_test_base.cc +++ b/ash/test/ash_test_base.cc @@ -10,6 +10,7 @@ #include #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 index 3e7377c2c1ee83..826690d560348c 100644 --- 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 index 4bfeefa56a363b..24fe44cc97a986 100644 --- a/ash/test/ash_test_helper.cc +++ b/ash/test/ash_test_helper.cc @@ -7,6 +7,7 @@ #include #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(); + 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 index 1704c8322211b0..1e0d1d93758bda 100644 --- a/ash/test/ash_test_helper.h +++ b/ash/test/ash_test_helper.h @@ -11,7 +11,6 @@ #include #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 session_controller_client_; std::unique_ptr test_keyboard_controller_observer_; + std::unique_ptr 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 index 7cf42137409b2b..4445e52aef525d 100644 --- 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 -#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(); - ASSERT_TRUE(data_dir_.CreateUniqueTempDir()); profile_manager_ = std::make_unique( TestingBrowserProcess::GetGlobal()); @@ -47,8 +41,6 @@ class AmbientClientImplTest : public ChromeAshTestBase { std::make_unique(profile_); user_manager_enabler_ = std::make_unique( std::make_unique()); - - 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 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 index 7b9f1684af72e9..0b292d43ddc2d7 100644 --- 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& GetAmbientModeOffSearchConcepts() { } bool IsAmbientModeAllowed() { + // TODO(b/172029925): Set up to test this code. return chromeos::features::IsAmbientModeEnabled() && + ash::AmbientClient::Get() && ash::AmbientClient::Get()->IsAmbientModeAllowed(); } diff --git a/chromeos/constants/chromeos_features.cc b/chromeos/constants/chromeos_features.cc index 192a053527be1b..617857f011be59 100644 --- a/chromeos/constants/chromeos_features.cc +++ b/chromeos/constants/chromeos_features.cc @@ -26,7 +26,7 @@ const base::Feature kAllowScrollSettings{"AllowScrollSettings", // Controls whether to enable Ambient mode feature. const base::Feature kAmbientModeFeature{"ChromeOSAmbientMode", - base::FEATURE_DISABLED_BY_DEFAULT}; + base::FEATURE_ENABLED_BY_DEFAULT}; constexpr base::FeatureParam kAmbientModeCapturedOnPixelAlbumEnabled{ &kAmbientModeFeature, "CapturedOnPixelAlbumEnabled", false}; diff --git a/chromeos/services/assistant/assistant_manager_service_impl.cc b/chromeos/services/assistant/assistant_manager_service_impl.cc index 038e24eb2bdc33..fe156fe4732143 100644 --- a/chromeos/services/assistant/assistant_manager_service_impl.cc +++ b/chromeos/services/assistant/assistant_manager_service_impl.cc @@ -212,10 +212,11 @@ void AssistantManagerServiceImpl::Start(const base::Optional& user, // Check the AmbientModeState to keep us synced on |ambient_state|. if (chromeos::features::IsAmbientModeEnabled()) { auto* model = ash::AmbientUiModel::Get(); - DCHECK(model); - - EnableAmbientMode(model->ui_visibility() != - ash::AmbientUiVisibility::kClosed); + // Could be nullptr in test. + if (model) { + EnableAmbientMode(model->ui_visibility() != + ash::AmbientUiVisibility::kClosed); + } } InitAssistant(user, assistant_state()->locale().value()); diff --git a/chromeos/services/assistant/service_unittest.cc b/chromeos/services/assistant/service_unittest.cc index 77a0dfb75a930c..ed38d1cd6d2aba 100644 --- a/chromeos/services/assistant/service_unittest.cc +++ b/chromeos/services/assistant/service_unittest.cc @@ -21,7 +21,6 @@ #include "base/time/time.h" #include "base/timer/timer.h" #include "chromeos/audio/cras_audio_handler.h" -#include "chromeos/constants/chromeos_features.h" #include "chromeos/dbus/power/fake_power_manager_client.h" #include "chromeos/services/assistant/fake_assistant_manager_service_impl.h" #include "chromeos/services/assistant/public/cpp/assistant_prefs.h" @@ -77,11 +76,8 @@ class AssistantServiceTest : public testing::Test { ~AssistantServiceTest() override = default; void SetUp() override { - scoped_feature_list_.InitWithFeatures( - /*enabled_features=*/{chromeos::features::kAmbientModeFeature, - chromeos::assistant::features:: - kEnableAmbientAssistant}, - /*disabled_features=*/{}); + scoped_feature_list_.InitAndEnableFeature( + chromeos::assistant::features::kEnableAmbientAssistant); chromeos::CrasAudioHandler::InitializeForTesting();