[dPWA] Move desktop intent picker behavior under desktop PWA flag

This CL moves the intent picker behavior for desktop (non-CrOS)
under the kDesktopPWAsLinkCapturing flag. As a result of this, the
following changes are made:
1. Ensuring the 2 different flags for CrOS and desktop are each
taken into account per platform when invoked from multiple
places.
2. This fixes crbug.com/1491258, where the OneOffIconObserver
is deprecated in favor of an IntentChipButton observer.
3. Tests are updated with the new flag which helps verify that
the desktop implementation works with all preexisting tests.

Bug: 1412856, b/304411994
Change-Id: I4712146637bbcd3fdc2a66a53f298e4c270eb5a0
Fixed: 1491258
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4967393
Reviewed-by: Mike Wasserman <msw@chromium.org>
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Commit-Queue: Dibyajyoti Pal <dibyapal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217169}

chrome/browser/apps/link_capturing/BUILD.gn

@@ -74,6 +74,18 @@ source_set("link_capturing") {
   }
 }
 
+source_set("test_support") {
+  testonly = true
+  sources = [
+    "link_capturing_feature_test_support.cc",
+    "link_capturing_feature_test_support.h",
+  ]
+  deps = [
+    ":link_capturing",
+    "//base/test:test_support",
+  ]
+}
+
 source_set("unit_tests") {
   testonly = true
   sources = [ "link_capturing_navigation_throttle_unittest.cc" ]

chrome/browser/apps/link_capturing/chromeos_apps_intent_picker_delegate.cc

@@ -155,10 +155,10 @@ bool ChromeOsAppsIntentPickerDelegate::ShouldLaunchAppDirectly(
     const GURL& url,
     const std::string& app_name) {
   // If there is only a single available app, immediately launch it if
-  // LinkCapturingUiUpdateEnabled() is enabled and the app is preferred for this
+  // ShouldShowLinkCapturingUX() is enabled and the app is preferred for this
   // link.
   CHECK(proxy_);
-  return apps::features::LinkCapturingUiUpdateEnabled() &&
+  return apps::features::ShouldShowLinkCapturingUX() &&
          (proxy_->PreferredAppsList().FindPreferredAppForUrl(url) == app_name);
 }
 

chrome/browser/apps/link_capturing/link_capturing_feature_test_support.cc

@@ -0,0 +1,36 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/apps/link_capturing/link_capturing_feature_test_support.h"
+
+#include "base/check_is_test.h"
+#include "base/test/scoped_feature_list.h"
+
+namespace apps {
+
+void EnableLinkCapturingUXForTesting(
+    base::test::ScopedFeatureList& scoped_feature_list) {
+  CHECK_IS_TEST();
+#if BUILDFLAG(IS_CHROMEOS)
+  scoped_feature_list.InitAndEnableFeature(
+      apps::features::kLinkCapturingUiUpdate);
+#else
+  scoped_feature_list.InitAndEnableFeature(
+      apps::features::kDesktopPWAsLinkCapturing);
+#endif  // BUILDFLAG(IS_CHROMEOS)
+}
+
+void DisableLinkCapturingUXForTesting(
+    base::test::ScopedFeatureList& scoped_feature_list) {
+  CHECK_IS_TEST();
+#if BUILDFLAG(IS_CHROMEOS)
+  scoped_feature_list.InitAndDisableFeature(
+      apps::features::kLinkCapturingUiUpdate);
+#else
+  scoped_feature_list.InitAndDisableFeature(
+      apps::features::kDesktopPWAsLinkCapturing);
+#endif  // BUILDFLAG(IS_CHROMEOS)
+}
+
+}  // namespace apps

chrome/browser/apps/link_capturing/link_capturing_feature_test_support.h

@@ -0,0 +1,25 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_APPS_LINK_CAPTURING_LINK_CAPTURING_FEATURE_TEST_SUPPORT_H_
+#define CHROME_BROWSER_APPS_LINK_CAPTURING_LINK_CAPTURING_FEATURE_TEST_SUPPORT_H_
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/apps/link_capturing/link_capturing_features.h"
+
+namespace apps {
+
+// The functions should only be called from tests, and is used to enable or
+// disable link capturing UXes. Only use these if link capturing needs to be
+// enabled on all platforms, i.e. ChromeOS, Windows, Mac and Linux. For platform
+// specific implementations, prefer initializing the feature list in the test
+// file itself.
+void EnableLinkCapturingUXForTesting(
+    base::test::ScopedFeatureList& scoped_feature_list);
+void DisableLinkCapturingUXForTesting(
+    base::test::ScopedFeatureList& scoped_feature_list);
+
+}  // namespace apps
+
+#endif  // CHROME_BROWSER_APPS_LINK_CAPTURING_LINK_CAPTURING_FEATURE_TEST_SUPPORT_H_

chrome/browser/apps/link_capturing/link_capturing_features.cc

@@ -11,18 +11,15 @@ namespace apps::features {
 
 // TODO(crbug.com/1357905): Remove feature on ChromeOS once all tests pass with
 // updated UI.
+#if BUILDFLAG(IS_CHROMEOS)
 BASE_FEATURE(kLinkCapturingUiUpdate,
              "LinkCapturingUiUpdate",
-#if BUILDFLAG(IS_CHROMEOS)
-             base::FEATURE_ENABLED_BY_DEFAULT
+             base::FEATURE_ENABLED_BY_DEFAULT);
 #else
-             base::FEATURE_DISABLED_BY_DEFAULT
-#endif
-);
-
 BASE_FEATURE(kDesktopPWAsLinkCapturing,
              "DesktopPWAsLinkCapturing",
              base::FEATURE_DISABLED_BY_DEFAULT);
+#endif
 
 #if BUILDFLAG(IS_CHROMEOS)
 BASE_FEATURE(kAppToAppLinkCapturing,
@@ -34,8 +31,12 @@ BASE_FEATURE(kAppToAppLinkCapturingWorkspaceApps,
              base::FEATURE_DISABLED_BY_DEFAULT);
 #endif
 
-bool LinkCapturingUiUpdateEnabled() {
+bool ShouldShowLinkCapturingUX() {
+#if BUILDFLAG(IS_CHROMEOS)
   return base::FeatureList::IsEnabled(kLinkCapturingUiUpdate);
+#else
+  return base::FeatureList::IsEnabled(kDesktopPWAsLinkCapturing);
+#endif  // BUILDFLAG(IS_CHROMEOS)
 }
 
 }  // namespace apps::features

chrome/browser/apps/link_capturing/link_capturing_features.h

@@ -6,14 +6,18 @@
 #define CHROME_BROWSER_APPS_LINK_CAPTURING_LINK_CAPTURING_FEATURES_H_
 
 #include "base/feature_list.h"
+#include "build/build_config.h"
 
 namespace apps::features {
 
+#if BUILDFLAG(IS_CHROMEOS)
+// Enables user link capturing on CrOS.
 BASE_DECLARE_FEATURE(kLinkCapturingUiUpdate);
-
+#else
 // Enables user link capturing on desktop platforms, i.e. Windows, Mac
 // Linux amd Fuchsia.
 BASE_DECLARE_FEATURE(kDesktopPWAsLinkCapturing);
+#endif  // BUILDFLAG(IS_CHROMEOS)
 
 #if BUILDFLAG(IS_CHROMEOS)
 // Enables link capturing into apps when links are clicked from another app
@@ -25,8 +29,10 @@ BASE_DECLARE_FEATURE(kAppToAppLinkCapturing);
 BASE_DECLARE_FEATURE(kAppToAppLinkCapturingWorkspaceApps);
 #endif
 
-// Returns true if the overall link capturing UI update feature is enabled.
-bool LinkCapturingUiUpdateEnabled();
+// Returns true if the updated UX for link capturing needs to be shown. Only set
+// to true if kDesktopPWAsLinkCapturing is enabled on desktop platforms, and
+// kLinkCapturingUiUpdate on CrOS platforms.
+bool ShouldShowLinkCapturingUX();
 
 }  // namespace apps::features
 

chrome/browser/ui/intent_picker_tab_helper.cc

@@ -167,7 +167,7 @@ void IntentPickerTabHelper::ShowOrHideIcon(content::WebContents* web_contents,
   if (!tab_helper)
     return;
 
-  if (apps::features::LinkCapturingUiUpdateEnabled()) {
+  if (apps::features::ShouldShowLinkCapturingUX()) {
     tab_helper->current_app_icon_ = ui::ImageModel();
     tab_helper->show_expanded_chip_from_usage_ = false;
     tab_helper->current_app_id_ = std::string();
@@ -181,7 +181,7 @@ void IntentPickerTabHelper::ShowOrHideIcon(content::WebContents* web_contents,
 // static
 int IntentPickerTabHelper::GetIntentPickerBubbleIconSize() {
   const int kIntentPickerUiUpdateIconSize = 40;
-  return apps::features::LinkCapturingUiUpdateEnabled()
+  return apps::features::ShouldShowLinkCapturingUX()
              ? kIntentPickerUiUpdateIconSize
              : gfx::kFaviconSize;
 }
@@ -202,7 +202,7 @@ void IntentPickerTabHelper::MaybeShowIconForApps(
 #endif  // BUILDFLAG(IS_CHROMEOS)
   }
 
-  if (apps::features::LinkCapturingUiUpdateEnabled()) {
+  if (apps::features::ShouldShowLinkCapturingUX()) {
     if (apps.size() == 1 && apps[0].launch_name != current_app_id_) {
       current_app_id_ = apps[0].launch_name;
 
@@ -323,7 +323,7 @@ void IntentPickerTabHelper::OnAppIconLoadedForChip(const std::string& app_id,
 }
 
 void IntentPickerTabHelper::ShowIconForLinkIntent(bool should_show_icon) {
-  if (apps::features::LinkCapturingUiUpdateEnabled()) {
+  if (apps::features::ShouldShowLinkCapturingUX()) {
     UpdateExpandedState(should_show_icon);
   }
 
@@ -362,7 +362,7 @@ void IntentPickerTabHelper::ShowIntentPickerOrLaunchAppImpl(
                               url, apps[0].launch_name)) {
     // TODO(b/305075981): Move IntentChipDisplayPrefs to
     // c/b/apps/link_capturing.
-    if (apps::features::LinkCapturingUiUpdateEnabled()) {
+    if (apps::features::ShouldShowLinkCapturingUX()) {
       Profile* profile =
           Profile::FromBrowserContext(web_contents()->GetBrowserContext());
       IntentChipDisplayPrefs::ResetIntentChipCounter(profile, url);
@@ -414,7 +414,7 @@ void IntentPickerTabHelper::OnIntentPickerClosedMaybeLaunch(
   if (should_launch_app) {
     // TODO(b/305075981): Move IntentChipDisplayPrefs to
     // c/b/apps/link_capturing.
-    if (apps::features::LinkCapturingUiUpdateEnabled()) {
+    if (apps::features::ShouldShowLinkCapturingUX()) {
       Profile* profile =
           Profile::FromBrowserContext(web_contents()->GetBrowserContext());
       IntentChipDisplayPrefs::ResetIntentChipCounter(profile, url);

chrome/browser/ui/intent_picker_tab_helper_unittest.cc

@@ -9,7 +9,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/apps/intent_helper/intent_chip_display_prefs.h"
 #include "chrome/browser/apps/link_capturing/intent_picker_info.h"
-#include "chrome/browser/apps/link_capturing/link_capturing_features.h"
+#include "chrome/browser/apps/link_capturing/link_capturing_feature_test_support.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/models/image_model.h"
@@ -38,7 +38,18 @@ class IntentPickerTabHelperTest : public ChromeRenderViewHostTestHarness {
   raw_ptr<IntentPickerTabHelper, DanglingUntriaged> helper_;
 };
 
-TEST_F(IntentPickerTabHelperTest, ShowOrHideIcon) {
+class IntentPickerTabHelperPlatformAgnosticTest
+    : public IntentPickerTabHelperTest {
+ public:
+  IntentPickerTabHelperPlatformAgnosticTest() {
+    apps::EnableLinkCapturingUXForTesting(scoped_feature_list_);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(IntentPickerTabHelperPlatformAgnosticTest, ShowOrHideIcon) {
   IntentPickerTabHelper::ShowOrHideIcon(web_contents(),
                                         /*should_show_icon=*/true);
 
@@ -50,19 +61,15 @@ TEST_F(IntentPickerTabHelperTest, ShowOrHideIcon) {
   ASSERT_FALSE(helper()->should_show_icon());
 }
 
-TEST_F(IntentPickerTabHelperTest, ShowIconForApps) {
-  base::test::ScopedFeatureList feature_list(
-      apps::features::kLinkCapturingUiUpdate);
-
+TEST_F(IntentPickerTabHelperPlatformAgnosticTest, ShowIconForApps) {
   NavigateAndCommit(GURL("https://www.google.com"));
   helper()->MaybeShowIconForApps(CreateTestAppList());
 
   ASSERT_TRUE(helper()->should_show_icon());
 }
 
-TEST_F(IntentPickerTabHelperTest, ShowIconForApps_ExpandedChip) {
-  base::test::ScopedFeatureList feature_list(
-      apps::features::kLinkCapturingUiUpdate);
+TEST_F(IntentPickerTabHelperPlatformAgnosticTest,
+       ShowIconForApps_ExpandedChip) {
   const GURL kTestUrl = GURL("https://www.google.com");
 
   NavigateAndCommit(kTestUrl);
@@ -71,9 +78,8 @@ TEST_F(IntentPickerTabHelperTest, ShowIconForApps_ExpandedChip) {
   ASSERT_TRUE(helper()->ShouldShowExpandedChip());
 }
 
-TEST_F(IntentPickerTabHelperTest, ShowIconForApps_CollapsedChip) {
-  base::test::ScopedFeatureList feature_list(
-      apps::features::kLinkCapturingUiUpdate);
+TEST_F(IntentPickerTabHelperPlatformAgnosticTest,
+       ShowIconForApps_CollapsedChip) {
   const GURL kTestUrl = GURL("https://www.google.com");
 
   // Simulate having seen the chip for this URL several times before, so that it
@@ -90,9 +96,8 @@ TEST_F(IntentPickerTabHelperTest, ShowIconForApps_CollapsedChip) {
   ASSERT_FALSE(helper()->ShouldShowExpandedChip());
 }
 
-TEST_F(IntentPickerTabHelperTest, ShowIntentIcon_ResetsExpandedState) {
-  base::test::ScopedFeatureList feature_list(
-      apps::features::kLinkCapturingUiUpdate);
+TEST_F(IntentPickerTabHelperPlatformAgnosticTest,
+       ShowIntentIcon_ResetsExpandedState) {
   const GURL kTestUrl = GURL("https://www.google.com");
 
   NavigateAndCommit(kTestUrl);

chrome/browser/ui/views/enable_link_capturing_infobar_browsertest.cc

@@ -19,8 +19,8 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
 #include "chrome/browser/ui/views/intent_picker_bubble_view.h"
-#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
-#include "chrome/browser/ui/views/page_action/page_action_icon_view_observer.h"
+#include "chrome/browser/ui/views/location_bar/intent_chip_button.h"
+#include "chrome/browser/ui/views/location_bar/omnibox_chip_button.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/ui/web_applications/test/web_app_navigation_browsertest.h"
@@ -49,20 +49,23 @@ content::WebContents* GetActiveWebContents(Browser* browser) {
   return browser->tab_strip_model()->GetActiveWebContents();
 }
 
-// TODO(dmurph): Remove this after all of the updating to the launch page action
-// view and we can know that updating is synchronous. https://crbug.com/1491258
-class OneOffIconObserver : public PageActionIconViewObserver {
+class IntentChipVisibilityObserver : public OmniboxChipButton::Observer {
  public:
-  explicit OneOffIconObserver(base::OnceClosure on_shown)
-      : on_shown_(std::move(on_shown)) {}
+  explicit IntentChipVisibilityObserver(IntentChipButton* intent_chip) {
+    observation_.Observe(intent_chip);
+  }
 
-  void OnPageActionIconViewShown(PageActionIconView* view) override {
-    std::move(on_shown_).Run();
-    view->RemovePageIconViewObserver(this);
+  void WaitForChipToBeVisible() { run_loop.Run(); }
+  void OnChipVisibilityChanged(bool is_visible) override {
+    if (is_visible) {
+      run_loop.Quit();
+    }
   }
 
  private:
-  base::OnceClosure on_shown_;
+  base::ScopedObservation<IntentChipButton, OmniboxChipButton::Observer>
+      observation_{this};
+  base::RunLoop run_loop;
 };
 
 class EnableLinkCapturingInfobarBrowserTest
@@ -73,10 +76,10 @@ class EnableLinkCapturingInfobarBrowserTest
         apps::features::kDesktopPWAsLinkCapturing);
   }
 
-  PageActionIconView* GetIntentPickerIcon() {
+  IntentChipButton* GetIntentPickerIcon() {
     return BrowserView::GetBrowserViewForBrowser(browser())
         ->toolbar_button_provider()
-        ->GetPageActionIconView(PageActionIconType::kIntentPicker);
+        ->GetIntentChipButton();
   }
 
   // Returns [app_id, in_scope_url]
@@ -144,19 +147,19 @@ class EnableLinkCapturingInfobarBrowserTest
              << "Intent picker app did not resolve an applicable app.";
     }
 
-    PageActionIconView* intent_picker_icon = GetIntentPickerIcon();
+    IntentChipButton* intent_picker_icon = GetIntentPickerIcon();
     if (!intent_picker_icon) {
       return testing::AssertionFailure()
              << "Intent picker icon does not exist.";
     }
 
     if (!intent_picker_icon->GetVisible()) {
-      base::test::TestFuture<void> icon_visible;
-      OneOffIconObserver observer(icon_visible.GetCallback());
-      intent_picker_icon->AddPageIconViewObserver(&observer);
-      if (!icon_visible.Wait()) {
+      IntentChipVisibilityObserver intent_chip_visibility_observer(
+          intent_picker_icon);
+      intent_chip_visibility_observer.WaitForChipToBeVisible();
+      if (!intent_picker_icon->GetVisible()) {
         return testing::AssertionFailure()
-               << "Intent picker icon never became visible.";
+               << "Intent picker icon did not become visible.";
       }
     }
     EXPECT_TRUE(intent_picker_icon->GetVisible());

chrome/browser/ui/views/intent_picker_bubble_view.cc

@@ -623,7 +623,7 @@ IntentPickerBubbleView::IntentPickerBubbleView(
     : LocationBarBubbleDelegateView(anchor_view, web_contents),
       intent_picker_cb_(std::move(intent_picker_cb)),
       app_info_(std::move(app_info)),
-      use_grid_view_(apps::features::LinkCapturingUiUpdateEnabled() &&
+      use_grid_view_(apps::features::ShouldShowLinkCapturingUX() &&
                      bubble_type == BubbleType::kLinkCapturing),
       show_stay_in_chrome_(show_stay_in_chrome && !use_grid_view_),
       show_remember_selection_(show_remember_selection),

chrome/browser/ui/views/intent_picker_bubble_view_browsertest.cc

@@ -80,7 +80,7 @@ class IntentPickerBrowserTest
   }
 
   views::Button* GetIntentPickerIcon() {
-    if (apps::features::LinkCapturingUiUpdateEnabled()) {
+    if (apps::features::ShouldShowLinkCapturingUX()) {
       return BrowserView::GetBrowserViewForBrowser(browser())
           ->toolbar_button_provider()
           ->GetIntentChipButton();