Skip to content

Commit

Permalink
Implement keyboard shortcut for Lens Region Search.
Browse files Browse the repository at this point in the history
Keyboard shortcut (Ctrl/Cmd + Shift + L) is temporary as the keyboard
shortcut has not yet been finalized. All code is flag-guarded so
keyboard behavior is unaffected when flag is disabled.

Tested on Linux, Mac, and Windows; with flag disabled and enabled; and
with search engine set to Google, Bing (third-party search engine that
supports region search), and Yahoo (does not support region search).
Availability of keyboard shortcut is identical to that of region search
when flag is enabled.

Screenshot of context menu item with keyboard shortcut label:
https://screenshot.googleplex.com/3MSyZ2kn9iGHDef

Bug: 1392456
Change-Id: Ifa4fb6297879d9ae4de3c0d9dc970c4878e52c04
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4043546
Reviewed-by: Juan Mojica <juanmojica@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Bryan Nguyen <nguyenbryan@google.com>
Cr-Commit-Position: refs/heads/main@{#1076590}
  • Loading branch information
Bryan Nguyen authored and Chromium LUCI CQ committed Nov 29, 2022
1 parent 6544fc8 commit 422b5c3
Show file tree
Hide file tree
Showing 14 changed files with 192 additions and 2 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/lens/region_search/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ static_library("region_search") {
"../metrics/lens_metrics.h",
"lens_region_search_controller.cc",
"lens_region_search_controller.h",
"lens_region_search_helper.cc",
"lens_region_search_helper.h",
]

# TODO(crbug/1383280): Remove allow_circular_includes_from when dependencies for controller are fixed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@

namespace lens {

LensRegionSearchControllerData::LensRegionSearchControllerData() = default;
LensRegionSearchControllerData::~LensRegionSearchControllerData() = default;

RegionSearchCapturedData::RegionSearchCapturedData() = default;
RegionSearchCapturedData::~RegionSearchCapturedData() = default;

Expand Down
19 changes: 17 additions & 2 deletions chrome/browser/lens/region_search/lens_region_search_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,23 @@ class LensRegionSearchController : public content::WebContentsObserver {
base::WeakPtrFactory<LensRegionSearchController> weak_factory_{this};
};

// Class to associate region search data with Profile across navigation. Used to
// support region search on a static WebUI page.
// Class to associate region search controller data with Profile across
// navigation. Used to support region search via keyboard shortcut.
class LensRegionSearchControllerData : public base::SupportsUserData::Data {
public:
LensRegionSearchControllerData();
~LensRegionSearchControllerData() override;
LensRegionSearchControllerData(const LensRegionSearchControllerData&) =
delete;
LensRegionSearchControllerData& operator=(
const LensRegionSearchControllerData&) = delete;

static constexpr char kDataKey[] = "lens_region_search_controller_data";
std::unique_ptr<LensRegionSearchController> lens_region_search_controller;
};

// Class to associate region search captured data with Profile across
// navigation. Used to support region search on a static WebUI page.
class RegionSearchCapturedData : public base::SupportsUserData::Data {
public:
RegionSearchCapturedData();
Expand Down
59 changes: 59 additions & 0 deletions chrome/browser/lens/region_search/lens_region_search_helper.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Copyright 2022 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/lens/region_search/lens_region_search_helper.h"

#include "base/feature_list.h"
#include "build/branding_buildflags.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/common/pref_names.h"
#include "components/lens/lens_features.h"
#include "components/prefs/pref_service.h"
#include "components/search_engines/template_url.h"
#include "components/search_engines/template_url_service.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/url_constants.h"
#include "url/gurl.h"

namespace lens {

bool IsRegionSearchEnabled(Browser* browser,
Profile* profile,
const GURL& url) {
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
if (!browser)
return false;

TemplateURLService* service =
TemplateURLServiceFactory::GetForProfile(profile);
if (!service)
return false;

// Region selection is broken in PWAs on Mac b/250074889
#if BUILDFLAG(IS_MAC)
if (IsInProgressiveWebApp(browser))
return false;
#endif // BUILDFLAG(IS_MAC)

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::kLensStandalone) &&
provider_supports_image_search &&
!url.SchemeIs(content::kChromeUIScheme) &&
profile->GetPrefs()->GetBoolean(prefs::kLensRegionSearchEnabled);
#else
return false;
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)
}

bool IsInProgressiveWebApp(Browser* browser) {
return browser && (browser->is_type_app() || browser->is_type_app_popup());
}

} // namespace lens
20 changes: 20 additions & 0 deletions chrome/browser/lens/region_search/lens_region_search_helper.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2022 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_LENS_REGION_SEARCH_LENS_REGION_SEARCH_HELPER_H_
#define CHROME_BROWSER_LENS_REGION_SEARCH_LENS_REGION_SEARCH_HELPER_H_

class Browser;
class Profile;
class GURL;

namespace lens {

bool IsRegionSearchEnabled(Browser* browser, Profile* profile, const GURL& url);

bool IsInProgressiveWebApp(Browser* browser);

} // namespace lens

#endif // CHROME_BROWSER_LENS_REGION_SEARCH_LENS_REGION_SEARCH_HELPER_H_
Original file line number Diff line number Diff line change
Expand Up @@ -3273,6 +3273,8 @@ bool RenderViewContextMenu::IsQRCodeGeneratorEnabled() const {

bool RenderViewContextMenu::IsRegionSearchEnabled() const {
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
// TODO(nguyenbryan): Refactor to use lens_region_search_helper.cc after PDF
// support is cleaned up.
if (!GetBrowser())
return false;

Expand Down
15 changes: 15 additions & 0 deletions chrome/browser/ui/browser_command_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#include "chrome/common/webui_url_constants.h"
#include "components/bookmarks/common/bookmark_pref_names.h"
#include "components/dom_distiller/core/dom_distiller_features.h"
#include "components/lens/lens_features.h"
#include "components/prefs/pref_service.h"
#include "components/services/screen_ai/buildflags/buildflags.h"
#include "components/sessions/core/tab_restore_service.h"
Expand Down Expand Up @@ -943,6 +944,12 @@ bool BrowserCommandController::ExecuteCommandWithDisposition(
break;
#endif

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
case IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH:
ExecLensRegionSearch(browser_);
break;
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)

default:
LOG(WARNING) << "Received Unimplemented Command: " << id;
break;
Expand Down Expand Up @@ -1238,6 +1245,14 @@ void BrowserCommandController::InitCommandState() {
true);
}

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
if (base::FeatureList::IsEnabled(
lens::features::kEnableRegionSearchKeyboardShortcut)) {
command_updater_.UpdateCommandEnabled(
IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH, true);
}
#endif

// Initialize other commands whose state changes based on various conditions.
UpdateCommandsForFullscreenMode();
UpdateCommandsForContentRestrictionState();
Expand Down
27 changes: 27 additions & 0 deletions chrome/browser/ui/browser_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "chrome/browser/media/router/media_router_feature.h"
#include "chrome/browser/prefs/incognito_mode_prefs.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search/search.h"
#include "chrome/browser/sessions/session_service.h"
#include "chrome/browser/sessions/session_service_base.h"
#include "chrome/browser/sessions/session_service_factory.h"
Expand Down Expand Up @@ -182,6 +183,12 @@
#include "chrome/browser/apps/intent_helper/supported_links_infobar_delegate.h"
#endif

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
#include "chrome/browser/lens/region_search/lens_region_search_controller.h"
#include "chrome/browser/lens/region_search/lens_region_search_helper.h"
#include "components/lens/lens_features.h"
#endif

namespace {

const char kOsOverrideForTabletSite[] = "Linux; Android 9; Chrome tablet";
Expand Down Expand Up @@ -1852,4 +1859,24 @@ void RunScreenAIVisualAnnotation(Browser* browser) {
}
#endif // BUILDFLAG(ENABLE_SCREEN_AI_SERVICE)

void ExecLensRegionSearch(Browser* browser) {
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
Profile* profile = browser->profile();
WebContents* contents = browser->tab_strip_model()->GetActiveWebContents();
GURL url = contents->GetController().GetLastCommittedEntry()->GetURL();

if (lens::IsRegionSearchEnabled(browser, profile, url)) {
auto lens_region_search_controller_data =
std::make_unique<lens::LensRegionSearchControllerData>();
lens_region_search_controller_data->lens_region_search_controller =
std::make_unique<lens::LensRegionSearchController>(browser);
lens_region_search_controller_data->lens_region_search_controller->Start(
contents, lens::features::IsLensFullscreenSearchEnabled(),
search::DefaultSearchProviderIsGoogle(profile));
browser->SetUserData(lens::LensRegionSearchControllerData::kDataKey,
std::move(lens_region_search_controller_data));
}
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)
}

} // namespace chrome
2 changes: 2 additions & 0 deletions chrome/browser/ui/browser_commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,8 @@ void UnfollowSite(content::WebContents* web_contents);
void RunScreenAIVisualAnnotation(Browser* browser);
#endif // BUILDFLAG(ENABLE_SCREEN_AI_SERVICE)

void ExecLensRegionSearch(Browser* browser);

} // namespace chrome

#endif // CHROME_BROWSER_UI_BROWSER_COMMANDS_H_
1 change: 1 addition & 0 deletions chrome/browser/ui/views/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ component("views") {
"//chrome/app:command_ids",
"//chrome/app:generated_resources",
"//components/keep_alive_registry",
"//components/lens",
"//components/services/app_service/public/mojom",
"//components/services/screen_ai/buildflags",
"//components/vector_icons",
Expand Down
20 changes: 20 additions & 0 deletions chrome/browser/ui/views/accelerator_table.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "chrome/app/chrome_command_ids.h"
#include "components/lens/lens_features.h"
#include "components/services/screen_ai/buildflags/buildflags.h"
#include "printing/buildflags/buildflags.h"
#include "ui/base/accelerators/accelerator.h"
Expand Down Expand Up @@ -262,6 +263,16 @@ const AcceleratorMapping kEnableWithNewMappingAcceleratorMap[] = {
};
#endif

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
// Accelerators to enable if lens::features::kEnableRegionSearchKeyboardShortcut
// is true.
constexpr AcceleratorMapping kRegionSearchAcceleratorMap[] = {
// TODO(nguyenbryan): This is a temporary hotkey; update when finalized.
{ui::VKEY_L, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH},
};
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)

constexpr int kDebugModifier =
ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_SHIFT_DOWN;

Expand Down Expand Up @@ -307,6 +318,15 @@ std::vector<AcceleratorMapping> GetAcceleratorList() {
}
#endif

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
if (base::FeatureList::IsEnabled(
lens::features::kEnableRegionSearchKeyboardShortcut)) {
accelerators->insert(accelerators->begin(),
std::begin(kRegionSearchAcceleratorMap),
std::end(kRegionSearchAcceleratorMap));
}
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)

if (base::FeatureList::IsEnabled(features::kUIDebugTools)) {
accelerators->insert(accelerators->begin(),
std::begin(kUIDebugAcceleratorMap),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <utility>

#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/logging.h"
#include "base/memory/raw_ptr.h"
#include "base/observer_list.h"
Expand All @@ -21,6 +22,7 @@
#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/grit/generated_resources.h"
#include "components/lens/lens_features.h"
#include "components/renderer_context_menu/views/toolkit_delegate_views.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/render_widget_host.h"
Expand Down Expand Up @@ -209,6 +211,20 @@ bool RenderViewContextMenuViews::GetAcceleratorForCommandId(
*accel = ui::Accelerator(ui::VKEY_S, ui::EF_CONTROL_DOWN);
return true;

#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
case IDC_CONTENT_CONTEXT_LENS_REGION_SEARCH:
case IDC_CONTENT_CONTEXT_WEB_REGION_SEARCH:
if (base::FeatureList::IsEnabled(
lens::features::kEnableRegionSearchKeyboardShortcut)) {
// TODO(nguyenbryan): This is a temporary hotkey; update when finalized.
*accel = ui::Accelerator(ui::VKEY_L,
ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN);
return true;
} else {
return false;
}
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)

case IDC_CONTENT_CONTEXT_EXIT_FULLSCREEN: {
// Esc only works in HTML5 (site-triggered) fullscreen.
if (IsHTML5Fullscreen()) {
Expand Down
4 changes: 4 additions & 0 deletions components/lens/lens_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ BASE_FEATURE(kEnableLatencyLogging,
"LensImageLatencyLogging",
base::FEATURE_ENABLED_BY_DEFAULT);

BASE_FEATURE(kEnableRegionSearchKeyboardShortcut,
"LensEnableRegionSearchKeyboardShortcut",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kEnableRegionSearchOnPdfViewer,
"LensEnableRegionSearchOnPdfViewer",
base::FEATURE_ENABLED_BY_DEFAULT);
Expand Down
4 changes: 4 additions & 0 deletions components/lens/lens_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ BASE_DECLARE_FEATURE(kLensSearchImageInScreenshotSharing);
COMPONENT_EXPORT(LENS_FEATURES)
BASE_DECLARE_FEATURE(kEnableLatencyLogging);

// Enable keyboard shortcut for the Lens Region Search feature.
COMPONENT_EXPORT(LENS_FEATURES)
BASE_DECLARE_FEATURE(kEnableRegionSearchKeyboardShortcut);

// Enable the Lens Region Search feature on the PDF viewer.
COMPONENT_EXPORT(LENS_FEATURES)
BASE_DECLARE_FEATURE(kEnableRegionSearchOnPdfViewer);
Expand Down

0 comments on commit 422b5c3

Please sign in to comment.