Make web app reparenting respect manifest launch_handler configuration

Reparenting a web app from a browser tab currently always creates a new
web app window for non-tabbed web apps.

This CL changes reparenting that such that if the web app has its
manifest.launch_handler.client_mode set to "navigate-existing" or
"focus-existing" and there is already a web app window open then a
fresh launch of the web app is executed with the browser tab's URL,
this launch will navigate/focus the existing web app window, and then
the browser tab is closed.

This avoids opening multiple web app windows where launch_handler
has been configured to dedup them.

Existing browser tab page state is now lost. A future "tombstone tab"
implementation is planned to enable users to "undo" the reparenting.

Closing the browser tab after launching the web app to the same URL
is consistent with popping browser tabs out to Arc Android apps and
also avoids leaving behind an active tab in the browser which didn't
happen in the previous flow.

Bug: 1385226
Change-Id: I91f8b16711c89d3e46bf0e91b62ff45bff809b0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4037721
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075653}

chrome/browser/ui/web_applications/app_browser_controller.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/browser_window_state.h"
 #include "chrome/browser/ui/color/chrome_color_id.h"
@@ -84,6 +85,24 @@ bool AppBrowserController::IsForWebApp(const Browser* browser,
   return IsWebApp(browser) && browser->app_controller()->app_id() == app_id;
 }
 
+// static
+Browser* AppBrowserController::FindForWebApp(const Profile& profile,
+                                             const AppId& app_id) {
+  const BrowserList* browser_list = BrowserList::GetInstance();
+  for (auto it = browser_list->begin_browsers_ordered_by_activation();
+       it != browser_list->end_browsers_ordered_by_activation(); ++it) {
+    Browser* browser = *it;
+    if (browser->type() == Browser::TYPE_POPUP)
+      continue;
+    if (browser->profile() != &profile)
+      continue;
+    if (!IsForWebApp(browser, app_id))
+      continue;
+    return browser;
+  }
+  return nullptr;
+}
+
 // static
 std::u16string AppBrowserController::FormatUrlOrigin(
     const GURL& url,

chrome/browser/ui/web_applications/app_browser_controller.h

@@ -68,6 +68,9 @@ class AppBrowserController
   static bool IsWebApp(const Browser* browser);
   // Returns whether |browser| is a web app window/pop-up for |app_id|.
   static bool IsForWebApp(const Browser* browser, const AppId& app_id);
+  // Returns a Browser* that is for |app_id| and |profile| if any, searches in
+  // order of last browser activation. Ignores pop-up Browsers.
+  static Browser* FindForWebApp(const Profile& profile, const AppId& app_id);
 
   // Renders |url|'s origin as Unicode.
   static std::u16string FormatUrlOrigin(

chrome/browser/ui/web_applications/web_app_launch_handler_browsertest.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chrome/browser/ui/web_applications/web_app_launch_utils.h"
 #include "chrome/browser/web_applications/manifest_update_manager.h"
 #include "chrome/browser/web_applications/os_integration/os_integration_manager.h"
 #include "chrome/browser/web_applications/test/web_app_install_test_utils.h"
@@ -24,6 +25,7 @@
 #include "components/page_load_metrics/browser/page_load_metrics_test_waiter.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/url_loader_interceptor.h"
 #include "third_party/blink/public/common/features.h"
 #include "ui/base/page_transition_types.h"
@@ -123,25 +125,45 @@ class WebAppLaunchHandlerBrowserTest : public InProcessBrowserTest {
     EXPECT_EQ(GetLaunchHandler(app_id),
               (LaunchHandler{ClientMode::kNavigateExisting}));
 
-    Browser* browser_1 = LaunchWebAppBrowserAndWait(profile(), app_id);
-    content::WebContents* web_contents =
-        browser_1->tab_strip_model()->GetActiveWebContents();
-    EXPECT_EQ(web_contents->GetLastCommittedURL(), start_url);
-    EXPECT_EQ(AwaitNextLaunchParamsTargetUrl(browser_1), start_url.spec());
+    // Create first web app browser window.
+    Browser* app_browser = LaunchWebAppBrowserAndWait(profile(), app_id);
+    content::WebContents* app_web_contents =
+        app_browser->tab_strip_model()->GetActiveWebContents();
+    EXPECT_EQ(app_web_contents->GetLastCommittedURL(), start_url);
+    EXPECT_EQ(AwaitNextLaunchParamsTargetUrl(app_browser), start_url.spec());
 
     // Navigate window away from start_url to check that the next launch navs to
     // start_url again.
-    GURL alt_url = embedded_test_server()->GetURL("/web_apps/basic.html");
-    NavigateToURLAndWait(browser_1, alt_url);
-    EXPECT_EQ(web_contents->GetLastCommittedURL(), alt_url);
+    {
+      GURL alt_url = embedded_test_server()->GetURL("/web_apps/basic.html");
+      NavigateToURLAndWait(app_browser, alt_url);
+      EXPECT_EQ(app_web_contents->GetLastCommittedURL(), alt_url);
+
+      Browser* app_browser_2 = LaunchWebAppBrowserAndWait(profile(), app_id);
+      EXPECT_EQ(app_browser, app_browser_2);
+      EXPECT_EQ(app_web_contents->GetLastCommittedURL(), start_url);
+      EXPECT_EQ(AwaitNextLaunchParamsTargetUrl(app_browser_2),
+                start_url.spec());
+    }
 
-    Browser* browser_2 = LaunchWebAppBrowserAndWait(profile(), app_id);
-    EXPECT_EQ(browser_1, browser_2);
-    EXPECT_EQ(web_contents->GetLastCommittedURL(), start_url);
-    EXPECT_EQ(AwaitNextLaunchParamsTargetUrl(browser_2), start_url.spec());
+    // Reparent an in scope browser tab and check that it navigates the existing
+    // web app window.
+    {
+      content::TestNavigationObserver observer(
+          app_web_contents, content::MessageLoopRunner::QuitMode::DEFERRED);
+
+      chrome::NewTab(browser());
+      EXPECT_EQ(browser()->tab_strip_model()->count(), 2);
+      NavigateToURLAndWait(browser(), start_url);
+      ReparentWebAppForActiveTab(browser());
+      EXPECT_EQ(browser()->tab_strip_model()->count(), 1);
+
+      observer.WaitForNavigationFinished();
+      EXPECT_EQ(AwaitNextLaunchParamsTargetUrl(app_browser), start_url.spec());
+    }
 
     histogram_tester.ExpectUniqueSample(kLaunchHandlerHistogram,
-                                        ClientMode::kNavigateExisting, 2);
+                                        ClientMode::kNavigateExisting, 3);
   }
 
   void ExpectFocusExistingBehaviour(const AppId& app_id,

chrome/browser/ui/web_applications/web_app_launch_process.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -204,7 +203,7 @@ WindowOpenDisposition WebAppLaunchProcess::GetNavigationDisposition(
 
   // If launch handler is routing to an existing client, we want to use the
   // existing WebContents rather than opening a new tab.
-  if (LaunchInExistingClient()) {
+  if (GetLaunchHandler().TargetsExistingClients()) {
     return WindowOpenDisposition::CURRENT_TAB;
   }
 
@@ -215,37 +214,18 @@ WindowOpenDisposition WebAppLaunchProcess::GetNavigationDisposition(
              : WindowOpenDisposition::NEW_FOREGROUND_TAB;
 }
 
-LaunchHandler::ClientMode WebAppLaunchProcess::GetLaunchClientMode() const {
+LaunchHandler WebAppLaunchProcess::GetLaunchHandler() const {
   DCHECK(web_app_);
-  LaunchHandler launch_handler =
-      web_app_->launch_handler().value_or(LaunchHandler());
+  return web_app_->launch_handler().value_or(LaunchHandler());
+}
+
+LaunchHandler::ClientMode WebAppLaunchProcess::GetLaunchClientMode() const {
+  LaunchHandler launch_handler = GetLaunchHandler();
   if (launch_handler.client_mode == LaunchHandler::ClientMode::kAuto)
     return LaunchHandler::ClientMode::kNavigateNew;
   return launch_handler.client_mode;
 }
 
-bool WebAppLaunchProcess::LaunchInExistingClient() const {
-  switch (GetLaunchClientMode()) {
-    case LaunchHandler::ClientMode::kAuto:
-    case LaunchHandler::ClientMode::kNavigateNew:
-      return false;
-    case LaunchHandler::ClientMode::kNavigateExisting:
-    case LaunchHandler::ClientMode::kFocusExisting:
-      return true;
-  }
-}
-
-bool WebAppLaunchProcess::NeverNavigateExistingClients() const {
-  switch (GetLaunchClientMode()) {
-    case LaunchHandler::ClientMode::kAuto:
-    case LaunchHandler::ClientMode::kNavigateNew:
-    case LaunchHandler::ClientMode::kNavigateExisting:
-      return false;
-    case LaunchHandler::ClientMode::kFocusExisting:
-      return true;
-  }
-}
-
 std::tuple<Browser*, bool /*is_new_browser*/>
 WebAppLaunchProcess::EnsureBrowser() {
   Browser* browser = MaybeFindBrowserForLaunch();
@@ -277,17 +257,7 @@ Browser* WebAppLaunchProcess::MaybeFindBrowserForLaunch() const {
     return nullptr;
   }
 
-  const BrowserList* browser_list = BrowserList::GetInstance();
-  for (auto it = browser_list->begin_browsers_ordered_by_activation();
-       it != browser_list->end_browsers_ordered_by_activation(); ++it) {
-    Browser* browser = *it;
-    if (browser->profile() == &*profile_ &&
-        AppBrowserController::IsForWebApp(browser, params_->app_id)) {
-      return browser;
-    }
-  }
-
-  return nullptr;
+  return AppBrowserController::FindForWebApp(*profile_, params_->app_id);
 }
 
 Browser* WebAppLaunchProcess::CreateBrowserForLaunch() {
@@ -330,7 +300,7 @@ WebAppLaunchProcess::NavigateResult WebAppLaunchProcess::MaybeNavigateBrowser(
 
   content::WebContents* existing_tab = tab_strip->GetActiveWebContents();
   DCHECK(existing_tab);
-  if (NeverNavigateExistingClients()) {
+  if (GetLaunchHandler().NeverNavigateExistingClients()) {
     if (base::ValuesEquivalent(WebAppTabHelper::FromWebContents(existing_tab)
                                    ->EnsureLaunchQueue()
                                    .GetPendingLaunchAppId(),

chrome/browser/ui/web_applications/web_app_launch_process.h

@@ -48,9 +48,8 @@ class WebAppLaunchProcess {
       const apps::ShareTarget* share_target) const;
   WindowOpenDisposition GetNavigationDisposition(bool is_new_browser) const;
   std::tuple<Browser*, bool /*is_new_browser*/> EnsureBrowser();
+  LaunchHandler GetLaunchHandler() const;
   LaunchHandler::ClientMode GetLaunchClientMode() const;
-  bool LaunchInExistingClient() const;
-  bool NeverNavigateExistingClients() const;
 
   Browser* MaybeFindBrowserForLaunch() const;
   Browser* CreateBrowserForLaunch();

chrome/browser/ui/web_applications/web_app_launch_utils.cc

@@ -21,6 +21,7 @@
 #include "build/buildflag.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
+#include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/sessions/app_session_service.h"
@@ -39,6 +40,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
 #include "chrome/browser/ui/web_applications/web_app_browser_controller.h"
+#include "chrome/browser/ui/web_applications/web_app_launch_process.h"
 #include "chrome/browser/ui/web_applications/web_app_tabbed_utils.h"
 #include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
@@ -270,6 +272,10 @@ Browser* ReparentWebContentsIntoAppBrowser(content::WebContents* contents,
   // affected.
   WebAppRegistrar& registrar =
       WebAppProvider::GetForWebApps(profile)->registrar();
+  const WebApp* web_app = registrar.GetAppById(app_id);
+  if (!web_app)
+    return nullptr;
+
   if (registrar.IsInstalled(app_id)) {
     absl::optional<GURL> app_scope = registrar.GetAppScope(app_id);
     if (!app_scope)
@@ -284,13 +290,32 @@ Browser* ReparentWebContentsIntoAppBrowser(content::WebContents* contents,
                       extensions::AppLaunchSource::kSourceReparenting,
                       launch_url, contents);
 
+  if (web_app->launch_handler()
+          .value_or(LaunchHandler{})
+          .TargetsExistingClients()) {
+    if (Browser* browser =
+            AppBrowserController::FindForWebApp(*profile, app_id)) {
+      // TODO(crbug.com/1385226): Use apps::AppServiceProxy::LaunchAppWithUrl()
+      // instead to ensure all the usual wrapping code around web app launches
+      // gets executed.
+      apps::AppLaunchParams params(
+          app_id, apps::LaunchContainer::kLaunchContainerWindow,
+          WindowOpenDisposition::CURRENT_TAB, apps::LaunchSource::kFromOmnibox);
+      params.override_url = launch_url;
+      content::WebContents* new_web_contents =
+          WebAppLaunchProcess(*profile, params).Run();
+      contents->Close();
+      return chrome::FindBrowserWithWebContents(new_web_contents);
+    }
+  }
+
   bool as_pinned_home_tab = IsPinnedHomeTabUrl(registrar, app_id, launch_url);
 
   if (registrar.IsTabbedWindowModeEnabled(app_id)) {
-    for (Browser* browser : *BrowserList::GetInstance()) {
-      if (AppBrowserController::IsForWebApp(browser, app_id))
-        return ReparentWebContentsIntoAppBrowser(contents, browser, app_id,
-                                                 as_pinned_home_tab);
+    if (Browser* browser =
+            AppBrowserController::FindForWebApp(*profile, app_id)) {
+      return ReparentWebContentsIntoAppBrowser(contents, browser, app_id,
+                                               as_pinned_home_tab);
     }
   }
 

chrome/browser/ui/web_applications/web_app_launch_utils.h

@@ -35,13 +35,19 @@ absl::optional<AppId> GetWebAppForActiveTab(Browser* browser);
 void PrunePreScopeNavigationHistory(const GURL& scope,
                                     content::WebContents* contents);
 
-// Reparents the active tab into a new app browser for the web app that has the
-// tab's URL in its scope. Does nothing if there is no web app in scope.
+// Invokes ReparentWebContentsIntoAppBrowser() for the active tab for the
+// web app that has the tab's URL in its scope. Does nothing if there is no web
+// app in scope.
 Browser* ReparentWebAppForActiveTab(Browser* browser);
 
-// Reparents |contents| into an app browser for |app_id|.
-// Uses existing app browser if they are in experimental tabbed mode, otherwise
-// creates a new browser window.
+// Reparents |contents| into a standalone web app window for |app_id|.
+// - If the web app has a launch_handler set to reuse existing windows and there
+// are existing web app windows around this will launch the web app into the
+// existing window and close |contents|.
+// - If the web app is in experimental tabbed mode and has and existing web app
+// window, |contents| will be reparented into the existing window.
+// - Otherwise a new browser window is created for |contents| to be reparented
+// into.
 Browser* ReparentWebContentsIntoAppBrowser(content::WebContents* contents,
                                            const AppId& app_id);
 

third_party/blink/common/manifest/manifest.cc

@@ -82,6 +82,28 @@ bool Manifest::LaunchHandler::operator!=(const LaunchHandler& other) const {
   return !(*this == other);
 }
 
+bool Manifest::LaunchHandler::TargetsExistingClients() const {
+  switch (client_mode) {
+    case ClientMode::kAuto:
+    case ClientMode::kNavigateNew:
+      return false;
+    case ClientMode::kNavigateExisting:
+    case ClientMode::kFocusExisting:
+      return true;
+  }
+}
+
+bool Manifest::LaunchHandler::NeverNavigateExistingClients() const {
+  switch (client_mode) {
+    case ClientMode::kAuto:
+    case ClientMode::kNavigateNew:
+    case ClientMode::kNavigateExisting:
+      return false;
+    case ClientMode::kFocusExisting:
+      return true;
+  }
+}
+
 Manifest::TranslationItem::TranslationItem() = default;
 
 Manifest::TranslationItem::~TranslationItem() = default;

third_party/blink/public/common/manifest/manifest.h

@@ -140,6 +140,9 @@ class BLINK_COMMON_EXPORT Manifest {
     bool operator==(const LaunchHandler& other) const;
     bool operator!=(const LaunchHandler& other) const;
 
+    bool TargetsExistingClients() const;
+    bool NeverNavigateExistingClients() const;
+
     ClientMode client_mode = ClientMode::kAuto;
   };