Use the ComposeManager to check if compose should be offered in the m…

…enu. Splits the #ShouldShow checks into two, one for the popup and one for the context menu (as the context menu does not have the FormFieldData at that time). At the moment, does not enable for ContentEditables as the renderer ids are not populated for them yet. Screenshot: https://drive.google.com/file/d/1JWREZQ5QN9rk_YUxN2EOzctEDAdPQ8GU/view?usp=share_link&resourcekey=0-HWBtL-Pi__C6OGcGlCDtHw Bug: 301371110 Change-Id: Iad8f2791b1f4447f21b6c8cdad557af9689d8803 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935166 Reviewed-by: Peter Williamson <petewil@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Sophey Dong <sophey@chromium.org> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#1212240}
chromium · Oct 19, 2023 · 4f6614d · 4f6614d
1 parent c1928d6
commit 4f6614d
Show file tree

Hide file tree

Showing 17 changed files with 222 additions and 63 deletions.
diff --git a/chrome/browser/DEPS b/chrome/browser/DEPS
@@ -608,7 +608,6 @@ specific_include_rules = {
     "+components/services/storage/shared_storage/shared_storage_manager.h",
   ],
   "render_view_context_menu_browsertest.cc" : [
-    "+components/compose/buildflags.h",
     "+third_party/libwebp/src/src/webp/decode.h",
   ],
   "webauth_interactive_uitest.cc": [

diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu.cc b/chrome/browser/renderer_context_menu/render_view_context_menu.cc
@@ -129,8 +129,8 @@
 #include "components/autofill/core/browser/ui/popup_types.h"
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/autofill/core/common/password_generation_util.h"
+#include "components/autofill/core/common/unique_ids.h"
 #include "components/compose/buildflags.h"
-#include "components/compose/core/browser/compose_features.h"
 #include "components/custom_handlers/protocol_handler.h"
 #include "components/download/public/common/download_url_parameters.h"
 #include "components/feed/feed_feature_list.h"
@@ -224,15 +224,15 @@
 #include "ui/strings/grit/ui_strings.h"
 #include "url/origin.h"
 
+#if BUILDFLAG(USE_RENDERER_SPELLCHECKER)
+#include "chrome/browser/renderer_context_menu/spelling_options_submenu_observer.h"
+#endif
+
 #if BUILDFLAG(ENABLE_COMPOSE)
 #include "chrome/browser/compose/chrome_compose_client.h"
 #include "components/compose/core/browser/compose_manager.h"
 #endif
 
-#if BUILDFLAG(USE_RENDERER_SPELLCHECKER)
-#include "chrome/browser/renderer_context_menu/spelling_options_submenu_observer.h"
-#endif
-
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/devtools_util.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -1518,6 +1518,13 @@ std::u16string RenderViewContextMenu::GetTargetLanguageDisplayName(
   return l10n_util::GetDisplayNameForLocale(target, target, true);
 }
 
+#if BUILDFLAG(ENABLE_COMPOSE)
+compose::ComposeManager* RenderViewContextMenu::GetComposeManager() const {
+  auto* client = ChromeComposeClient::FromWebContents(source_web_contents_);
+  return client ? &client->GetManager() : nullptr;
+}
+#endif  // BUILDFLAG(ENABLE_COMPOSE)
+
 void RenderViewContextMenu::AppendDeveloperItems() {
   // Do not Show Inspect Element for DevTools unless DevTools runs with the
   // debugFrontend query param.
@@ -2250,17 +2257,24 @@ void RenderViewContextMenu::AppendSpellingAndSearchSuggestionItems() {
                                       IDS_CONTENT_CONTEXT_EMOJI);
     }
   }
-  // TODO(b/301371110): Update enabling constraints.
-  if (base::FeatureList::IsEnabled(compose::features::kEnableCompose)) {
-    menu_model_.AddItemWithStringId(IDC_CONTEXT_COMPOSE,
-                                    IDS_COMPOSE_SUGGESTION_MAIN_TEXT);
-    // TODO(b/303646344): Remove new feature tag when no longer new.
-    menu_model_.SetIsNewFeatureAt(
-        menu_model_.GetItemCount() - 1,
-        !content_type_->SupportsGroup(ContextMenuContentType::ITEM_GROUP_LINK));
+#if BUILDFLAG(ENABLE_COMPOSE)
+  RenderFrameHost* render_frame_host = GetRenderFrameHost();
+  if (render_frame_host && params_.form_renderer_id &&
+      params_.field_renderer_id) {
+    compose::ComposeManager* compose_manager = GetComposeManager();
+    if (compose_manager && compose_manager->ShouldOfferComposeContextMenu()) {
+      menu_model_.AddItemWithStringId(IDC_CONTEXT_COMPOSE,
+                                      IDS_COMPOSE_SUGGESTION_MAIN_TEXT);
+      // TODO(b/303646344): Remove new feature tag when no longer new.
+      menu_model_.SetIsNewFeatureAt(
+          menu_model_.GetItemCount() - 1,
+          !content_type_->SupportsGroup(
+              ContextMenuContentType::ITEM_GROUP_LINK));
 
-    render_separator = true;
+      render_separator = true;
+    }
   }
+#endif  // BUILDFLAG(ENABLE_COMPOSE)
   if (render_separator) {
     menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
   }

diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu.h b/chrome/browser/renderer_context_menu/render_view_context_menu.h
@@ -18,6 +18,7 @@
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/ui/autofill/autofill_context_menu_manager.h"
+#include "components/compose/buildflags.h"
 #include "components/custom_handlers/protocol_handler_registry.h"
 #include "components/lens/buildflags.h"
 #include "components/renderer_context_menu/context_menu_content_type.h"
@@ -37,6 +38,10 @@
 #include "ui/base/window_open_disposition.h"
 #include "ui/gfx/geometry/vector2d.h"
 
+#if BUILDFLAG(ENABLE_COMPOSE)
+#include "components/compose/core/browser/compose_manager.h"
+#endif
+
 #if BUILDFLAG(ENABLE_LENS_DESKTOP_GOOGLE_BRANDED_FEATURES)
 #include "chrome/browser/lens/region_search/lens_region_search_controller.h"
 #endif
@@ -196,6 +201,10 @@ class RenderViewContextMenu
   virtual const policy::DlpRulesManager* GetDlpRulesManager() const;
 #endif
 
+#if BUILDFLAG(ENABLE_COMPOSE)
+  virtual compose::ComposeManager* GetComposeManager() const;
+#endif
+
   // RenderViewContextMenuBase:
   // If called in Ash when Lacros is the only browser, this open the URL in
   // Lacros. In that case, only the |url| and some values of |disposition| are

diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
@@ -134,7 +134,7 @@
 #include "ui/gfx/codec/png_codec.h"
 
 #if BUILDFLAG(ENABLE_COMPOSE)
-#include "components/compose/core/browser/compose_features.h"
+#include "components/compose/core/browser/mock_compose_manager.h"
 #endif
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
@@ -173,6 +173,10 @@ using extensions::TestMimeHandlerViewGuest;
 using web_app::WebAppProvider;
 using webapps::AppId;
 
+using ::testing::_;
+using ::testing::Return;
+using ::testing::TestWithParam;
+
 namespace {
 
 const char kAppUrl1[] = "https://www.google.com/";
@@ -1152,38 +1156,72 @@ IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 #if BUILDFLAG(ENABLE_COMPOSE)
-class ContextMenuForComposeBrowserTest : public ContextMenuBrowserTest {
- public:
-  ContextMenuForComposeBrowserTest() {
-    scoped_feature_list_.InitWithFeatures(
-        /*enabled_features=*/{compose::features::kEnableCompose},
-        /*disabled_features=*/{});
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
+struct ContextMenuForComposeTestCase {
+  std::string test_name;
+  bool is_editable;
+  absl::optional<uint64_t> form_renderer_id;
+  absl::optional<uint64_t> field_renderer_id;
+  bool should_offer_compose_context_menu;
+  bool expected;
 };
 
-IN_PROC_BROWSER_TEST_F(ContextMenuForComposeBrowserTest,
-                       ContextMenuForCompose_Editable) {
-  content::ContextMenuParams params;
-  params.is_editable = true;
+class ContextMenuForComposeBrowserTest
+    : public ContextMenuBrowserTest,
+      public ::testing::WithParamInterface<ContextMenuForComposeTestCase> {};
 
-  auto menu = CreateContextMenuFromParams(params);
+IN_PROC_BROWSER_TEST_P(ContextMenuForComposeBrowserTest,
+                       TestComposeItemPresent) {
+  const ContextMenuForComposeTestCase& test_case = GetParam();
 
-  EXPECT_TRUE(menu->IsItemPresent(IDC_CONTEXT_COMPOSE));
-}
-
-IN_PROC_BROWSER_TEST_F(ContextMenuForComposeBrowserTest,
-                       ContextMenuForCompose_NonEditable) {
   content::ContextMenuParams params;
-  params.is_editable = false;
-
-  auto menu = CreateContextMenuFromParams(params);
-
-  // Compose context menu item should never be present on a non-editable field.
-  EXPECT_FALSE(menu->IsItemPresent(IDC_CONTEXT_COMPOSE));
-}
+  params.is_editable = test_case.is_editable;
+  params.form_renderer_id = test_case.form_renderer_id;
+  params.field_renderer_id = test_case.field_renderer_id;
+  compose::MockComposeManager compose_manager;
+  ON_CALL(compose_manager, ShouldOfferComposeContextMenu())
+      .WillByDefault(Return(test_case.should_offer_compose_context_menu));
+
+  auto menu =
+      std::make_unique<TestRenderViewContextMenu>(*browser()
+                                                       ->tab_strip_model()
+                                                       ->GetActiveWebContents()
+                                                       ->GetPrimaryMainFrame(),
+                                                  params);
+  menu->SetComposeManager(&compose_manager);
+  menu->Init();
+
+  ASSERT_EQ(menu->IsItemPresent(IDC_CONTEXT_COMPOSE), test_case.expected);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    ContextMenuBrowserTests,
+    ContextMenuForComposeBrowserTest,
+    ::testing::ValuesIn<ContextMenuForComposeTestCase>({
+        {"Enabled", /*is_editable=*/true,
+         /*form_renderer_id=*/absl::make_optional(123),
+         /*field_renderer_id=*/absl::make_optional(456),
+         /*should_offer_compose_context_menu=*/true, /*expected=*/true},
+        {"NotEditable", /*is_editable=*/false,
+         /*form_renderer_id=*/absl::make_optional(123),
+         /*field_renderer_id=*/absl::make_optional(456),
+         /*should_offer_compose_context_menu=*/true, /*expected=*/false},
+        {"NoFormRendererId", /*is_editable=*/true,
+         /*form_renderer_id=*/absl::nullopt,
+         /*field_renderer_id=*/absl::make_optional(456),
+         /*should_offer_compose_context_menu=*/true, /*expected=*/false},
+        {"NoFieldRendererId", /*is_editable=*/true,
+         /*form_renderer_id=*/absl::make_optional(123),
+         /*field_renderer_id=*/absl::nullopt,
+         /*should_offer_compose_context_menu=*/true, /*expected=*/false},
+        {"ShouldNotOffer", /*is_editable=*/true,
+         /*form_renderer_id=*/absl::make_optional(123),
+         /*field_renderer_id=*/absl::make_optional(456),
+         /*should_offer_compose_context_menu=*/false, /*expected=*/false},
+    }),
+    [](const testing::TestParamInfo<
+        ContextMenuForComposeBrowserTest::ParamType>& info) {
+      return info.param.test_name;
+    });
 #endif  // BUILDFLAG(ENABLE_COMPOSE)
 
 IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, CopyLinkTextMouse) {

diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
@@ -137,3 +137,14 @@ void TestRenderViewContextMenu::set_dlp_rules_manager(
   dlp_rules_manager_ = dlp_rules_manager;
 }
 #endif
+
+#if BUILDFLAG(ENABLE_COMPOSE)
+compose::ComposeManager* TestRenderViewContextMenu::GetComposeManager() const {
+  return compose_manager_;
+}
+
+void TestRenderViewContextMenu::SetComposeManager(
+    compose::ComposeManager* compose_manager) {
+  compose_manager_ = compose_manager;
+}
+#endif  // BUILDFLAG(ENABLE_COMPOSE)
diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h b/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
@@ -113,6 +113,9 @@ class TestRenderViewContextMenu : public RenderViewContextMenu {
   void set_dlp_rules_manager(policy::DlpRulesManager* dlp_rules_manager);
 #endif
 
+#if BUILDFLAG(ENABLE_COMPOSE)
+  void SetComposeManager(compose::ComposeManager* compose_manager_);
+#endif
   // If `browser` is not null, sets it as the return value of GetBrowser(),
   // overriding the base class behavior. If the Browser object is destroyed
   // before this class is, then SetBrowser(nullptr) should be called. If
@@ -123,13 +126,21 @@ class TestRenderViewContextMenu : public RenderViewContextMenu {
   // RenderViewContextMenu:
   Browser* GetBrowser() const override;
 
+#if BUILDFLAG(ENABLE_COMPOSE)
+  compose::ComposeManager* GetComposeManager() const override;
+#endif
+
  private:
   raw_ptr<Browser> browser_ = nullptr;
 
 #if BUILDFLAG(IS_CHROMEOS)
   raw_ptr<policy::DlpRulesManager, ExperimentalAsh> dlp_rules_manager_ =
       nullptr;
 #endif
+
+#if BUILDFLAG(ENABLE_COMPOSE)
+  raw_ptr<compose::ComposeManager> compose_manager_ = nullptr;
+#endif
 };
 
 #endif  // CHROME_BROWSER_RENDERER_CONTEXT_MENU_RENDER_VIEW_CONTEXT_MENU_TEST_UTIL_H_
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -1660,7 +1660,7 @@ if (!is_android) {
       "//components/commerce/core:shopping_service_test_support",
       "//components/component_updater/installer_policies:installer_policies_no_content_deps",
       "//components/compose:buildflags",
-      "//components/compose/core/browser:features",
+      "//components/compose/core/browser:test_support",
       "//components/constrained_window",
       "//components/content_settings/browser",
       "//components/content_settings/common:mojom",

diff --git a/components/autofill/core/browser/autofill_compose_delegate.h b/components/autofill/core/browser/autofill_compose_delegate.h
@@ -37,10 +37,8 @@ class AutofillComposeDelegate {
     kAutofillPopup,
     kContextMenu,
   };
-  // Returns whether compose is available for this `ui_entry_point` and
-  // `trigger_field`.
-  virtual bool ShouldOfferCompose(UiEntryPoint ui_entry_point,
-                                  const FormFieldData& trigger_field) = 0;
+  // Returns whether the compose popup is available for this `trigger_field`.
+  virtual bool ShouldOfferComposePopup(const FormFieldData& trigger_field) = 0;
 
   // Opens the Compose UI. `ui_entry_point` and `trigger_field` describe the
   // field on which Compose was triggered. `popup_screen_location` contains the

diff --git a/components/autofill/core/browser/browser_autofill_manager.cc b/components/autofill/core/browser/browser_autofill_manager.cc
@@ -3856,9 +3856,7 @@ BrowserAutofillManager::MaybeGetPlusAddressSuggestion() {
 absl::optional<Suggestion> BrowserAutofillManager::MaybeGetComposeSuggestion(
     const FormFieldData& field) {
   AutofillComposeDelegate* compose_delegate = client().GetComposeDelegate();
-  if (!compose_delegate ||
-      !compose_delegate->ShouldOfferCompose(
-          AutofillComposeDelegate::UiEntryPoint::kAutofillPopup, field)) {
+  if (!compose_delegate || !compose_delegate->ShouldOfferComposePopup(field)) {
     return absl::nullopt;
   }
   Suggestion suggestion(

diff --git a/components/autofill/core/browser/browser_autofill_manager_unittest.cc b/components/autofill/core/browser/browser_autofill_manager_unittest.cc
@@ -10082,7 +10082,7 @@ TEST_F(BrowserAutofillManagerTest, NoComposeSuggestionsByDefault) {
   // The third field is meant to correspond to address line 1. For that (unlike
   // for first and last name), parsing also derives that type if it is a
   // textarea.
-  EXPECT_CALL(compose_delegate, ShouldOfferCompose).Times(0);
+  EXPECT_CALL(compose_delegate, ShouldOfferComposePopup).Times(0);
   GetAutofillSuggestions(form, form.fields[3]);
   CheckSuggestions(form.fields[3].global_id(),
                    Suggestion("123 Apple St., unit 6", "123 Apple St.",
@@ -10106,11 +10106,9 @@ TEST_F(BrowserAutofillManagerTest, ComposeSuggestionsAreQueriedForTextareas) {
   form.action = GURL("https://myform.com/submit.html");
   FormsSeen({form});
 
-  EXPECT_CALL(
-      compose_delegate,
-      ShouldOfferCompose(
-          AutofillComposeDelegate::UiEntryPoint::kAutofillPopup,
-          Property(&FormFieldData::global_id, Eq(form.fields[0].global_id()))));
+  EXPECT_CALL(compose_delegate,
+              ShouldOfferComposePopup(Property(
+                  &FormFieldData::global_id, Eq(form.fields[0].global_id()))));
   GetAutofillSuggestions(form, form.fields[0]);
 }
 

diff --git a/components/autofill/core/browser/mock_autofill_compose_delegate.h b/components/autofill/core/browser/mock_autofill_compose_delegate.h
@@ -16,8 +16,8 @@ class MockAutofillComposeDelegate : public AutofillComposeDelegate {
   ~MockAutofillComposeDelegate() override;
 
   MOCK_METHOD(bool,
-              ShouldOfferCompose,
-              (UiEntryPoint, const FormFieldData&),
+              ShouldOfferComposePopup,
+              (const FormFieldData&),
               (override));
   MOCK_METHOD(void,
               OpenCompose,

diff --git a/components/compose/core/browser/BUILD.gn b/components/compose/core/browser/BUILD.gn
@@ -28,3 +28,19 @@ static_library("browser") {
     "//components/keyed_service/core",
   ]
 }
+
+static_library("test_support") {
+  testonly = true
+  sources = [
+    "mock_compose_manager.cc",
+    "mock_compose_manager.h",
+  ]
+
+  public_deps = [ ":browser" ]
+
+  deps = [
+    "//components/autofill/core/browser",
+    "//components/autofill/core/common",
+    "//testing/gmock",
+  ]
+}
diff --git a/components/compose/core/browser/compose_manager.h b/components/compose/core/browser/compose_manager.h
@@ -13,6 +13,9 @@ namespace compose {
 class ComposeManager : public autofill::AutofillComposeDelegate {
  public:
   // TODO(b/300325327): Add non-Autofill specific methods.
+  // Returns whether the compose context menu option is available.
+  // TODO(b/301371110): Gate on the field type here too when it is ready.
+  virtual bool ShouldOfferComposeContextMenu() = 0;
   // Opens compose from the context menu given the 'frame_token',
   // 'field_renderer_id', and 'bounds'.
   virtual void OpenComposeFromContextMenu(