[dPWA] Refactoring launches to always be async.
Context:
Reading information from the WebAppProvider system is often inherently
racy (given all of our async data sources). Putting operations behind
commands guarantees the the data read is not going to change across
async calls (and that you aren't reading it in the middle of an
operation like 'install', 'uninstall', etc).
This project protects 'launch' to make sure it doesn't happen during
an 'uncommitted read' state.
This change was created by using 'git cl split', and is the first part
in making dPWA launches async. The app service is designed to have
launches be async (and in lacros they will ALWAYS be async), but
currently they are "mostly" synchronous. This means that there are a
variety of tests (and even one instance of production code - the
intent launcher) which assumes this. This patch fixes all of those
cases.
Specifically patches split here:
* Add browser or navigation waiters to wait until launches complete in
tests.
* Modifies the method signatures required for making launches async for
making launches async (but just calls the callback synchronously for
now.
* Fixes a future bug for Lacros where the intent navigation closes the
window before launching the app, causing the browser/profile to start
shutting down. Keep-alives were added to ensure this does not happen.
There is no functional change in this patch.
If you want to see a patch with all test changes & functionality
changes, see:
https://chromium-review.googlesource.com/c/chromium/src/+/3929017
$description
This CL was uploaded by git cl split.
R=tobyhuang@chromium.org
Bug: b/242059352, 1377102
Change-Id: Ifffb6083d65c69de119475c7720779e579581481
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3969023
Reviewed-by: Toby Huang <tobyhuang@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062966}
diff --git a/chrome/browser/ui/ash/projector/projector_client_impl_browsertest.cc b/chrome/browser/ui/ash/projector/projector_client_impl_browsertest.cc
index 5e800363..e5a6a3c 100644
--- a/chrome/browser/ui/ash/projector/projector_client_impl_browsertest.cc
+++ b/chrome/browser/ui/ash/projector/projector_client_impl_browsertest.cc
@@ -189,7 +189,10 @@
auto* profile = browser()->profile();
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ browser_opened.Wait();
// Verify that Projector App is opened.
Browser* app_browser =
@@ -212,7 +215,10 @@
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
// Launch the app for the first time.
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ browser_opened.Wait();
// Verify that Projector App is opened.
Browser* app_browser1 =
@@ -247,7 +253,10 @@
auto* profile = browser()->profile();
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ browser_opened.Wait();
// Verify that Projector App is opened.
Browser* app_browser =
@@ -268,7 +277,10 @@
auto* profile = browser()->profile();
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ browser_opened.Wait();
// Verify that Projector App is opened.
Browser* app_browser =
@@ -379,7 +391,11 @@
auto* profile = browser()->profile();
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ if (!is_child())
+ browser_opened.Wait();
// Verify that Projector App is opened.
Browser* app_browser =
@@ -408,7 +424,10 @@
auto* profile = browser()->profile();
SystemWebAppManager::GetForTest(profile)->InstallSystemAppsForTesting();
+ ui_test_utils::BrowserChangeObserver browser_opened(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
client()->OpenProjectorApp();
+ browser_opened.Wait();
// Verify the user can open the Projector App when the policy is enabled.
Browser* app_browser =
diff --git a/chrome/browser/ui/ash/projector/projector_navigation_throttle_browsertest.cc b/chrome/browser/ui/ash/projector/projector_navigation_throttle_browsertest.cc
index 671df5d6..5cda03f 100644
--- a/chrome/browser/ui/ash/projector/projector_navigation_throttle_browsertest.cc
+++ b/chrome/browser/ui/ash/projector/projector_navigation_throttle_browsertest.cc
@@ -52,15 +52,12 @@
// chrome://projector/app/ | Yes | No
// chrome-untrusted://projector | No | No
-class ProjectorNavigationThrottleTest : public InProcessBrowserTest,
- public BrowserListObserver {
+class ProjectorNavigationThrottleTest : public InProcessBrowserTest {
public:
ProjectorNavigationThrottleTest()
: scoped_feature_list_(features::kProjector) {}
- ~ProjectorNavigationThrottleTest() override {
- BrowserList::RemoveObserver(this);
- }
+ ~ProjectorNavigationThrottleTest() override = default;
void SetUpOnMainThread() override {
InProcessBrowserTest::SetUpOnMainThread();
@@ -74,23 +71,9 @@
task_runner_->AdvanceMockTickClock(forward_by);
apps::CommonAppsNavigationThrottle::SetClockForTesting(
task_runner_->GetMockTickClock());
- BrowserList::AddObserver(this);
- }
-
- void WaitForBrowserRemoved() {
- base::RunLoop run_loop;
- on_browser_removed_callback_ = run_loop.QuitClosure();
- run_loop.Run();
}
protected:
- // BrowserListObserver:
- void OnBrowserRemoved(Browser* browser) override {
- if (on_browser_removed_callback_) {
- std::move(on_browser_removed_callback_).Run();
- }
- }
-
Profile* profile() { return browser()->profile(); }
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
@@ -127,6 +110,12 @@
EXPECT_EQ(BrowserList::GetInstance()->size(), 1u);
Browser* old_browser = browser();
+ // We have to listen for both the browser being removed AND the new browser
+ // being added.
+ ui_test_utils::BrowserChangeObserver removed_observer(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kRemoved);
+ ui_test_utils::BrowserChangeObserver added_observer(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
if (navigate_from_link()) {
// Simulate the user clicking a link.
NavigateParams params(browser(), gurl,
@@ -138,8 +127,8 @@
browser(), gurl, WindowOpenDisposition::CURRENT_TAB,
ui_test_utils::BrowserTestWaitFlags::BROWSER_TEST_WAIT_FOR_BROWSER);
}
-
- WaitForBrowserRemoved();
+ removed_observer.Wait();
+ added_observer.Wait();
// During the navigation, we closed the previous browser to prevent dangling
// about:blank pages and opened a new app browser for the Projector SWA.
@@ -205,6 +194,12 @@
GURL(
"https://www.google.com/url?q=https://"
"screencast.apps.chrome&sa=D&source=hangouts&ust=1642759200000000")));
+
+ // We wait for both the old browser to close and the new app browser to open.
+ ui_test_utils::BrowserChangeObserver removed_observer(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kRemoved);
+ ui_test_utils::BrowserChangeObserver added_observer(
+ nullptr, ui_test_utils::BrowserChangeObserver::ChangeType::kAdded);
// The Google servers would redirect to the URL in the ?q= query parameter.
// Simulate this behavior in this test without actually pinging the Google
// servers to prevent flakiness.
@@ -212,7 +207,8 @@
browser(), GURL(kChromeUIUntrustedProjectorPwaUrl),
WindowOpenDisposition::CURRENT_TAB,
ui_test_utils::BrowserTestWaitFlags::BROWSER_TEST_WAIT_FOR_BROWSER);
- WaitForBrowserRemoved();
+ removed_observer.Wait();
+ added_observer.Wait();
// During the navigation, we closed the previous browser to prevent dangling
// blank redirect pages and opened a new app browser for the Projector SWA.
