Skip to content

Commit

Permalink
Revert "Reland "Add region search support for non-Google default sear…
Browse files Browse the repository at this point in the history 
…ch providers."."

This reverts commit 23f829c.

Reason for revert: Breaks TranslateManagerRenderViewHostTest.BubbleUnknownLanguage on linux-chromeos-chrome

[ RUN      ] TranslateManagerRenderViewHostTest.BubbleUnknownLanguage
Received signal 11 SEGV_MAPERR 000000000108
#0 0x56400916c589 base::debug::CollectStackTrace()
#1 0x5640090da7c3 base::debug::StackTrace::StackTrace()
#2 0x56400916c0df base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7fc34d615980 (/lib/x86_64-linux-gnu/libpthread-2.27.so+0x1297f)
#4 0x56400acc1794 TemplateURLService::GetDefaultSearchProvider()
#5 0x564009765d6a RenderViewContextMenu::IsRegionSearchEnabled()
#6 0x564009762d17 RenderViewContextMenu::AppendPageItems()
#7 0x564009762838 RenderViewContextMenu::InitMenu()
#8 0x56400886430c RenderViewContextMenuBase::Init()
#9 0x5640034ae263 translate::(anonymous namespace)::TranslateManagerRenderViewHostTest_BubbleUnknownLanguage_Test::TestBody()
...

Original change's description:
> Reland "Add region search support for non-Google default search providers.".
>
> This an attempted reland of the following reverted change:
> https://chromium-review.googlesource.com/c/chromium/src/+/3199698
>
> PS1 is identical to the original change with a few minor modifications to resolve merge conflicts.
>
> The changes from the original is the adding of an instance variable to LensRegionSearchController representing whether the default search engine is Google. This could not be used in the controller directly due to an include rule. This is now instead passed as part of the Start() function through render_view_context_menu.
>
>
> Bug: 1255702,1255897, b/198833501
> Change-Id: I83c4b0a5728ab3ba122d248c05c2f27b76a05a4b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203970
> Commit-Queue: Juan Mojica <juanmojica@google.com>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Ben Goldberger <benwgold@google.com>
> Cr-Commit-Position: refs/heads/main@{#928974}

Bug: 1255702,1255897, b/198833501
Change-Id: I1bf2e732ac781a5708f4a319ecf5df960239a860
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3210916
Auto-Submit: Timothy Loh <timloh@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Owners-Override: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#929066}
  • Loading branch information
@tim-loh
tim-loh authored and Chromium LUCI CQ committed Oct 7, 2021
1 parent 0e7a62c commit c3e61fd
Show file tree
Hide file tree
Showing 9 changed files with 50 additions and 158 deletions.
3 changes: 1 addition & 2 deletions chrome/app/chrome_command_ids.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,12 +329,11 @@
#define IDC_CONTENT_CONTEXT_NO_SPELLING_SUGGESTIONS 50155
#define IDC_CONTENT_CONTEXT_SPELLING_SUGGESTION 50156
#define IDC_CONTENT_CONTEXT_SPELLING_TOGGLE 50157
// A gap here. Feel free to insert new IDs.
#define IDC_CONTENT_CONTEXT_INSPECTBACKGROUNDPAGE 50161
#define IDC_CONTENT_CONTEXT_RELOAD_PACKAGED_APP 50162
#define IDC_CONTENT_CONTEXT_RESTART_PACKAGED_APP 50163
#define IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH 50164
#define IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH 50165
// A gap here. Feel free to insert new IDs.
#define IDC_CONTENT_CONTEXT_GENERATEPASSWORD 50166
#define IDC_CONTENT_CONTEXT_EXIT_FULLSCREEN 50167
#define IDC_CONTENT_CONTEXT_SHOWALLSAVEDPASSWORDS 50168
Expand Down
18 changes: 5 additions & 13 deletions chrome/browser/lens/region_search/lens_region_search_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ LensRegionSearchController::~LensRegionSearchController() {
CloseWithReason(views::Widget::ClosedReason::kLostFocus);
}

void LensRegionSearchController::Start(bool use_fullscreen_capture,
bool is_google_default_search_provider) {
is_google_default_search_provider_ = is_google_default_search_provider;
void LensRegionSearchController::Start(bool use_fullscreen_capture) {
if (!web_contents() || !browser_)
return;

Expand Down Expand Up @@ -172,16 +170,10 @@ void LensRegionSearchController::OnCaptureCompleted(
lens::LensRegionSearchCaptureResult::FAILED_TO_OPEN_TAB);
return;
}

if (is_google_default_search_provider_) {
core_tab_helper->SearchWithLensInNewTab(
image, captured_image.Size(),
lens::EntryPoint::CHROME_REGION_SEARCH_MENU_ITEM,
lens::features::kEnableSidePanelForLensRegionSearch.Get());
} else {
core_tab_helper->SearchByImageInNewTab(image, captured_image.Size());
}

core_tab_helper->SearchWithLensInNewTab(
image, captured_image.Size(),
lens::EntryPoint::CHROME_REGION_SEARCH_MENU_ITEM,
lens::features::kEnableSidePanelForLensRegionSearch.Get());
RecordCaptureResult(lens::LensRegionSearchCaptureResult::SUCCESS);
}

Expand Down
8 changes: 1 addition & 7 deletions chrome/browser/lens/region_search/lens_region_search_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ class LensRegionSearchController : public content::WebContentsObserver {
// around the web contents. When finished with selection, the region is
// converted into a PNG and sent to Lens. If `use_fullscreen_capture` is set
// to true, the whole screen will automatically be captured.
void Start(bool use_fullscreen_capture,
bool is_google_default_search_provider);
void Start(bool use_fullscreen_capture);

// Closes the UI overlay and user education bubble if currently being shown.
// The closed reason for this method is defaulted to the close button being
Expand Down Expand Up @@ -74,11 +73,6 @@ class LensRegionSearchController : public content::WebContentsObserver {

gfx::Image ResizeImageIfNecessary(const gfx::Image& image);

// Variable for tracking the default search provider as to launch the image
// results in correct search engine. This value is set every time the capture
// mode is started to have an accurate value for the completed capture.
bool is_google_default_search_provider_ = false;

std::unique_ptr<image_editor::ScreenshotFlow> screenshot_flow_;

Browser* browser_ = nullptr;
Expand Down
67 changes: 16 additions & 51 deletions chrome/browser/renderer_context_menu/render_view_context_menu.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,14 +401,13 @@ const std::map<int, int>& GetIdcToUmaMap(UmaEnumIdLookupType type) {
{IDC_CONTENT_CONTEXT_SEARCHLENSFORIMAGE, 113},
{IDC_CONTENT_CONTEXT_REMOVELINKTOTEXT, 114},
{IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH, 115},
{IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH, 116},
// To add new items:
// - Add one more line above this comment block, using the UMA value
// from the line below this comment block.
// - Increment the UMA value in that latter line.
// - Add the new item to the RenderViewContextMenuItem enum in
// tools/metrics/histograms/enums.xml.
{0, 117}});
{0, 116}});

// These UMA values are for the the ContextMenuOptionDesktop enum, used for
// the ContextMenu.SelectedOptionDesktop histograms.
Expand Down Expand Up @@ -436,14 +435,13 @@ const std::map<int, int>& GetIdcToUmaMap(UmaEnumIdLookupType type) {
{IDC_CONTENT_CONTEXT_COPYLINKTOTEXT, 20},
{IDC_CONTENT_CONTEXT_SEARCHLENSFORIMAGE, 21},
{IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH, 22},
{IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH, 23},
// To add new items:
// - Add one more line above this comment block, using the UMA value
// from the line below this comment block.
// - Increment the UMA value in that latter line.
// - Add the new item to the ContextMenuOptionDesktop enum in
// tools/metrics/histograms/enums.xml.
{0, 24}});
{0, 23}});

return *(type == UmaEnumIdLookupType::GeneralEnumId ? kGeneralMap
: kSpecificMap);
Expand Down Expand Up @@ -1662,8 +1660,8 @@ void RenderViewContextMenu::AppendPageItems() {
menu_model_.AddItemWithStringId(IDC_PRINT, IDS_CONTENT_CONTEXT_PRINT);
AppendMediaRouterItem();
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
if (IsRegionSearchEnabled()) {
AppendRegionSearchItem();
if (IsLensRegionSearchEnabled()) {
AppendLensRegionSearchItem();
}
#endif

Expand Down Expand Up @@ -2122,7 +2120,7 @@ void RenderViewContextMenu::AppendSharedClipboardItem() {
shared_clipboard_context_menu_observer_->InitMenu(params_);
}

void RenderViewContextMenu::AppendRegionSearchItem() {
void RenderViewContextMenu::AppendLensRegionSearchItem() {
int resource_id = IDS_CONTENT_CONTEXT_LENS_REGION_SEARCH;
if (lens::features::kRegionSearchUseMenuItemAltText1.Get()) {
resource_id = IDS_CONTENT_CONTEXT_LENS_REGION_SEARCH_ALT1;
Expand All @@ -2134,28 +2132,11 @@ void RenderViewContextMenu::AppendRegionSearchItem() {
resource_id = IDS_CONTENT_CONTEXT_LENS_REGION_SEARCH_ALT4;
}

if (search::DefaultSearchProviderIsGoogle(GetProfile())) {
menu_model_.AddItem(
IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH,
l10n_util::GetStringFUTF16(resource_id, std::u16string(kGoogleLens)));
} else {
TemplateURLService* service =
TemplateURLServiceFactory::GetForProfile(GetProfile());
const TemplateURL* provider = service->GetDefaultSearchProvider();
// GetDefaultSearchProvider can return null in unit tests or when the
// default search provider is disabled by policy. In these cases, we still
// add the menu item but disable it. This is to prevent any user confusion
// regarding this menu item being in the proper place.
if (provider) {
menu_model_.AddItem(
IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH,
l10n_util::GetStringFUTF16(resource_id, provider->short_name()));
} else {
menu_model_.AddItem(
IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH,
l10n_util::GetStringFUTF16(resource_id, std::u16string(kGoogleLens)));
}
}
// TODO(crbug.com/1234592): When support is added for non-Google default
// search providers, set the name here using the provider |short_name|.
menu_model_.AddItem(
IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH,
l10n_util::GetStringFUTF16(resource_id, std::u16string(kGoogleLens)));
}

// Menu delegate functions -----------------------------------------------------
Expand Down Expand Up @@ -2355,12 +2336,8 @@ bool RenderViewContextMenu::IsCommandIdEnabled(int id) const {
case IDC_CONTENT_CONTEXT_LANGUAGE_SETTINGS:
case IDC_SEND_TAB_TO_SELF:
case IDC_SEND_TAB_TO_SELF_SINGLE_TARGET:
return true;

case IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH:
case IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH:
// Disable region search menu items if default search provider is null.
return IsRegionSearchEnabled();
return true;

case IDC_CONTENT_CONTEXT_GENERATE_QR_CODE:
return IsQRCodeGeneratorEnabled();
Expand Down Expand Up @@ -2551,10 +2528,7 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
break;

case IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH:
ExecRegionSearch(event_flags, true);
break;
case IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH:
ExecRegionSearch(event_flags, false);
ExecLensRegionSearch(event_flags);
break;

case IDC_CONTENT_CONTEXT_OPEN_ORIGINAL_IMAGE_NEW_TAB:
Expand Down Expand Up @@ -3084,20 +3058,14 @@ bool RenderViewContextMenu::IsQRCodeGeneratorEnabled() const {
IsGeneratorAvailable(entry->GetURL());
}

bool RenderViewContextMenu::IsRegionSearchEnabled() const {
bool RenderViewContextMenu::IsLensRegionSearchEnabled() const {
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
const Browser* browser = GetBrowser();
const bool in_app =
browser && (browser->is_type_app() || browser->is_type_app_popup());
TemplateURLService* service =
TemplateURLServiceFactory::GetForProfile(GetProfile());
const TemplateURL* provider = service->GetDefaultSearchProvider();
const bool provider_supports_image_search =
provider && !provider->image_url().empty() &&
provider->image_url_ref().IsValid(service->search_terms_data());
return base::FeatureList::IsEnabled(lens::features::kLensRegionSearch) &&
search::DefaultSearchProviderIsGoogle(GetProfile()) &&
!IsPdfExtensionOrigin(url::Origin::Create(GetDocumentURL(params_))) &&
provider_supports_image_search &&
!GetDocumentURL(params_).SchemeIs(content::kChromeUIScheme) &&
!in_app &&
GetPrefs(browser_context_)
Expand Down Expand Up @@ -3348,18 +3316,15 @@ void RenderViewContextMenu::ExecSearchLensForImage() {
lens::features::kEnableSidePanelForLensImageSearch.Get());
}

void RenderViewContextMenu::ExecRegionSearch(
int event_flags,
bool is_google_default_search_provider) {
void RenderViewContextMenu::ExecLensRegionSearch(int event_flags) {
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
if (!lens_region_search_controller_)
lens_region_search_controller_ =
std::make_unique<lens::LensRegionSearchController>(source_web_contents_,
GetBrowser());
bool use_fullscreen_capture =
GetMenuSourceType(event_flags) == ui::MENU_SOURCE_KEYBOARD;
lens_region_search_controller_->Start(use_fullscreen_capture,
is_google_default_search_provider);
lens_region_search_controller_->Start(use_fullscreen_capture);
#endif
}

Expand Down
7 changes: 3 additions & 4 deletions chrome/browser/renderer_context_menu/render_view_context_menu.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ class RenderViewContextMenu : public RenderViewContextMenuBase,
void AppendClickToCallItem();
#endif
void AppendSharedClipboardItem();
void AppendRegionSearchItem();
void AppendLensRegionSearchItem();
void AppendQRCodeGeneratorItem(bool for_image, bool draw_icon);

std::unique_ptr<ui::DataTransferEndpoint> CreateDataEndpoint(
Expand All @@ -246,7 +246,7 @@ class RenderViewContextMenu : public RenderViewContextMenuBase,
bool IsRouteMediaEnabled() const;
bool IsOpenLinkOTREnabled() const;
bool IsSearchWebForEnabled() const;
bool IsRegionSearchEnabled() const;
bool IsLensRegionSearchEnabled() const;

// Command execution functions.
void ExecOpenWebApp();
Expand All @@ -260,8 +260,7 @@ class RenderViewContextMenu : public RenderViewContextMenuBase,
void ExecCopyLinkText();
void ExecCopyImageAt();
void ExecSearchLensForImage();
void ExecRegionSearch(int event_flags,
bool is_google_default_search_provider);
void ExecLensRegionSearch(int event_flags);
void ExecSearchWebForImage();
void ExecLoadImage();
void ExecPlayPause();
Expand Down
49 changes: 10 additions & 39 deletions chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,14 +477,10 @@ class RenderViewContextMenuPrefsTest : public ChromeRenderViewHostTestHarness {
menu->AppendImageItems();
}

void SetUserSelectedDefaultSearchProvider(const std::string& base_url,
bool supports_image_search) {
void SetUserSelectedDefaultSearchProvider(const std::string& base_url) {
TemplateURLData data;
data.SetShortName(u"t");
data.SetURL(base_url + "?q={searchTerms}");
if (supports_image_search) {
data.image_url = base_url;
}
TemplateURL* template_url =
template_url_service_->Add(std::make_unique<TemplateURL>(data));
template_url_service_->SetUserSelectedDefaultSearchProvider(template_url);
Expand Down Expand Up @@ -727,8 +723,7 @@ TEST_F(RenderViewContextMenuPrefsTest, ShowAllPasswordsIncognito) {
TEST_F(RenderViewContextMenuPrefsTest, LensRegionSearch) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.google.com",
/*supports_image_search=*/true);
SetUserSelectedDefaultSearchProvider("https://www.google.com");
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
auto menu = std::make_unique<TestRenderViewContextMenu>(
web_contents()->GetMainFrame(), params);
Expand All @@ -743,8 +738,7 @@ TEST_F(RenderViewContextMenuPrefsTest,
LensRegionSearchEnterprisePoicyDisabled) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.google.com",
/*supports_image_search=*/true);
SetUserSelectedDefaultSearchProvider("https://www.google.com");
// Set enterprise policy to false.
profile()->GetPrefs()->SetBoolean(prefs::kLensRegionSearchEnabled, false);
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
Expand All @@ -760,49 +754,28 @@ TEST_F(RenderViewContextMenuPrefsTest,
TEST_F(RenderViewContextMenuPrefsTest, LensRegionSearchDisabledOnImage) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.google.com",
/*supports_image_search=*/true);
SetUserSelectedDefaultSearchProvider("https://www.google.com");
content::ContextMenuParams params = CreateParams(MenuItem::IMAGE);
params.has_image_contents = true;
auto menu = std::make_unique<TestRenderViewContextMenu>(
web_contents()->GetMainFrame(), params);
AppendImageItems(menu.get());

EXPECT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH));
EXPECT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH));
}

// Verify that the web region search menu item is enabled for a non-Google
// search engine that supports visual search.
TEST_F(RenderViewContextMenuPrefsTest,
LensRegionSearchNonGoogleDefaultSearchEngineSupportsImageSearch) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.search.com",
/*supports_image_search=*/true);
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
auto menu = std::make_unique<TestRenderViewContextMenu>(
web_contents()->GetMainFrame(), params);
menu->Init();

EXPECT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH));
EXPECT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH));
}

// Verify that region search menu items are disabled for a search engine that
// does not support visual search.
// Verify that the Lens Region Search menu item is disabled when the user's
// default browser is not Google.
TEST_F(RenderViewContextMenuPrefsTest,
LensRegionSearchDefaultSearchEngineDoesNotSupportImageSearch) {
LensRegionSearchNonGoogleDefaultSearchEngine) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.search.com",
/*supports_image_search=*/false);
SetUserSelectedDefaultSearchProvider("https://www.search.com");
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
auto menu = std::make_unique<TestRenderViewContextMenu>(
web_contents()->GetMainFrame(), params);
menu->Init();

EXPECT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH));
EXPECT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH));
}

Expand All @@ -811,8 +784,7 @@ TEST_F(RenderViewContextMenuPrefsTest,
TEST_F(RenderViewContextMenuPrefsTest, LensRegionSearchExperimentDisabled) {
base::test::ScopedFeatureList features;
features.InitAndDisableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.google.com",
/*supports_image_search=*/true);
SetUserSelectedDefaultSearchProvider("https://www.google.com");
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
auto menu = std::make_unique<TestRenderViewContextMenu>(
web_contents()->GetMainFrame(), params);
Expand All @@ -826,8 +798,7 @@ TEST_F(RenderViewContextMenuPrefsTest, LensRegionSearchExperimentDisabled) {
TEST_F(RenderViewContextMenuPrefsTest, LensRegionSearchChromeUIScheme) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(lens::features::kLensRegionSearch);
SetUserSelectedDefaultSearchProvider("https://www.google.com",
/*supports_image_search=*/true);
SetUserSelectedDefaultSearchProvider("https://www.google.com");
content::ContextMenuParams params = CreateParams(MenuItem::PAGE);
params.page_url = GURL(chrome::kChromeUISettingsURL);
auto menu = std::make_unique<TestRenderViewContextMenu>(
Expand Down
Loading

0 comments on commit c3e61fd

Please sign in to comment.