Skip to content

Commit

Permalink
[M108]Let AppBannerManager.State track if the event is canceled
Browse files Browse the repository at this point in the history
This splits PENDING_PROMPT to PENDING_PROMPT_NOT_CANCELED and
PENDING_PROMPT_CANCELED, tracks whether the beforeinstallprompt event
is canceled.

(cherry picked from commit 457c1cc)

Bug: 1371091
Change-Id: I13c66cbe808f86d289cd1eb2daa875b743923e95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3944931
Reviewed-by: Glenn Hartmann <hartmanng@chromium.org>
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1058980}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3958994
Cr-Commit-Position: refs/branch-heads/5359@{#46}
Cr-Branched-From: 27d3765-refs/heads/main@{#1058933}
  • Loading branch information
EiraGe authored and Chromium LUCI CQ committed Oct 17, 2022
1 parent 4c157d9 commit dfce288
Show file tree
Hide file tree
Showing 8 changed files with 119 additions and 53 deletions.
45 changes: 27 additions & 18 deletions chrome/browser/banners/app_banner_manager_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ class AppBannerManagerTest : public AppBannerManager {
void UpdateState(AppBannerManager::State state) override {
AppBannerManager::UpdateState(state);
if (state == AppBannerManager::State::PENDING_ENGAGEMENT ||
state == AppBannerManager::State::PENDING_PROMPT) {
state == AppBannerManager::State::PENDING_PROMPT_CANCELED ||
state == AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED) {
if (on_done_)
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
std::move(on_done_));
Expand Down Expand Up @@ -263,7 +264,8 @@ class AppBannerManagerBrowserTest : public AppBannerManagerBrowserTestBase {
// Generally the manager will be in the complete state, however some test
// cases navigate the page, causing the state to go back to INACTIVE.
EXPECT_TRUE(manager->state() == State::COMPLETE ||
manager->state() == State::PENDING_PROMPT ||
manager->state() == State::PENDING_PROMPT_CANCELED ||
manager->state() == State::PENDING_PROMPT_NOT_CANCELED ||
manager->state() == State::PENDING_WORKER ||
manager->state() == State::INACTIVE);

Expand Down Expand Up @@ -360,7 +362,8 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
browser()->tab_strip_model()->GetActiveWebContents(),
"addManifestLinkTag()"));
}),
false, AppBannerManager::State::PENDING_PROMPT);
false,
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);
histograms.ExpectTotalCount(kInstallableStatusCodeHistogram, 0);
}

Expand All @@ -372,7 +375,8 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
browser(), manager.get(),
embedded_test_server()->GetURL("/banners/manifest_test_page.html"),
absl::nullopt);
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);

// Dynamically remove the manifest.
base::HistogramTester histograms;
Expand All @@ -398,7 +402,8 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
browser(), manager.get(),
embedded_test_server()->GetURL("/banners/manifest_test_page.html"),
absl::nullopt);
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);

// Dynamically change the manifest, which results in a
// Stop(RENDERER_CANCELLED), and a restart of the pipeline.
Expand All @@ -419,14 +424,16 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
}
// The pipeline should either have completed, or it is scheduled in the
// background. Wait for the next prompt request if so.
if (manager->state() != AppBannerManager::State::PENDING_PROMPT) {
if (manager->state() !=
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED) {
base::HistogramTester histograms;
base::RunLoop run_loop;
manager->PrepareDone(run_loop.QuitClosure());
run_loop.Run();
histograms.ExpectTotalCount(kInstallableStatusCodeHistogram, 0);
}
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);
}

IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, NoManifest) {
Expand Down Expand Up @@ -500,7 +507,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerNotCreated) {
// Navigate and expect the manager to end up waiting for prompt to be called.
TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
false /* expected_will_show */,
State::PENDING_PROMPT);
State::PENDING_PROMPT_NOT_CANCELED);

// Navigate and expect Stop() to be called.
TriggerBannerFlowWithNavigation(browser(), manager.get(), GURL("about:blank"),
Expand Down Expand Up @@ -528,7 +535,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerCancelled) {
// called.
TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
false /* expected_will_show */,
State::PENDING_PROMPT);
State::PENDING_PROMPT_CANCELED);

// Navigate to about:blank and expect Stop() to be called.
TriggerBannerFlowWithNavigation(browser(), manager.get(), GURL("about:blank"),
Expand All @@ -554,7 +561,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
// Navigate to page and get the pipeline started.
TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
false /* expected_will_show */,
State::PENDING_PROMPT);
State::PENDING_PROMPT_NOT_CANCELED);

// Now let the page call prompt with a gesture. The banner should be shown.
TriggerBannerFlow(
Expand Down Expand Up @@ -594,7 +601,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
base::Unretained(service),
browser()->tab_strip_model()->GetActiveWebContents(),
ui::PageTransition::PAGE_TRANSITION_TYPED),
false /* expected_will_show */, State::PENDING_PROMPT);
false /* expected_will_show */, State::PENDING_PROMPT_NOT_CANCELED);

// Trigger prompt() and expect the banner to be shown.
TriggerBannerFlow(
Expand All @@ -621,7 +628,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerReprompt) {
// Navigate to page and get the pipeline started.
TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
false /* expected_will_show */,
State::PENDING_PROMPT);
State::PENDING_PROMPT_NOT_CANCELED);

// Call prompt to show the banner.
TriggerBannerFlow(
Expand Down Expand Up @@ -708,7 +715,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerTerminated) {
// called.
TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
false /* expected_will_show */,
State::PENDING_PROMPT);
State::PENDING_PROMPT_NOT_CANCELED);

// Navigate to about:blank and expect it to be terminated because the previous
// URL is still pending.
Expand Down Expand Up @@ -823,9 +830,10 @@ IN_PROC_BROWSER_TEST_P(AppBannerManagerBrowserTestWithChromeBFCache,
// Triggering flow to first URL with a pending prompt.
TriggerBannerFlowWithNavigation(browser(), manager.get(), GetBannerURL(),
/*expected_will_show=*/false,
State::PENDING_PROMPT);
State::PENDING_PROMPT_NOT_CANCELED);
content::RenderFrameHostWrapper rfh_a(current_frame_host());
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);
histograms.ExpectTotalCount(kInstallableStatusCodeHistogram, 0);

// Navigating to 2nd installable URL while PENDING_PROMPT will trigger
Expand Down Expand Up @@ -939,7 +947,8 @@ IN_PROC_BROWSER_TEST_F(
// still successfully get to the PENDING_PROMPT state of the pipeline, as it
// should retry the call to GetData on the InstallableManager.
RunBannerTest(browser(), manager.get(), test_url, MANIFEST_URL_CHANGED);
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);

{
base::HistogramTester histograms;
Expand Down Expand Up @@ -1121,7 +1130,8 @@ IN_PROC_BROWSER_TEST_P(AppBannerServiceWorkerCriteriaTest, ShowBanner) {
browser(), manager.get(),
embedded_test_server()->GetURL("/banners/manifest_test_page.html"),
absl::nullopt);
EXPECT_EQ(manager->state(), AppBannerManager::State::PENDING_PROMPT);
EXPECT_EQ(manager->state(),
AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED);
EXPECT_EQ(manager->GetInstallableWebAppCheckResultForTesting(),
AppBannerManager::InstallableWebAppCheckResult::kYes_Promotable);
}
Expand Down Expand Up @@ -1213,7 +1223,6 @@ IN_PROC_BROWSER_TEST_P(AppBannerServiceWorkerCriteriaTest,
manager->GetInstallableWebAppCheckResultForTesting(),
AppBannerManager::InstallableWebAppCheckResult::kYes_ByUserRequest);
}
EXPECT_EQ(manager->GetAppName(), u"Manifest test app");
}

INSTANTIATE_TEST_SUITE_P(
Expand Down
20 changes: 10 additions & 10 deletions chrome/browser/banners/app_banner_manager_desktop_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), GetBannerURLWithAction("stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

{
Expand Down Expand Up @@ -134,7 +134,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), GetBannerURLWithAction("verify_appinstalled_stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

{
Expand Down Expand Up @@ -187,7 +187,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), GetBannerURLWithAction("stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

{
Expand Down Expand Up @@ -232,7 +232,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
browser(), GetBannerURLWithManifestAndQuery("/banners/minimal-ui.json",
"action", "stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

// Install the app via the menu instead of the banner.
Expand Down Expand Up @@ -263,7 +263,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
browser(), GetBannerURLWithManifestAndQuery("/banners/fullscreen.json",
"action", "stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

// Install the app via the menu instead of the banner.
Expand Down Expand Up @@ -299,7 +299,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GetBannerURL()));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

EXPECT_EQ(AppBannerManager::InstallableWebAppCheckResult::kYes_Promotable,
Expand Down Expand Up @@ -343,7 +343,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GetBannerURL()));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

EXPECT_EQ(AppBannerManager::InstallableWebAppCheckResult::kYes_Promotable,
Expand All @@ -368,7 +368,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
"/banners/manifest_display_override.json", "action",
"stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

// Install the app via the menu instead of the banner.
Expand Down Expand Up @@ -400,7 +400,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,
"/banners/manifest_display_override_display_is_browser.json",
"action", "stash_event")));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

{
Expand Down Expand Up @@ -442,7 +442,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerDesktopBrowserTest,

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GetBannerURL()));
run_loop.Run();
EXPECT_EQ(State::PENDING_PROMPT, manager->state());
EXPECT_EQ(State::PENDING_PROMPT_NOT_CANCELED, manager->state());
}

EXPECT_EQ(AppBannerManager::InstallableWebAppCheckResult::kYes_Promotable,
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/banners/test_app_banner_manager_desktop.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ void TestAppBannerManagerDesktop::UpdateState(AppBannerManager::State state) {
AppBannerManager::UpdateState(state);

if (state == AppBannerManager::State::PENDING_ENGAGEMENT ||
state == AppBannerManager::State::PENDING_PROMPT ||
state == AppBannerManager::State::PENDING_PROMPT_CANCELED ||
state == AppBannerManager::State::PENDING_PROMPT_NOT_CANCELED ||
state == AppBannerManager::State::COMPLETE) {
OnFinished();
}
Expand Down
3 changes: 3 additions & 0 deletions chrome/test/data/banners/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ function addPromptListener(action) {
e.preventDefault();
setTimeout(() => e.prompt(), 0);
break;
case Action.CANCEL_PROMPT:
e.preventDefault();
break;
case Action.CANCEL_PROMPT_AND_NAVIGATE:
e.preventDefault();
// Navigate the window to trigger cancellation in the renderer.
Expand Down
Loading

0 comments on commit dfce288

Please sign in to comment.