system-web-apps: update web app launch code to handle aborted SWA launch

This CL updates web app launch process to take aborted SWA launch into consideration, and returns early if that's the case. Fixed: 1323146 Change-Id: Id3fe1bc3e54714611c714d622e3cfe285ba855ae Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3637337 Reviewed-by: Alan Cutter <alancutter@chromium.org> Commit-Queue: Jiewei Qian <qjw@chromium.org> Reviewed-by: Dominic Schulz <dominicschulz@google.com> Cr-Commit-Position: refs/heads/main@{#1002471}
chromium · May 12, 2022 · 0771ec0 · 0771ec0
1 parent 633412c
commit 0771ec0
Show file tree

Hide file tree

Showing 10 changed files with 107 additions and 30 deletions.
diff --git a/chrome/browser/ui/web_applications/system_web_app_delegate_ui_impl.cc b/chrome/browser/ui/web_applications/system_web_app_delegate_ui_impl.cc
@@ -21,6 +21,9 @@
 
 namespace web_app {
 
+// TODO(crbug.com/1231886): Reduce code duplication between SWA launch code and
+// web app launch code, so SWAs can easily maintain feature parity with regular
+// web apps (e.g. launch_handler behaviours).
 Browser* SystemWebAppDelegate::LaunchAndNavigateSystemWebApp(
     Profile* profile,
     WebAppProvider* provider,

diff --git a/chrome/browser/ui/web_applications/system_web_app_ui_utils.h b/chrome/browser/ui/web_applications/system_web_app_ui_utils.h
@@ -91,6 +91,9 @@ void SetLaunchFiles(bool should_include_launch_directory,
 
 // Implementation of LaunchSystemWebApp. Do not use this before discussing your
 // use case with the System Web Apps team.
+//
+// This method returns `nullptr` if the app aborts the launch (e.g. delaying the
+// launch after some async operation).
 Browser* LaunchSystemWebAppImpl(Profile* profile,
                                 SystemAppType type,
                                 const GURL& url,

diff --git a/chrome/browser/ui/web_applications/test/system_web_app_interactive_uitest.cc b/chrome/browser/ui/web_applications/test/system_web_app_interactive_uitest.cc
@@ -1089,14 +1089,6 @@ class SystemWebAppNewWindowMenuItemTest
     return menu;
   }
 
-  size_t GetSystemWebAppBrowserCount(SystemAppType type) {
-    auto* browser_list = BrowserList::GetInstance();
-    return std::count_if(
-        browser_list->begin(), browser_list->end(), [&](Browser* browser) {
-          return web_app::IsBrowserForSystemWebApp(browser, type);
-        });
-  }
-
   void ExpectMenuCommandLaunchesSystemWebApp(
       std::unique_ptr<ui::MenuModel> menu,
       int command_id,

diff --git a/chrome/browser/ui/web_applications/web_app_launch_process.cc b/chrome/browser/ui/web_applications/web_app_launch_process.cc
@@ -98,18 +98,21 @@ content::WebContents* WebAppLaunchProcess::Run() {
 #endif
 
   // System Web Apps have their own launch code path.
-  // TODO(crbug.com/1231886): Don't use a separate code path so that SWAs can
-  // maintain feature parity with regular web apps (e.g. launch_handler
-  // behaviours).
-  content::WebContents* web_contents = MaybeLaunchSystemWebApp(launch_url);
-  if (web_contents)
-    return web_contents;
+  absl::optional<SystemAppType> system_app_type =
+      GetSystemWebAppTypeForAppId(&profile_, params_.app_id);
+  if (system_app_type) {
+    Browser* browser = LaunchSystemWebAppImpl(&profile_, *system_app_type,
+                                              launch_url, params_);
+
+    return browser ? browser->tab_strip_model()->GetActiveWebContents()
+                   : nullptr;
+  }
 
   auto [browser, is_new_browser] = EnsureBrowser();
 
   NavigateResult navigate_result =
       MaybeNavigateBrowser(browser, is_new_browser, launch_url, share_target);
-  web_contents = navigate_result.web_contents;
+  content::WebContents* web_contents = navigate_result.web_contents;
   if (!web_contents)
     return nullptr;
 
@@ -229,18 +232,6 @@ bool WebAppLaunchProcess::NeverNavigateExistingClients() const {
   }
 }
 
-content::WebContents* WebAppLaunchProcess::MaybeLaunchSystemWebApp(
-    const GURL& launch_url) {
-  absl::optional<SystemAppType> system_app_type =
-      GetSystemWebAppTypeForAppId(&profile_, params_.app_id);
-  if (!system_app_type)
-    return nullptr;
-
-  Browser* browser =
-      LaunchSystemWebAppImpl(&profile_, *system_app_type, launch_url, params_);
-  return browser->tab_strip_model()->GetActiveWebContents();
-}
-
 std::tuple<Browser*, bool /*is_new_browser*/>
 WebAppLaunchProcess::EnsureBrowser() {
   Browser* browser = MaybeFindBrowserForLaunch();

diff --git a/chrome/browser/ui/web_applications/web_app_launch_process.h b/chrome/browser/ui/web_applications/web_app_launch_process.h
@@ -46,7 +46,6 @@ class WebAppLaunchProcess {
   std::tuple<GURL, bool /*is_file_handling*/> GetLaunchUrl(
       const apps::ShareTarget* share_target) const;
   WindowOpenDisposition GetNavigationDisposition(bool is_new_browser) const;
-  content::WebContents* MaybeLaunchSystemWebApp(const GURL& launch_url);
   std::tuple<Browser*, bool /*is_new_browser*/> EnsureBrowser();
   LaunchHandler::RouteTo GetLaunchRouteTo() const;
   bool RouteToExistingClient() const;

diff --git a/chrome/browser/web_applications/system_web_apps/test/system_web_app_browsertest_base.cc b/chrome/browser/web_applications/system_web_apps/test/system_web_app_browsertest_base.cc
@@ -101,8 +101,11 @@ content::WebContents* SystemWebAppBrowserTestBase::LaunchApp(
     DCHECK(navigation_observer.last_navigation_succeeded());
   }
 
-  if (out_browser)
-    *out_browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (out_browser) {
+    *out_browser = web_contents
+                       ? chrome::FindBrowserWithWebContents(web_contents)
+                       : nullptr;
+  }
 
   return web_contents;
 }
@@ -148,6 +151,15 @@ GURL SystemWebAppBrowserTestBase::GetStartUrl() {
   return GetStartUrl(LaunchParamsForApp(GetMockAppType()));
 }
 
+size_t SystemWebAppBrowserTestBase::GetSystemWebAppBrowserCount(
+    SystemAppType type) {
+  auto* browser_list = BrowserList::GetInstance();
+  return std::count_if(
+      browser_list->begin(), browser_list->end(), [&](Browser* browser) {
+        return web_app::IsBrowserForSystemWebApp(browser, type);
+      });
+}
+
 SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest(
     bool install_mock)
     : TestProfileTypeMixin<SystemWebAppBrowserTestBase>(install_mock) {

diff --git a/chrome/browser/web_applications/system_web_apps/test/system_web_app_browsertest_base.h b/chrome/browser/web_applications/system_web_apps/test/system_web_app_browsertest_base.h
@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/web_applications/os_integration/os_integration_manager.h"
 #include "chrome/browser/web_applications/system_web_apps/test/test_system_web_app_installation.h"
 #include "chrome/browser/web_applications/test/fake_web_app_provider.h"
@@ -88,6 +89,9 @@ class SystemWebAppBrowserTestBase : public InProcessBrowserTest {
   content::WebContents* LaunchAppWithoutWaiting(SystemAppType type,
                                                 Browser** browser = nullptr);
 
+  // Returns number of system web app browser windows matching |type|.
+  size_t GetSystemWebAppBrowserCount(SystemAppType type);
+
  protected:
   std::unique_ptr<TestSystemWebAppInstallation> maybe_installation_;
 

diff --git a/chrome/browser/web_applications/system_web_apps/test/system_web_app_manager_browsertest.cc b/chrome/browser/web_applications/system_web_apps/test/system_web_app_manager_browsertest.cc
@@ -1791,6 +1791,26 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppAccessibilityTest,
   // Start the actions.
   speech_monitor_.Replay();
 }
+
+class SystemWebAppAbortsLaunchTest : public SystemWebAppManagerBrowserTest {
+ public:
+  SystemWebAppAbortsLaunchTest()
+      : SystemWebAppManagerBrowserTest(/*install_mock*/ false) {
+    maybe_installation_ =
+        TestSystemWebAppInstallation::SetUpAppThatAbortsLaunch();
+  }
+  ~SystemWebAppAbortsLaunchTest() override = default;
+};
+
+IN_PROC_BROWSER_TEST_P(SystemWebAppAbortsLaunchTest, LaunchAborted) {
+  WaitForTestSystemAppInstall();
+
+  LaunchSystemWebAppAsync(browser()->profile(), maybe_installation_->GetType());
+  FlushSystemWebAppLaunchesForTesting(browser()->profile());
+
+  EXPECT_EQ(0U, GetSystemWebAppBrowserCount(maybe_installation_->GetType()));
+}
+
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 #if !BUILDFLAG(IS_CHROMEOS_LACROS)
@@ -1859,6 +1879,8 @@ INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_REGULAR_PROFILE_P(
     SystemWebAppManagerDefaultBoundsTest);
 INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_REGULAR_PROFILE_P(
     SystemWebAppAccessibilityTest);
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_REGULAR_PROFILE_P(
+    SystemWebAppAbortsLaunchTest);
 #endif
 
 #if !BUILDFLAG(IS_CHROMEOS_LACROS)

diff --git a/chrome/browser/web_applications/system_web_apps/test/test_system_web_app_installation.cc b/chrome/browser/web_applications/system_web_apps/test/test_system_web_app_installation.cc
@@ -167,6 +167,19 @@ gfx::Rect UnittestingSystemAppDelegate::GetDefaultBounds(
   }
   return gfx::Rect();
 }
+Browser* UnittestingSystemAppDelegate::LaunchAndNavigateSystemWebApp(
+    Profile* profile,
+    WebAppProvider* provider,
+    const GURL& url,
+    const apps::AppLaunchParams& params) const {
+  if (launch_and_navigate_system_web_apps_) {
+    return launch_and_navigate_system_web_apps_.Run(profile, provider, url,
+                                                    params);
+  }
+  return SystemWebAppDelegate::LaunchAndNavigateSystemWebApp(profile, provider,
+                                                             url, params);
+}
+
 bool UnittestingSystemAppDelegate::IsAppEnabled() const {
   return is_app_enabled;
 }
@@ -245,6 +258,10 @@ void UnittestingSystemAppDelegate::SetDefaultBounds(
     base::RepeatingCallback<gfx::Rect(Browser*)> lambda) {
   get_default_bounds_ = std::move(lambda);
 }
+void UnittestingSystemAppDelegate::SetLaunchAndNavigateSystemWebApp(
+    LaunchAndNavigateSystemWebAppCallback lambda) {
+  launch_and_navigate_system_web_apps_ = std::move(lambda);
+}
 void UnittestingSystemAppDelegate::SetIsAppEnabled(bool value) {
   is_app_enabled = value;
 }
@@ -624,6 +641,22 @@ TestSystemWebAppInstallation::SetUpAppWithShortcuts() {
       new TestSystemWebAppInstallation(std::move(delegate)));
 }
 
+// static
+std::unique_ptr<TestSystemWebAppInstallation>
+TestSystemWebAppInstallation::SetUpAppThatAbortsLaunch() {
+  std::unique_ptr<UnittestingSystemAppDelegate> delegate =
+      std::make_unique<UnittestingSystemAppDelegate>(
+          SystemAppType::OS_FEEDBACK, "Test",
+          GURL("chrome://test-system-app/pwa.html"),
+          base::BindRepeating(&GenerateWebAppInstallInfoForTestApp));
+  delegate->SetLaunchAndNavigateSystemWebApp(base::BindRepeating(
+      [](Profile*, WebAppProvider*, const GURL&,
+         const apps::AppLaunchParams&) -> Browser* { return nullptr; }));
+
+  return base::WrapUnique(
+      new TestSystemWebAppInstallation(std::move(delegate)));
+}
+
 namespace {
 enum SystemWebAppWindowConfig {
   SINGLE_WINDOW,

diff --git a/chrome/browser/web_applications/system_web_apps/test/test_system_web_app_installation.h b/chrome/browser/web_applications/system_web_apps/test/test_system_web_app_installation.h
@@ -29,6 +29,12 @@ class UnittestingSystemAppDelegate : public SystemWebAppDelegate {
       delete;
   ~UnittestingSystemAppDelegate() override;
 
+  using LaunchAndNavigateSystemWebAppCallback =
+      base::RepeatingCallback<Browser*(Profile*,
+                                       WebAppProvider*,
+                                       const GURL&,
+                                       const apps::AppLaunchParams&)>;
+
   std::unique_ptr<WebAppInstallInfo> GetWebAppInfo() const override;
 
   std::vector<AppId> GetAppIdsToUninstallAndReplace() const override;
@@ -49,6 +55,11 @@ class UnittestingSystemAppDelegate : public SystemWebAppDelegate {
   bool ShouldAllowScriptsToCloseWindows() const override;
   absl::optional<SystemAppBackgroundTaskInfo> GetTimerInfo() const override;
   gfx::Rect GetDefaultBounds(Browser* browser) const override;
+  Browser* LaunchAndNavigateSystemWebApp(
+      Profile* profile,
+      WebAppProvider* provider,
+      const GURL& url,
+      const apps::AppLaunchParams& params) const override;
   bool IsAppEnabled() const override;
   bool IsUrlInSystemAppScope(const GURL& url) const override;
   bool PreferManifestBackgroundColor() const override;
@@ -74,6 +85,7 @@ class UnittestingSystemAppDelegate : public SystemWebAppDelegate {
   void SetShouldAllowScriptsToCloseWindows(bool);
   void SetTimerInfo(const SystemAppBackgroundTaskInfo&);
   void SetDefaultBounds(base::RepeatingCallback<gfx::Rect(Browser*)>);
+  void SetLaunchAndNavigateSystemWebApp(LaunchAndNavigateSystemWebAppCallback);
   void SetIsAppEnabled(bool);
   void SetUrlInSystemAppScope(const GURL& url);
   void SetPreferManifestBackgroundColor(bool);
@@ -109,6 +121,9 @@ class UnittestingSystemAppDelegate : public SystemWebAppDelegate {
   base::RepeatingCallback<gfx::Rect(Browser*)> get_default_bounds_ =
       base::NullCallback();
 
+  LaunchAndNavigateSystemWebAppCallback launch_and_navigate_system_web_apps_ =
+      base::NullCallback();
+
   absl::optional<SystemAppBackgroundTaskInfo> timer_info_;
 };
 
@@ -179,6 +194,9 @@ class TestSystemWebAppInstallation {
 
   static std::unique_ptr<TestSystemWebAppInstallation> SetUpAppWithShortcuts();
 
+  static std::unique_ptr<TestSystemWebAppInstallation>
+  SetUpAppThatAbortsLaunch();
+
   // This creates 4 system web app types for testing context menu with
   // different windowing options:
   //