desktop-pwas: Make "capture_links": "new-client" capture navigations

This CL makes the "capture_links": "new-client" web app manifest
setting behave like the existing experimental tabbed web app link
capturing.

This behavior is hidden behind the WebAppEnableLinkCapturing flag.

Screencast: https://bugs.chromium.org/p/chromium/issues/attachment?aid=484716&signed_aid=tplWTvCmrc2w7LVRH_JXdA==&inline=1

Bug: 1163398
Change-Id: I34c525bb35b09f25b2dfca3237d8baec9d5a1010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626618
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843908}

chrome/browser/apps/intent_helper/apps_navigation_throttle.cc

@@ -21,12 +21,15 @@
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
 #include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
+#include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_registrar.h"
 #include "chrome/common/chrome_features.h"
 #include "components/services/app_service/public/mojom/types.mojom.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
+#include "third_party/blink/public/common/features.h"
 #include "ui/gfx/image/image.h"
 #include "url/origin.h"
 
@@ -108,10 +111,10 @@ ThrottleCheckResult AppsNavigationThrottle::HandleRequest() {
 
   MaybeRemoveComingFromArcFlag(web_contents, starting_url_, url);
 
-  base::Optional<ThrottleCheckResult> tab_strip_capture =
-      CaptureExperimentalTabStripWebAppScopeNavigations(web_contents, handle);
-  if (tab_strip_capture.has_value())
-    return tab_strip_capture.value();
+  base::Optional<ThrottleCheckResult> web_app_capture =
+      CaptureWebAppScopeNavigations(web_contents, handle);
+  if (web_app_capture.has_value())
+    return web_app_capture.value();
 
   // Do not pop up the intent picker bubble or automatically launch the app if
   // we shouldn't override url loading, or if we don't have a browser, or we are
@@ -141,17 +144,21 @@ ThrottleCheckResult AppsNavigationThrottle::HandleRequest() {
 }
 
 base::Optional<ThrottleCheckResult>
-AppsNavigationThrottle::CaptureExperimentalTabStripWebAppScopeNavigations(
+AppsNavigationThrottle::CaptureWebAppScopeNavigations(
     content::WebContents* web_contents,
     content::NavigationHandle* handle) const {
   if (!navigate_from_link())
     return base::nullopt;
 
-  if (!base::FeatureList::IsEnabled(features::kDesktopPWAsTabStrip) ||
-      !base::FeatureList::IsEnabled(
-          features::kDesktopPWAsTabStripLinkCapturing)) {
+  bool tabbed_web_apps =
+      base::FeatureList::IsEnabled(features::kDesktopPWAsTabStrip);
+  bool tabbed_link_capturing =
+      base::FeatureList::IsEnabled(features::kDesktopPWAsTabStripLinkCapturing);
+  bool link_capturing =
+      base::FeatureList::IsEnabled(blink::features::kWebAppEnableLinkCapturing);
+
+  if (!link_capturing && (!tabbed_web_apps || !tabbed_link_capturing))
     return base::nullopt;
-  }
 
   Profile* const profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
@@ -166,33 +173,66 @@ AppsNavigationThrottle::CaptureExperimentalTabStripWebAppScopeNavigations(
   if (!app_id)
     return base::nullopt;
 
-  if (!provider->registrar().IsInExperimentalTabbedWindowMode(*app_id))
+  bool app_in_tabbed_mode =
+      provider->registrar().IsInExperimentalTabbedWindowMode(*app_id);
+  if (!link_capturing && !app_in_tabbed_mode)
     return base::nullopt;
 
+  auto* tab_helper =
+      web_app::WebAppTabHelperBase::FromWebContents(web_contents);
+  if (tab_helper && tab_helper->GetAppId() == *app_id) {
+    // Already in app scope, do not alter window state while using the app.
+    return base::nullopt;
+  }
+
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
   if (web_app::AppBrowserController::IsForWebApp(browser, *app_id)) {
     // Already in the app window; navigation already captured.
     return base::nullopt;
   }
 
-  // If |web_contents| hasn't loaded yet or has only loaded about:blank we
-  // should reparent it into the app window to avoid leaving behind a blank tab.
-  auto* tab_helper =
-      web_app::WebAppTabHelperBase::FromWebContents(web_contents);
-  if (tab_helper && !tab_helper->HasLoadedNonAboutBlankPage()) {
-    web_app::ReparentWebContentsIntoAppBrowser(web_contents, *app_id);
-    return content::NavigationThrottle::PROCEED;
+  blink::mojom::CaptureLinks capture_links = provider->registrar()
+                                                 .AsWebAppRegistrar()
+                                                 ->GetAppById(*app_id)
+                                                 ->capture_links();
+
+  // Experimental tabbed web app link capturing behaves like new-client.
+  // This will be removed once we phase out kDesktopPWAsTabStripLinkCapturing in
+  // favor of kWebAppEnableLinkCapturing.
+  if (capture_links == blink::mojom::CaptureLinks::kUndefined &&
+      app_in_tabbed_mode && tabbed_link_capturing) {
+    capture_links = blink::mojom::CaptureLinks::kNewClient;
   }
 
-  apps::AppLaunchParams launch_params(
-      *app_id, apps::mojom::LaunchContainer::kLaunchContainerWindow,
-      WindowOpenDisposition::CURRENT_TAB,
-      apps::mojom::AppLaunchSource::kSourceUrlHandler);
-  launch_params.override_url = handle->GetURL();
-  apps::AppServiceProxyFactory::GetForProfile(profile)
-      ->BrowserAppLauncher()
-      ->LaunchAppWithParams(std::move(launch_params));
-  return content::NavigationThrottle::CANCEL_AND_IGNORE;
+  switch (capture_links) {
+    case blink::mojom::CaptureLinks::kUndefined:
+    case blink::mojom::CaptureLinks::kNone:
+      return base::nullopt;
+
+    case blink::mojom::CaptureLinks::kNewClient: {
+      // If |web_contents| hasn't loaded yet or has only loaded about:blank we
+      // should reparent it into the app window to avoid leaving behind a blank
+      // tab.
+      if (tab_helper && !tab_helper->HasLoadedNonAboutBlankPage()) {
+        web_app::ReparentWebContentsIntoAppBrowser(web_contents, *app_id);
+        return content::NavigationThrottle::PROCEED;
+      }
+
+      apps::AppLaunchParams launch_params(
+          *app_id, apps::mojom::LaunchContainer::kLaunchContainerWindow,
+          WindowOpenDisposition::CURRENT_TAB,
+          apps::mojom::AppLaunchSource::kSourceUrlHandler);
+      launch_params.override_url = handle->GetURL();
+      apps::AppServiceProxyFactory::GetForProfile(profile)
+          ->BrowserAppLauncher()
+          ->LaunchAppWithParams(std::move(launch_params));
+      return content::NavigationThrottle::CANCEL_AND_IGNORE;
+    }
+
+    case blink::mojom::CaptureLinks::kExistingClientNavigate:
+      // TODO(crbug.com/1163398): Implement.
+      return base::nullopt;
+  }
 }
 
 }  // namespace apps

chrome/browser/apps/intent_helper/apps_navigation_throttle.h

@@ -69,8 +69,7 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
  private:
   // Returns whether navigation to |url| was captured by a web app and what to
   // do next if so.
-  base::Optional<ThrottleCheckResult>
-  CaptureExperimentalTabStripWebAppScopeNavigations(
+  base::Optional<ThrottleCheckResult> CaptureWebAppScopeNavigations(
       content::WebContents* web_contents,
       content::NavigationHandle* handle) const;
 

chrome/browser/ui/web_applications/test/web_app_browsertest_util.cc

@@ -88,6 +88,17 @@ class BrowserRemovedWaiter final : public BrowserListObserver {
   base::RunLoop run_loop_;
 };
 
+void AutoAcceptDialogCallback(
+    content::WebContents* initiator_web_contents,
+    std::unique_ptr<WebApplicationInfo> web_app_info,
+    ForInstallableSite for_installable_site,
+    InstallManager::WebAppInstallationAcceptanceCallback acceptance_callback) {
+  web_app_info->open_as_window = true;
+  std::move(acceptance_callback)
+      .Run(
+          /*user_accepted=*/true, std::move(web_app_info));
+}
+
 }  // namespace
 
 AppId InstallWebApp(Profile* profile,
@@ -114,6 +125,32 @@ AppId InstallWebApp(Profile* profile,
   return app_id;
 }
 
+AppId InstallWebAppFromPage(Browser* browser, const GURL& app_url) {
+  NavigateToURLAndWait(browser, app_url);
+
+  AppId app_id;
+  base::RunLoop run_loop;
+
+  auto* provider = WebAppProvider::Get(browser->profile());
+  DCHECK(provider);
+  WaitUntilReady(provider);
+  provider->install_manager().InstallWebAppFromManifestWithFallback(
+      browser->tab_strip_model()->GetActiveWebContents(),
+      /*force_shortcut_app=*/true,
+      webapps::WebappInstallSource::MENU_BROWSER_TAB,
+      base::BindOnce(&AutoAcceptDialogCallback),
+      base::BindLambdaForTesting(
+          [&run_loop, &app_id](const AppId& installed_app_id,
+                               InstallResultCode code) {
+            DCHECK_EQ(code, InstallResultCode::kSuccessNewInstall);
+            app_id = installed_app_id;
+            run_loop.Quit();
+          }));
+
+  run_loop.Run();
+  return app_id;
+}
+
 AppId InstallWebAppFromManifest(Browser* browser, const GURL& app_url) {
   ServiceWorkerRegistrationWaiter registration_waiter(browser->profile(),
                                                       app_url);
@@ -130,16 +167,7 @@ AppId InstallWebAppFromManifest(Browser* browser, const GURL& app_url) {
       browser->tab_strip_model()->GetActiveWebContents(),
       /*force_shortcut_app=*/false,
       webapps::WebappInstallSource::MENU_BROWSER_TAB,
-      base::BindLambdaForTesting(
-          [](content::WebContents* initiator_web_contents,
-             std::unique_ptr<WebApplicationInfo> web_app_info,
-             ForInstallableSite for_installable_site,
-             InstallManager::WebAppInstallationAcceptanceCallback
-                 acceptance_callback) {
-            std::move(acceptance_callback)
-                .Run(
-                    /*user_accepted=*/true, std::move(web_app_info));
-          }),
+      base::BindOnce(&AutoAcceptDialogCallback),
       base::BindLambdaForTesting(
           [&run_loop, &app_id](const AppId& installed_app_id,
                                InstallResultCode code) {

chrome/browser/ui/web_applications/test/web_app_browsertest_util.h

@@ -23,6 +23,10 @@ enum class InstallResultCode;
 // Synchronous version of InstallManager::InstallWebAppFromInfo.
 AppId InstallWebApp(Profile* profile, std::unique_ptr<WebApplicationInfo>);
 
+// Navigates to |app_url| and installs app without any installability checks.
+// Always selects to open app in its own window.
+AppId InstallWebAppFromPage(Browser* browser, const GURL& app_url);
+
 // Navigates to |app_url|, verifies WebApp installability, and installs app.
 AppId InstallWebAppFromManifest(Browser* browser, const GURL& app_url);
 

chrome/browser/ui/web_applications/web_app_link_capturing_browsertest.cc

@@ -18,6 +18,7 @@
 #include "chrome/common/chrome_features.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "third_party/blink/public/common/features.h"
 
 namespace web_app {
 
@@ -26,23 +27,26 @@ class WebAppLinkCapturingBrowserTest : public WebAppNavigationBrowserTest {
   WebAppLinkCapturingBrowserTest() = default;
   ~WebAppLinkCapturingBrowserTest() override = default;
 
-  void SetUp() override {
-    // TODO(crbug.com/1092789): Migrate the implementation to make web app link
-    // capturing feature to work with AppServiceIntentHandling.
-    features_.InitWithFeatures({features::kDesktopPWAsTabStrip,
-                                features::kDesktopPWAsTabStripLinkCapturing},
-                               {});
-    WebAppNavigationBrowserTest::SetUp();
-  }
-
   void SetUpOnMainThread() override {
     WebAppNavigationBrowserTest::SetUpOnMainThread();
-    auto web_app_info = std::make_unique<WebApplicationInfo>();
-    web_app_info->start_url = start_url_;
-    web_app_info->open_as_window = true;
-    app_id_ = web_app::InstallWebApp(profile(), std::move(web_app_info));
-    provider().registry_controller().SetExperimentalTabbedWindowMode(
-        app_id_, true, /*is_user_action=*/false);
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
+  void InstallTestApp(const char* path) {
+    start_url_ = embedded_test_server()->GetURL(path);
+    in_scope_1_ = start_url_.Resolve("page1.html");
+    in_scope_2_ = start_url_.Resolve("page2.html");
+    scope_ = start_url_.GetWithoutFilename();
+
+    // Create new tab to navigate, install, automatically pop out and then
+    // close. This sequence avoids altering the browser window state we started
+    // with.
+    AddTab(browser(), about_blank_);
+    app_id_ = web_app::InstallWebAppFromPage(browser(), start_url_);
+    Browser* app_browser = BrowserList::GetInstance()->GetLastActive();
+    EXPECT_NE(app_browser, browser());
+    EXPECT_TRUE(AppBrowserController::IsForWebApp(app_browser, app_id_));
+    chrome::CloseWindow(app_browser);
   }
 
   WebAppProviderBase& provider() {
@@ -51,8 +55,15 @@ class WebAppLinkCapturingBrowserTest : public WebAppNavigationBrowserTest {
     return *provider;
   }
 
-  void NavigateMainBrowser(const GURL& url) {
-    ClickLinkAndWait(browser()->tab_strip_model()->GetActiveWebContents(), url,
+  void AddTab(Browser* browser, GURL url) {
+    auto observer = std::make_unique<content::TestNavigationObserver>(url);
+    observer->StartWatchingNewWebContents();
+    chrome::AddTabAt(browser, url, /*index=*/-1, /*foreground=*/true);
+    observer->Wait();
+  }
+
+  void Navigate(Browser* browser, const GURL& url) {
+    ClickLinkAndWait(browser->tab_strip_model()->GetActiveWebContents(), url,
                      LinkTarget::SELF, "");
   }
 
@@ -74,55 +85,76 @@ class WebAppLinkCapturingBrowserTest : public WebAppNavigationBrowserTest {
 
  protected:
   AppId app_id_;
-  GURL start_url_{"https://example.org/dir/start.html"};
-  GURL about_blank_{"about:blank"};
-  GURL in_scope_1_{"https://example.org/dir/page1.html"};
-  GURL in_scope_2_{"https://example.org/dir/page2.html"};
-  GURL origin_{"https://example.org/"};
-  GURL out_of_scope_{"https://other-domain.org/"};
+  GURL start_url_;
+  GURL in_scope_1_;
+  GURL in_scope_2_;
+  GURL scope_;
+
+  const GURL about_blank_{"about:blank"};
+  const GURL out_of_scope_{"https://other-domain.org/"};
+};
+
+class WebAppTabStripLinkCapturingBrowserTest
+    : public WebAppLinkCapturingBrowserTest {
+ public:
+  WebAppTabStripLinkCapturingBrowserTest() {
+    features_.InitWithFeatures({features::kDesktopPWAsTabStrip,
+                                features::kDesktopPWAsTabStripLinkCapturing},
+                               {});
+  }
+
+  void InstallTestApp() {
+    WebAppLinkCapturingBrowserTest::InstallTestApp("/web_apps/basic.html");
+    provider().registry_controller().SetExperimentalTabbedWindowMode(
+        app_id_, true, /*is_user_action=*/false);
+  }
 
  private:
   base::test::ScopedFeatureList features_;
 };
 
-IN_PROC_BROWSER_TEST_F(WebAppLinkCapturingBrowserTest,
+IN_PROC_BROWSER_TEST_F(WebAppTabStripLinkCapturingBrowserTest,
                        InScopeNavigationsCaptured) {
+  InstallTestApp();
+
   // Start browser at an out of scope page.
-  NavigateMainBrowser(out_of_scope_);
+  Navigate(browser(), out_of_scope_);
 
   // In scope navigation should open app window.
-  NavigateMainBrowser(in_scope_1_);
+  Navigate(browser(), in_scope_1_);
   Browser* app_browser = BrowserList::GetInstance()->GetLastActive();
   EXPECT_TRUE(AppBrowserController::IsForWebApp(app_browser, app_id_));
   ExpectTabs(browser(), {out_of_scope_});
   ExpectTabs(app_browser, {in_scope_1_});
 
   // Another in scope navigation should open a new tab in the same app window.
-  NavigateMainBrowser(in_scope_2_);
+  Navigate(browser(), in_scope_2_);
   ExpectTabs(browser(), {out_of_scope_});
   ExpectTabs(app_browser, {in_scope_1_, in_scope_2_});
 
   // Whole origin should count as in scope.
-  NavigateMainBrowser(origin_);
+  Navigate(browser(), scope_);
   ExpectTabs(browser(), {out_of_scope_});
-  ExpectTabs(app_browser, {in_scope_1_, in_scope_2_, origin_});
+  ExpectTabs(app_browser, {in_scope_1_, in_scope_2_, scope_});
 
   // Out of scope should behave as usual.
-  NavigateMainBrowser(out_of_scope_);
+  Navigate(browser(), out_of_scope_);
   ExpectTabs(browser(), {out_of_scope_});
-  ExpectTabs(app_browser, {in_scope_1_, in_scope_2_, origin_});
+  ExpectTabs(app_browser, {in_scope_1_, in_scope_2_, scope_});
 }
 
 // First about:blank captures in scope navigations.
-IN_PROC_BROWSER_TEST_F(WebAppLinkCapturingBrowserTest,
+IN_PROC_BROWSER_TEST_F(WebAppTabStripLinkCapturingBrowserTest,
                        AboutBlankNavigationReparented) {
+  InstallTestApp();
+
   ExpectTabs(browser(), {about_blank_});
   content::WebContents* reparent_web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
 
   // Navigations from a fresh about:blank page should reparent.
   // When no app window is open one should be created.
-  NavigateMainBrowser(in_scope_1_);
+  Navigate(browser(), in_scope_1_);
   Browser* app_browser = BrowserList::GetInstance()->GetLastActive();
   EXPECT_TRUE(AppBrowserController::IsForWebApp(app_browser, app_id_));
   ExpectTabs(browser(), {NtpUrl()});
@@ -133,7 +165,7 @@ IN_PROC_BROWSER_TEST_F(WebAppLinkCapturingBrowserTest,
   // Navigations from a fresh about:blank page via JavaScript should also
   // reparent. When there is already an app window open we should reparent into
   // it.
-  chrome::AddTabAt(browser(), about_blank_, /*index=*/-1, /*foreground=*/true);
+  AddTab(browser(), about_blank_);
   ExpectTabs(browser(), {NtpUrl(), about_blank_});
   reparent_web_contents = browser()->tab_strip_model()->GetActiveWebContents();
   {
@@ -151,4 +183,79 @@ IN_PROC_BROWSER_TEST_F(WebAppLinkCapturingBrowserTest,
             app_browser->tab_strip_model()->GetActiveWebContents());
 }
 
+class WebAppDeclarativeLinkCapturingBrowserTest
+    : public WebAppLinkCapturingBrowserTest {
+ public:
+  WebAppDeclarativeLinkCapturingBrowserTest() {
+    features_.InitAndEnableFeature(blink::features::kWebAppEnableLinkCapturing);
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(WebAppDeclarativeLinkCapturingBrowserTest,
+                       CaptureLinksUnset) {
+  InstallTestApp("/web_apps/basic.html");
+
+  // No link capturing should happen.
+  Navigate(browser(), start_url_);
+  EXPECT_EQ(browser(), BrowserList::GetInstance()->GetLastActive());
+  ExpectTabs(browser(), {start_url_});
+}
+
+IN_PROC_BROWSER_TEST_F(WebAppDeclarativeLinkCapturingBrowserTest,
+                       CaptureLinksNone) {
+  InstallTestApp("/web_apps/capture_links_none.html");
+
+  // No link capturing should happen.
+  Navigate(browser(), start_url_);
+  EXPECT_EQ(browser(), BrowserList::GetInstance()->GetLastActive());
+  ExpectTabs(browser(), {start_url_});
+}
+
+IN_PROC_BROWSER_TEST_F(WebAppDeclarativeLinkCapturingBrowserTest,
+                       CaptureLinksNewClient) {
+  InstallTestApp("/web_apps/capture_links_new_client.html");
+
+  Navigate(browser(), out_of_scope_);
+
+  // In scope navigation should open an app window.
+  Navigate(browser(), in_scope_1_);
+  Browser* app_browser_1 = BrowserList::GetInstance()->GetLastActive();
+  EXPECT_TRUE(AppBrowserController::IsForWebApp(app_browser_1, app_id_));
+  ExpectTabs(browser(), {out_of_scope_});
+  ExpectTabs(app_browser_1, {in_scope_1_});
+
+  // In scope navigation should open a new app window.
+  Navigate(browser(), in_scope_2_);
+  Browser* app_browser_2 = BrowserList::GetInstance()->GetLastActive();
+  EXPECT_TRUE(AppBrowserController::IsForWebApp(app_browser_2, app_id_));
+  EXPECT_NE(app_browser_1, app_browser_2);
+  ExpectTabs(browser(), {out_of_scope_});
+  ExpectTabs(app_browser_1, {in_scope_1_});
+  ExpectTabs(app_browser_2, {in_scope_2_});
+
+  // In scope navigation from app window should not capture.
+  Navigate(app_browser_1, in_scope_2_);
+  EXPECT_EQ(app_browser_2, BrowserList::GetInstance()->GetLastActive());
+  ExpectTabs(browser(), {out_of_scope_});
+  ExpectTabs(app_browser_1, {in_scope_2_});
+  ExpectTabs(app_browser_2, {in_scope_2_});
+}
+
+IN_PROC_BROWSER_TEST_F(WebAppDeclarativeLinkCapturingBrowserTest,
+                       InAppScopeNavigationIgnored) {
+  InstallTestApp("/web_apps/capture_links_new_client.html");
+
+  // Start browser in app scope.
+  AddTab(browser(), in_scope_1_);
+
+  // Navigations that happen inside the app scope should not capture even if
+  // done outside of an app window.
+  Navigate(browser(), in_scope_2_);
+  EXPECT_EQ(browser(), BrowserList::GetInstance()->GetLastActive());
+  ExpectTabs(browser(), {about_blank_, in_scope_2_});
+}
+
 }  // namespace web_app

chrome/test/data/web_apps/capture_links_new_client.html

+<!DOCTYPE html>
+<head>
+  <title>"capture_links": "new-client"</title>
+  <link rel="manifest" href="capture_links_new_client.json">
+</head>

chrome/test/data/web_apps/capture_links_new_client.json

+{
+  "name": "\"capture_links\": \"new-client\"",
+  "start_url": "capture_links_new_client.html",
+  "display": "standalone",
+  "capture_links": "new-client"
+}

chrome/test/data/web_apps/capture_links_none.html

+<!DOCTYPE html>
+<head>
+  <title>"capture_links": "none"</title>
+  <link rel="manifest" href="capture_links_none.json">
+</head>

chrome/test/data/web_apps/capture_links_none.json

+{
+  "name": "\"capture_links\": \"none\"",
+  "start_url": "capture_links_none.html",
+  "display": "standalone",
+  "capture_links": "none"
+}