Skip to content

Commit

Permalink
Update ShouldTriggerContextMenu with params from the Context Menu.
Browse files Browse the repository at this point in the history
Move the context menu check from ComposeManager to ChromeComposeClient, as we eventually need RenderFrameHost which cannot be passed into components/.

Bug: b:305798934
Change-Id: Iac9a027c296de227bb725d9ce6807acf8a43b270
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4973816
Commit-Queue: Sophey Dong <sophey@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217271}
  • Loading branch information
Sophey authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent a6b0cb9 commit 533025a
Show file tree
Hide file tree
Showing 20 changed files with 177 additions and 154 deletions.
10 changes: 7 additions & 3 deletions chrome/browser/compose/chrome_compose_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include "components/optimization_guide/proto/features/compose.pb.h"
#include "components/strings/grit/components_strings.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/context_menu_params.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents_user_data.h"
#include "mojo/public/cpp/bindings/callback_helpers.h"
#include "ui/base/l10n/l10n_util.h"
Expand Down Expand Up @@ -185,11 +187,13 @@ bool ChromeComposeClient::ShouldTriggerPopup(std::string autocomplete_attribute,
translate_manager, saved_state);
}

bool ChromeComposeClient::ShouldTriggerContextMenu() {
bool ChromeComposeClient::ShouldTriggerContextMenu(
content::RenderFrameHost* rfh,
content::ContextMenuParams& params) {
translate::TranslateManager* translate_manager =
ChromeTranslateClient::GetManagerFromWebContents(&GetWebContents());
return compose_enabling_.ShouldTriggerContextMenu(profile_,
translate_manager);
return compose_enabling_.ShouldTriggerContextMenu(profile_, translate_manager,
rfh, params);
}

optimization_guide::OptimizationGuideModelExecutor*
Expand Down
7 changes: 5 additions & 2 deletions chrome/browser/compose/chrome_compose_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include "components/compose/core/browser/compose_manager_impl.h"
#include "components/optimization_guide/core/optimization_guide_decision.h"
#include "components/optimization_guide/core/optimization_guide_model_executor.h"
#include "content/public/browser/context_menu_params.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents_user_data.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
Expand Down Expand Up @@ -59,7 +61,8 @@ class ChromeComposeClient

bool ShouldTriggerPopup(std::string autocomplete_attribute,
autofill::FieldGlobalId field_id) override;
bool ShouldTriggerContextMenu() override;
virtual bool ShouldTriggerContextMenu(content::RenderFrameHost* rfh,
content::ContextMenuParams& params);

void BindComposeDialog(
mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
Expand All @@ -81,14 +84,14 @@ class ChromeComposeClient
ComposeEnabling& GetComposeEnabling();

protected:
explicit ChromeComposeClient(content::WebContents* web_contents);
optimization_guide::OptimizationGuideModelExecutor* GetModelExecutor();
optimization_guide::OptimizationGuideDecider* GetOptimizationGuide();
std::unique_ptr<TranslateLanguageProvider> translate_language_provider_;
ComposeEnabling compose_enabling_;

private:
friend class content::WebContentsUserData<ChromeComposeClient>;
explicit ChromeComposeClient(content::WebContents* web_contents);
raw_ptr<Profile> profile_;

// Creates a session for `trigger_field` and initializes it as necessary.
Expand Down
14 changes: 13 additions & 1 deletion chrome/browser/compose/compose_enabling.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "components/compose/core/browser/compose_features.h"
#include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
#include "compose_enabling.h"
#include "content/public/browser/context_menu_params.h"
#include "content/public/browser/render_frame_host.h"

namespace {

Expand Down Expand Up @@ -114,12 +116,22 @@ bool ComposeEnabling::ShouldTriggerPopup(
return false;
}

// TODO(b/301609046): Add ContentEditable and TextArea checks.

return true;
}

bool ComposeEnabling::ShouldTriggerContextMenu(
Profile* profile,
translate::TranslateManager* translate_manager) {
translate::TranslateManager* translate_manager,
content::RenderFrameHost* rfh,
content::ContextMenuParams& params) {
if (!(params.is_content_editable_for_autofill ||
(params.form_control_type &&
*params.form_control_type ==
blink::mojom::FormControlType::kTextArea))) {
return false;
}
return PageLevelChecks(profile, translate_manager);
}

Expand Down
6 changes: 5 additions & 1 deletion chrome/browser/compose/compose_enabling.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "chrome/browser/profiles/profile_manager.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/translate/core/browser/translate_manager.h"
#include "content/public/browser/context_menu_params.h"
#include "content/public/browser/render_frame_host.h"

class ComposeEnabling {
public:
Expand All @@ -25,7 +27,9 @@ class ComposeEnabling {
translate::TranslateManager* translate_manager,
bool has_saved_state);
bool ShouldTriggerContextMenu(Profile* profile,
translate::TranslateManager* translate_manager);
translate::TranslateManager* translate_manager,
content::RenderFrameHost* rfh,
content::ContextMenuParams& params);

private:
raw_ptr<TranslateLanguageProvider> translate_language_provider_;
Expand Down
70 changes: 63 additions & 7 deletions chrome/browser/compose/compose_enabling_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class ComposeEnablingTest : public testing::Test {
{compose::features::kEnableCompose,
compose::features::kEnableComposeNudge},
{});
context_menu_params_.is_content_editable_for_autofill = true;
}

void TearDown() override {
Expand Down Expand Up @@ -93,6 +94,7 @@ class ComposeEnablingTest : public testing::Test {
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
signin::IdentityTestEnvironment identity_test_env_;
std::unique_ptr<ComposeEnabling> compose_enabling_;
content::ContextMenuParams context_menu_params_;

private:
std::unique_ptr<TestingProfileManager> profile_manager_;
Expand Down Expand Up @@ -194,7 +196,8 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuDisabledTest) {
.WillByDefault(Return(std::string("en")));

EXPECT_FALSE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuLanguageTest) {
Expand All @@ -213,7 +216,8 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuLanguageTest) {
.WillOnce(Return(std::string("eo")));

EXPECT_FALSE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuLanguageBypassTest) {
Expand All @@ -239,7 +243,8 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuLanguageBypassTest) {
// Although the language is unsupported, ShouldTrigger should return true as
// the bypass is enabled.
EXPECT_TRUE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuEmptyLangugeTest) {
Expand All @@ -259,7 +264,8 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuEmptyLangugeTest) {
.WillOnce(Return(std::string()));

EXPECT_TRUE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuUndeterminedLangugeTest) {
Expand All @@ -279,10 +285,34 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuUndeterminedLangugeTest) {
.WillOnce(Return(std::string("und")));

EXPECT_TRUE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuAllEnabledTest) {
TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuFieldTypeTest) {
testing::NiceMock<MockTranslateLanguageProvider>
mock_translate_language_provider;
ComposeEnabling compose_enabling(&mock_translate_language_provider);
// Set ContextMenuParams to non-contenteditable and non-textarea, which we do
// not support.
context_menu_params_.is_content_editable_for_autofill = false;
context_menu_params_.form_control_type =
blink::mojom::FormControlType::kInputButton;

// Set the language to something we support. Not expected to be called.
MockTranslateClient mock_translate_client(translate_driver(), nullptr);
testing::NiceMock<MockTranslateManager> mock_translate_manager(
&mock_translate_client);
ON_CALL(mock_translate_language_provider, GetSourceLanguage(_))
.WillByDefault(Return(std::string("en")));

EXPECT_FALSE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest,
ShouldTriggerContextMenuAllEnabledContentEditableTest) {
testing::NiceMock<MockTranslateLanguageProvider>
mock_translate_language_provider;
ComposeEnabling compose_enabling(&mock_translate_language_provider);
Expand All @@ -298,7 +328,33 @@ TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuAllEnabledTest) {
.WillOnce(Return(std::string("en")));

EXPECT_TRUE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager));
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerContextMenuAllEnabledTextAreaTest) {
testing::NiceMock<MockTranslateLanguageProvider>
mock_translate_language_provider;
ComposeEnabling compose_enabling(&mock_translate_language_provider);
// Enable everything.
compose_enabling.SetEnabledForTesting();

// Set the language to something we support.
MockTranslateClient mock_translate_client(translate_driver(), nullptr);
testing::NiceMock<MockTranslateManager> mock_translate_manager(
&mock_translate_client);
EXPECT_CALL(mock_translate_language_provider,
GetSourceLanguage(&mock_translate_manager))
.WillOnce(Return(std::string("en")));

// Set ContextMenuParams to textarea, which we support.
context_menu_params_.is_content_editable_for_autofill = false;
context_menu_params_.form_control_type =
blink::mojom::FormControlType::kTextArea;

EXPECT_TRUE(compose_enabling.ShouldTriggerContextMenu(
test_profile_, &mock_translate_manager, /*rfh=*/nullptr,
context_menu_params_));
}

TEST_F(ComposeEnablingTest, ShouldTriggerPopupDisabledTest) {
Expand Down
11 changes: 11 additions & 0 deletions chrome/browser/compose/mock_chrome_compose_client.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// 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/compose/mock_chrome_compose_client.h"

MockChromeComposeClient::MockChromeComposeClient(
content::WebContents* web_contents)
: ChromeComposeClient(web_contents) {}

MockChromeComposeClient::~MockChromeComposeClient() = default;
24 changes: 24 additions & 0 deletions chrome/browser/compose/mock_chrome_compose_client.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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_COMPOSE_MOCK_CHROME_COMPOSE_CLIENT_H_
#define CHROME_BROWSER_COMPOSE_MOCK_CHROME_COMPOSE_CLIENT_H_

#include "chrome/browser/compose/chrome_compose_client.h"
#include "components/autofill/core/browser/autofill_client.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "url/gurl.h"

class MockChromeComposeClient : public ChromeComposeClient {
public:
explicit MockChromeComposeClient(content::WebContents* web_contents);
~MockChromeComposeClient() override;

MOCK_METHOD(bool,
ShouldTriggerContextMenu,
(content::RenderFrameHost*, content::ContextMenuParams&),
(override));
};

#endif // CHROME_BROWSER_COMPOSE_MOCK_CHROME_COMPOSE_CLIENT_H_
26 changes: 15 additions & 11 deletions chrome/browser/renderer_context_menu/render_view_context_menu.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1525,9 +1525,8 @@ std::u16string RenderViewContextMenu::GetTargetLanguageDisplayName(
}

#if BUILDFLAG(ENABLE_COMPOSE)
compose::ComposeManager* RenderViewContextMenu::GetComposeManager() const {
auto* client = ChromeComposeClient::FromWebContents(source_web_contents_);
return client ? &client->GetManager() : nullptr;
ChromeComposeClient* RenderViewContextMenu::GetChromeComposeClient() const {
return ChromeComposeClient::FromWebContents(source_web_contents_);
}
#endif // BUILDFLAG(ENABLE_COMPOSE)

Expand Down Expand Up @@ -2261,9 +2260,10 @@ void RenderViewContextMenu::AppendSpellingAndSearchSuggestionItems() {
}
#if BUILDFLAG(ENABLE_COMPOSE)
RenderFrameHost* render_frame_host = GetRenderFrameHost();
if (render_frame_host && params_.form_control_type) {
compose::ComposeManager* compose_manager = GetComposeManager();
if (compose_manager && compose_manager->ShouldOfferComposeContextMenu()) {
if (render_frame_host) {
auto* compose_client = GetChromeComposeClient();
if (compose_client &&
compose_client->ShouldTriggerContextMenu(render_frame_host, params_)) {
menu_model_.AddItemWithStringId(IDC_CONTEXT_COMPOSE,
IDS_COMPOSE_SUGGESTION_MAIN_TEXT);
// TODO(b/303646344): Remove new feature tag when no longer new.
Expand Down Expand Up @@ -3283,16 +3283,20 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {

#if BUILDFLAG(ENABLE_COMPOSE)
case IDC_CONTEXT_COMPOSE: {
auto* client = ChromeComposeClient::FromWebContents(source_web_contents_);
auto* client = GetChromeComposeClient();
compose::ComposeManager* compose_manager =
client ? &client->GetManager() : nullptr;
RenderFrameHost* render_frame_host = GetRenderFrameHost();
if (compose_manager && render_frame_host) {
compose_manager->OpenComposeFromContextMenu(
auto* content_autofill_driver =
autofill::ContentAutofillDriver::GetForRenderFrameHost(
render_frame_host),
autofill::FormRendererId(params_.form_renderer_id),
autofill::FieldRendererId(params_.field_renderer_id));
render_frame_host);
if (content_autofill_driver) {
compose_manager->OpenComposeFromContextMenu(
content_autofill_driver,
autofill::FormRendererId(params_.form_renderer_id),
autofill::FieldRendererId(params_.field_renderer_id));
}
}
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
#include "ui/gfx/geometry/vector2d.h"

#if BUILDFLAG(ENABLE_COMPOSE)
#include "components/compose/core/browser/compose_manager.h"
#include "chrome/browser/compose/chrome_compose_client.h"
#endif

#if BUILDFLAG(ENABLE_LENS_DESKTOP_GOOGLE_BRANDED_FEATURES)
Expand Down Expand Up @@ -202,7 +202,7 @@ class RenderViewContextMenu
#endif

#if BUILDFLAG(ENABLE_COMPOSE)
virtual compose::ComposeManager* GetComposeManager() const;
virtual ChromeComposeClient* GetChromeComposeClient() const;
#endif

// RenderViewContextMenuBase:
Expand Down

0 comments on commit 533025a

Please sign in to comment.