From 65b93573a016736e5cde3e5253329053c2b149c8 Mon Sep 17 00:00:00 2001 From: Hidehiko Abe Date: Wed, 19 Jan 2022 11:01:37 +0000 Subject: [PATCH] Make Lacros lifetime closer to real use. We started to enable Lacros's KeepAlive for AppService. Following that, now AshBrowserTestStarter uses it to make it closer to the real use cases. To avoid name conflict, put AshBrowserTestStarter to crosapi::test namespace. BUG=1277898 TEST=Ran browser_tests. Change-Id: I007e2cf5dc75cf581b3fdca595c18b2a9fbc8c5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3386527 Reviewed-by: Sven Zheng Reviewed-by: David Tseng Commit-Queue: Hidehiko Abe Cr-Commit-Position: refs/heads/main@{#960880} --- chrome/browser/ash/crosapi/browser_manager.h | 8 ++++++++ .../base/chromeos/ash_browser_test_starter.cc | 20 ++++++++++--------- .../base/chromeos/ash_browser_test_starter.h | 4 ++++ .../demo_ash_requires_lacros_browsertest.cc | 2 +- chrome/test/base/javascript_browser_test.cc | 11 +++------- chrome/test/base/javascript_browser_test.h | 6 ++++-- 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/chrome/browser/ash/crosapi/browser_manager.h b/chrome/browser/ash/crosapi/browser_manager.h index 0a138ed8970a29..bda90bf0373445 100644 --- a/chrome/browser/ash/crosapi/browser_manager.h +++ b/chrome/browser/ash/crosapi/browser_manager.h @@ -44,6 +44,10 @@ namespace mojom { class Crosapi; } // namespace mojom +namespace test { +class AshBrowserTestStarter; +} // namespace test + class BrowserLoader; class TestMojoConnectionManager; @@ -273,6 +277,10 @@ class BrowserManager : public session_manager::SessionManagerObserver, // web apps in Lacros. Need to decouple the App Platform systems from // needing lacros-chrome running all the time. friend class apps::AppServiceProxyAsh; + // Currently in practice KeepAlive is enabled for AppService as described + // above. To make the testing environment closer to the case, set up + // KeepAlive for AshBrowserTestStarter, too. + friend class test::AshBrowserTestStarter; // Returns true if the binary is ready to launch or already launched. bool IsReady() const; diff --git a/chrome/test/base/chromeos/ash_browser_test_starter.cc b/chrome/test/base/chromeos/ash_browser_test_starter.cc index ae6bbd1cd2c3f2..73373115d8e69a 100644 --- a/chrome/test/base/chromeos/ash_browser_test_starter.cc +++ b/chrome/test/base/chromeos/ash_browser_test_starter.cc @@ -20,6 +20,7 @@ #include "chrome/test/base/in_process_browser_test.h" #include "testing/gtest/include/gtest/gtest.h" +namespace crosapi { namespace test { AshBrowserTestStarter::AshBrowserTestStarter() = default; @@ -45,7 +46,7 @@ bool AshBrowserTestStarter::PrepareEnvironmentForLacros() { return true; } -class LacrosStartedObserver : public crosapi::BrowserManagerObserver { +class LacrosStartedObserver : public BrowserManagerObserver { public: LacrosStartedObserver() = default; LacrosStartedObserver(const LacrosStartedObserver&) = delete; @@ -53,13 +54,13 @@ class LacrosStartedObserver : public crosapi::BrowserManagerObserver { ~LacrosStartedObserver() override = default; void OnStateChanged() override { - if (crosapi::BrowserManager::Get()->IsRunning()) { + if (BrowserManager::Get()->IsRunning()) { run_loop_.Quit(); } } void Wait(base::TimeDelta timeout) { - if (crosapi::BrowserManager::Get()->IsRunning()) { + if (BrowserManager::Get()->IsRunning()) { return; } base::ThreadPool::PostDelayedTask(FROM_HERE, run_loop_.QuitClosure(), @@ -91,16 +92,16 @@ void AshBrowserTestStarter::StartLacros(InProcessBrowserTest* test_class_obj) { DCHECK(HasLacrosArgument()); WaitForExoStarted(scoped_temp_dir_xdg_.GetPath()); - - crosapi::BrowserManager::Get()->NewWindow( - /*incongnito=*/false, /*should_trigger_session_restore=*/false); + auto* browser_manager = BrowserManager::Get(); + lacros_keep_alive_ = + browser_manager->KeepAlive(BrowserManager::Feature::kTestOnly); LacrosStartedObserver observer; - crosapi::BrowserManager::Get()->AddObserver(&observer); + browser_manager->AddObserver(&observer); observer.Wait(TestTimeouts::action_max_timeout()); - crosapi::BrowserManager::Get()->RemoveObserver(&observer); + browser_manager->RemoveObserver(&observer); - CHECK(crosapi::BrowserManager::Get()->IsRunning()); + CHECK(browser_manager->IsRunning()); // Create a new ash browser window so browser() can work. Profile* profile = ProfileManager::GetActiveUserProfile(); @@ -109,3 +110,4 @@ void AshBrowserTestStarter::StartLacros(InProcessBrowserTest* test_class_obj) { } } // namespace test +} // namespace crosapi diff --git a/chrome/test/base/chromeos/ash_browser_test_starter.h b/chrome/test/base/chromeos/ash_browser_test_starter.h index f4ca460e78e178..ae6f689c6025c8 100644 --- a/chrome/test/base/chromeos/ash_browser_test_starter.h +++ b/chrome/test/base/chromeos/ash_browser_test_starter.h @@ -7,9 +7,11 @@ #include "base/files/scoped_temp_dir.h" #include "base/test/scoped_feature_list.h" +#include "chrome/browser/ash/crosapi/browser_manager.h" class InProcessBrowserTest; +namespace crosapi { namespace test { class AshBrowserTestStarter { @@ -37,8 +39,10 @@ class AshBrowserTestStarter { // This is XDG_RUNTIME_DIR. base::ScopedTempDir scoped_temp_dir_xdg_; base::test::ScopedFeatureList scoped_feature_list_; + std::unique_ptr lacros_keep_alive_; }; } // namespace test +} // namespace crosapi #endif // CHROME_TEST_BASE_CHROMEOS_ASH_BROWSER_TEST_STARTER_H_ diff --git a/chrome/test/base/chromeos/demo_ash_requires_lacros_browsertest.cc b/chrome/test/base/chromeos/demo_ash_requires_lacros_browsertest.cc index 43216908955d0e..5d9157795f461e 100644 --- a/chrome/test/base/chromeos/demo_ash_requires_lacros_browsertest.cc +++ b/chrome/test/base/chromeos/demo_ash_requires_lacros_browsertest.cc @@ -34,7 +34,7 @@ class DemoAshRequiresLacrosTest : public InProcessBrowserTest { } protected: - test::AshBrowserTestStarter ash_starter_; + crosapi::test::AshBrowserTestStarter ash_starter_; }; IN_PROC_BROWSER_TEST_F(DemoAshRequiresLacrosTest, NewTab) { diff --git a/chrome/test/base/javascript_browser_test.cc b/chrome/test/base/javascript_browser_test.cc index f818f11f423cc6..774407df62e6d7 100644 --- a/chrome/test/base/javascript_browser_test.cc +++ b/chrome/test/base/javascript_browser_test.cc @@ -18,18 +18,13 @@ void JavaScriptBrowserTest::AddLibrary(const base::FilePath& library_path) { user_libraries_.push_back(library_path); } -JavaScriptBrowserTest::JavaScriptBrowserTest() { -#if BUILDFLAG(IS_CHROMEOS_ASH) +JavaScriptBrowserTest::JavaScriptBrowserTest() = default; -#endif -} - -JavaScriptBrowserTest::~JavaScriptBrowserTest() { -} +JavaScriptBrowserTest::~JavaScriptBrowserTest() = default; void JavaScriptBrowserTest::SetUpInProcessBrowserTestFixture() { #if BUILDFLAG(IS_CHROMEOS_ASH) - ash_starter_ = std::make_unique(); + ash_starter_ = std::make_unique(); if (ash_starter_->HasLacrosArgument()) ASSERT_TRUE(ash_starter_->PrepareEnvironmentForLacros()); #endif // BUILDFLAG(IS_CHROMEOS_ASH) diff --git a/chrome/test/base/javascript_browser_test.h b/chrome/test/base/javascript_browser_test.h index ef5a044c79e4d9..da2a9199cbfbc0 100644 --- a/chrome/test/base/javascript_browser_test.h +++ b/chrome/test/base/javascript_browser_test.h @@ -49,7 +49,9 @@ class JavaScriptBrowserTest : public InProcessBrowserTest { std::vector args); #if BUILDFLAG(IS_CHROMEOS_ASH) - test::AshBrowserTestStarter* ash_starter() { return ash_starter_.get(); } + crosapi::test::AshBrowserTestStarter* ash_starter() { + return ash_starter_.get(); + } #endif // BUILDFLAG(IS_CHROMEOS_ASH) private: @@ -60,7 +62,7 @@ class JavaScriptBrowserTest : public InProcessBrowserTest { std::vector library_search_paths_; #if BUILDFLAG(IS_CHROMEOS_ASH) - std::unique_ptr ash_starter_; + std::unique_ptr ash_starter_; #endif // BUILDFLAG(IS_CHROMEOS_ASH) };