0

Control page actions from LensOverlayEntryPointController

This CL updates LensOverlayEntryPointController to control page actions through the new page actions framework. With this new framework, the Lens Overlay controller is responsible for listening and responding to:
* Updates to the "Google Lens Shortcut" preference
* Focus changes on the location bar.

This matches the current conditions of the legacy Lens Overlay page action implementation.

Notes:
1. The new path is gated by a new feature flag,`PageActionsMigration`.
2. A logical circular dependency on `c/b/ui:ui` is introduced to `c/b/ui/lens:lens`, because the lens controller needs to read NTP state, which hasn't been modularized. The lens BUILD is split into  header and impl targets to accommodate this.

Change-Id: Iada3e1330b97cbb878e6b9306937022439d8d658
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6038852
Reviewed-by: Caroline Rising <corising@chromium.org>
Reviewed-by: David Pennington <dpenning@chromium.org>
Commit-Queue: Kaan Alsan <alsan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1393957}
This commit is contained in:
Kaan Alsan
2024-12-09 23:15:26 +00:00
committed by Chromium LUCI CQ
parent 67e2ca00a9
commit 7ac0eb2a03
15 changed files with 306 additions and 51 deletions

@ -1891,6 +1891,9 @@ static_library("ui") {
# //c/b/ui.
"//chrome/browser/ui/tabs:tab_strip_model_observer_impl",
# TODO(crbug.com/382237520): Remove this once NTP code is modularized.
"//chrome/browser/ui/lens:impl",
"//chrome/browser/ui/signin:impl",
"//chrome/browser/ui/webui/commerce:impl",
"//chrome/browser/ui/webui/signin:signin_impl",
@ -2989,6 +2992,7 @@ static_library("ui") {
"//chrome/browser/themes",
"//chrome/browser/ui/autofill/payments",
"//chrome/browser/ui/lens",
"//chrome/browser/ui/lens:impl",
"//components/lens/proto/server:proto",
"//third_party/lens_server_proto:lens_overlay_proto",
"//third_party/omnibox_proto",

@ -30,6 +30,9 @@ using bookmarks::BookmarkNode;
namespace {
// TODO(crbug.com/382494946): Similar bespoke checks are used throughout the
// codebase. This should be factored out as a common util and other callsites
// converted to use this.
bool IsNTP(content::WebContents* web_contents) {
// Use the committed entry (or the visible entry, if the committed entry is
// the initial NavigationEntry) so the bookmarks bar disappears at the same

@ -33,6 +33,7 @@
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/data_sharing/data_sharing_open_group_helper.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
#include "chrome/browser/ui/views/media_router/cast_browser_controller.h"
#include "chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_toolbar_bubble_controller.h"
#include "chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h"
@ -138,14 +139,23 @@ void BrowserWindowFeatures::InitPostWindowConstruction(Browser* browser) {
send_tab_to_self_toolbar_bubble_controller_ = std::make_unique<
send_tab_to_self::SendTabToSelfToolbarBubbleController>(browser);
// TODO(b/350508658): Ideally, we don't pass in a reference to browser as
// per the guidance in the comment above. However, currently, we need
// browser to properly determine if the lens overlay is enabled.
// TODO(crbug.com/350508658): Ideally, we don't pass in a reference to
// browser as per the guidance in the comment above. However, currently,
// we need browser to properly determine if the lens overlay is enabled.
// Cannot be in Init since needs to listen to the fullscreen controller
// which is initialized after Init.
// and location bar view which are initialized after Init.
if (lens::features::IsLensOverlayEnabled()) {
views::View* location_bar = nullptr;
// TODO(crbug.com/360163254): We should really be using
// Browser::GetBrowserView, which always returns a non-null BrowserView
// in production, but this crashes during unittests using
// BrowserWithTestWindowTest; these should eventually be refactored.
if (BrowserView* browser_view =
BrowserView::GetBrowserViewForBrowser(browser)) {
location_bar = browser_view->GetLocationBarView();
}
lens_overlay_entry_point_controller_->Initialize(
browser, browser->command_controller());
browser, browser->command_controller(), location_bar);
}
auto* experiment_manager =
@ -212,6 +222,7 @@ void BrowserWindowFeatures::InitPostBrowserViewConstruction(
void BrowserWindowFeatures::TearDownPreBrowserViewDestruction() {
memory_saver_opt_in_iph_controller_.reset();
lens_overlay_entry_point_controller_.reset();
// TODO(crbug.com/346148093): This logic should not be gated behind a
// conditional.

@ -28,63 +28,82 @@ source_set("lens") {
"//chrome/browser/ui/ash/capture_mode",
]
sources = [
"lens_overlay_blur_layer_delegate.cc",
"lens_overlay_blur_layer_delegate.h",
"lens_overlay_colors.h",
"lens_overlay_controller.cc",
"lens_overlay_entry_point_controller.cc",
"lens_overlay_event_handler.cc",
"lens_overlay_event_handler.h",
"lens_overlay_gen204_controller.cc",
"lens_overlay_image_helper.cc",
"lens_overlay_image_helper.h",
"lens_overlay_languages_controller.cc",
"lens_overlay_languages_controller.h",
"lens_overlay_proto_converter.cc",
"lens_overlay_proto_converter.h",
"lens_overlay_query_controller.cc",
"lens_overlay_query_controller.h",
"lens_overlay_request_id_generator.cc",
"lens_overlay_request_id_generator.h",
"lens_overlay_side_panel_coordinator.cc",
"lens_overlay_side_panel_coordinator.h",
"lens_overlay_side_panel_navigation_throttle.cc",
"lens_overlay_side_panel_web_view.cc",
"lens_overlay_side_panel_web_view.h",
"lens_overlay_theme_utils.cc",
"lens_overlay_theme_utils.h",
"lens_overlay_untrusted_ui.cc",
"lens_overlay_url_builder.cc",
"lens_overlay_url_builder.h",
"lens_permission_bubble_controller.cc",
"lens_permission_bubble_controller.h",
"lens_preselection_bubble.cc",
"lens_preselection_bubble.h",
"lens_side_panel_untrusted_ui.cc",
"ref_counted_lens_overlay_client_logs.h",
]
public_deps = [
"//base",
"//chrome/browser/content_extraction",
"//chrome/browser/lens/core/mojom:mojo_bindings",
"//chrome/browser/search_engines:search_engines",
"//chrome/browser/themes",
"//chrome/browser/ui/exclusive_access",
"//chrome/browser/ui/omnibox",
"//chrome/browser/ui/views/side_panel",
"//chrome/browser/ui/webui/searchbox",
"//chrome/browser/ui/webui/top_chrome",
"//components/endpoint_fetcher",
"//components/find_in_page",
"//components/lens",
"//components/user_education/webui",
"//content/public/browser",
"//third_party/lens_server_proto:lens_overlay_proto",
"//ui/base/mojom:ui_base_types",
"//ui/views",
]
if (enable_pdf) {
public_deps += [ "//pdf/mojom" ]
}
}
source_set("impl") {
public = []
sources = [
"lens_overlay_blur_layer_delegate.cc",
"lens_overlay_controller.cc",
"lens_overlay_entry_point_controller.cc",
"lens_overlay_event_handler.cc",
"lens_overlay_gen204_controller.cc",
"lens_overlay_image_helper.cc",
"lens_overlay_languages_controller.cc",
"lens_overlay_proto_converter.cc",
"lens_overlay_query_controller.cc",
"lens_overlay_request_id_generator.cc",
"lens_overlay_side_panel_coordinator.cc",
"lens_overlay_side_panel_navigation_throttle.cc",
"lens_overlay_side_panel_web_view.cc",
"lens_overlay_theme_utils.cc",
"lens_overlay_untrusted_ui.cc",
"lens_overlay_url_builder.cc",
"lens_permission_bubble_controller.cc",
"lens_preselection_bubble.cc",
"lens_side_panel_untrusted_ui.cc",
]
public_deps = []
deps = [
"//base",
":lens",
"//build:branding_buildflags",
"//chrome/app:generated_resources_grit",
"//chrome/app/theme:theme_resources_grit",
"//chrome/app/vector_icons",
"//chrome/browser:browser_process",
"//chrome/browser:primitives",
"//chrome/browser/content_extraction",
"//chrome/browser/feedback",
"//chrome/browser/media/webrtc",
"//chrome/browser/profiles",
@ -93,25 +112,21 @@ source_set("lens") {
"//chrome/browser/resources/lens/shared:resources",
"//chrome/browser/resources/lens/shared:resources_grit",
"//chrome/browser/search",
"//chrome/browser/search_engines:search_engines",
"//chrome/browser/task_manager",
"//chrome/browser/ui:browser_element_identifiers",
"//chrome/browser/ui/browser_window",
"//chrome/browser/ui/color:color_headers",
"//chrome/browser/ui/exclusive_access",
"//chrome/browser/ui/hats",
"//chrome/browser/ui/views/bubble",
"//chrome/browser/ui/views/side_panel",
"//chrome/browser/ui/views/page_action",
"//chrome/browser/ui/views/toolbar",
"//chrome/browser/ui/webui",
"//chrome/browser/ui/webui:webui_util",
"//chrome/browser/ui/webui/searchbox",
"//chrome/browser/ui/webui/util",
"//chrome/common",
"//chrome/common:channel_info",
"//components/base32",
"//components/constrained_window",
"//components/endpoint_fetcher",
"//components/feature_engagement/public",
"//components/input",
"//components/language/core/common",
@ -126,7 +141,6 @@ source_set("lens") {
"//components/sync/service",
"//components/sync_preferences",
"//components/unified_consent",
"//components/user_education/webui",
"//components/variations",
"//components/variations:variations_mojom",
"//components/version_info:channel",
@ -150,14 +164,12 @@ source_set("lens") {
"//ui/gfx/codec:codec",
"//ui/gfx/geometry:geometry",
"//ui/menus",
"//ui/views",
"//ui/webui",
"//url",
]
if (enable_pdf) {
deps += [ "//components/pdf/browser" ]
public_deps += [ "//pdf/mojom" ]
}
}

@ -4,6 +4,7 @@
#include "chrome/browser/ui/lens/lens_overlay_entry_point_controller.h"
#include "base/functional/bind.h"
#include "base/system/sys_info.h"
#include "chrome/browser/command_updater.h"
#include "chrome/browser/search/search.h"
@ -12,10 +13,41 @@
#include "chrome/browser/ui/exclusive_access/exclusive_access_context.h"
#include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h"
#include "chrome/browser/ui/lens/lens_overlay_controller.h"
#include "chrome/browser/ui/tabs/public/tab_features.h"
#include "chrome/browser/ui/tabs/public/tab_interface.h"
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/page_action/page_action_controller.h"
#include "chrome/browser/ui/views/toolbar/pinned_toolbar_actions_container.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.h"
#include "chrome/browser/ui/webui/new_tab_page_third_party/new_tab_page_third_party_ui.h"
#include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
#include "components/lens/lens_features.h"
#include "components/lens/lens_overlay_permission_utils.h"
#include "components/omnibox/browser/omnibox_prefs.h"
#include "content/public/browser/navigation_entry.h"
namespace {
// TODO(crbug.com/382494946): Similar bespoke checks are used throughout the
// codebase. This should be factored out as a common util and other callsites
// converted to use this.
bool IsNewTabPage(content::WebContents* const web_contents) {
// Use the committed entry (or the visible entry, if the committed entry is
// the initial NavigationEntry).
CHECK(web_contents);
content::NavigationEntry* entry =
web_contents->GetController().GetLastCommittedEntry();
if (entry->IsInitialEntry()) {
entry = web_contents->GetController().GetVisibleEntry();
}
const GURL& url = entry->GetURL();
return NewTabUI::IsNewTab(url) || NewTabPageUI::IsNewTabPageOrigin(url) ||
NewTabPageThirdPartyUI::IsNewTabPageOrigin(url) ||
search::NavEntryIsInstantNTP(web_contents, entry);
}
} // namespace
namespace lens {
@ -23,8 +55,22 @@ LensOverlayEntryPointController::LensOverlayEntryPointController() = default;
void LensOverlayEntryPointController::Initialize(
BrowserWindowInterface* browser_window_interface,
CommandUpdater* command_updater) {
CommandUpdater* command_updater,
views::View* location_bar) {
browser_window_interface_ = browser_window_interface;
location_bar_ = location_bar;
if (location_bar_) {
location_bar_->GetFocusManager()->AddFocusChangeListener(this);
location_bar_->AddObserver(this);
}
pref_change_registrar_.Init(
browser_window_interface_->GetProfile()->GetPrefs());
pref_change_registrar_.Add(
omnibox::kShowGoogleLensShortcut,
base::BindRepeating(
&LensOverlayEntryPointController::UpdatePageActionState,
base::Unretained(this)));
command_updater_ = command_updater;
// Observe changes to fullscreen state.
@ -43,7 +89,12 @@ void LensOverlayEntryPointController::Initialize(
UpdateEntryPointsState(/*hide_if_needed=*/true);
}
LensOverlayEntryPointController::~LensOverlayEntryPointController() = default;
LensOverlayEntryPointController::~LensOverlayEntryPointController() {
// Initialize may not have been called (e.g. for non-normal browser windows).
if (location_bar_) {
location_bar_->RemoveObserver(this);
}
}
bool LensOverlayEntryPointController::IsEnabled() {
// This class is initialized if and only if it is observing.
@ -98,6 +149,25 @@ bool LensOverlayEntryPointController::IsEnabled() {
return phys_mem_mb > lens::features::GetLensOverlayMinRamMb();
}
void LensOverlayEntryPointController::OnViewAddedToWidget(views::View* view) {
CHECK(location_bar_);
location_bar_->GetFocusManager()->AddFocusChangeListener(this);
}
void LensOverlayEntryPointController::OnViewRemovedFromWidget(
views::View* view) {
CHECK(location_bar_);
location_bar_->GetFocusManager()->RemoveFocusChangeListener(this);
}
void LensOverlayEntryPointController::OnWillChangeFocus(views::View* before,
views::View* now) {}
void LensOverlayEntryPointController::OnDidChangeFocus(views::View* before,
views::View* now) {
UpdatePageActionState();
}
void LensOverlayEntryPointController::OnFullscreenStateChanged() {
// Disable the Lens entry points in the top chrome if there is no top bar in
// Chrome. On Mac and ChromeOS, it is possible to hover over the top of the
@ -138,4 +208,59 @@ actions::ActionItem* LensOverlayEntryPointController::GetToolbarEntrypoint() {
kActionSidePanelShowLensOverlayResults,
/*scope=*/browser_window_interface_->GetActions()->root_action_item());
}
void LensOverlayEntryPointController::UpdatePageActionState() {
if (!base::FeatureList::IsEnabled(::features::kPageActionsMigration)) {
return;
}
// This may not have been initialized (e.g. for non-normal browser types).
if (!location_bar_) {
return;
}
CHECK(browser_window_interface_);
tabs::TabInterface* active_tab =
browser_window_interface_->GetActiveTabInterface();
// Possible during browser window initialization or teardown.
if (!active_tab) {
return;
}
CHECK(active_tab->GetTabFeatures());
page_actions::PageActionController* page_action_controller =
active_tab->GetTabFeatures()->page_action_controller();
CHECK(page_action_controller);
const actions::ActionId page_action_id =
kActionSidePanelShowLensOverlayResults;
if (!IsEnabled()) {
page_action_controller->Hide(page_action_id);
return;
}
if (!browser_window_interface_->GetProfile()->GetPrefs()->GetBoolean(
omnibox::kShowGoogleLensShortcut)) {
page_action_controller->Hide(page_action_id);
return;
}
if (!features::IsOmniboxEntrypointAlwaysVisible() &&
!location_bar_->HasFocus()) {
page_action_controller->Hide(page_action_id);
return;
}
// The overlay is unavailable on the NTP as it is unlikely to be useful to
// users on the page. It would also appear immediately when a new tab or
// window is created due to focus immediatey jumping into the location bar.
if (active_tab && IsNewTabPage(active_tab->GetContents())) {
page_action_controller->Hide(page_action_id);
return;
}
// TODO(crbug.com/376283383): We should always use the chip state once that's
// implemented.
page_action_controller->Show(page_action_id);
}
} // namespace lens

@ -8,13 +8,20 @@
#include "base/scoped_observation.h"
#include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
#include "chrome/browser/ui/exclusive_access/fullscreen_observer.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/search_engines/template_url_service.h"
#include "components/search_engines/template_url_service_observer.h"
#include "ui/actions/actions.h"
#include "ui/views/focus/focus_manager.h"
#include "ui/views/view_observer.h"
class BrowserWindowInterface;
class CommandUpdater;
namespace views {
class View;
}
namespace lens {
// Per-browser-window class responsible for keeping Lens Overlay entry points in
@ -22,14 +29,17 @@ namespace lens {
// LensOverlayController, since LensOverlayController exist per tab, while entry
// points are per browser window.
class LensOverlayEntryPointController : public FullscreenObserver,
public TemplateURLServiceObserver {
public TemplateURLServiceObserver,
public views::FocusChangeListener,
public views::ViewObserver {
public:
LensOverlayEntryPointController();
~LensOverlayEntryPointController() override;
// This class does nothing if not initialized. IsEnabled returns false.
void Initialize(BrowserWindowInterface* browser_window_interface,
CommandUpdater* command_updater);
CommandUpdater* command_updater,
views::View* location_bar);
// Whether the entry points should be enabled.
bool IsEnabled();
@ -42,11 +52,22 @@ class LensOverlayEntryPointController : public FullscreenObserver,
void OnTemplateURLServiceChanged() override;
void OnTemplateURLServiceShuttingDown() override;
// views::FocusChangeListener
void OnWillChangeFocus(views::View* before, views::View* now) override;
void OnDidChangeFocus(views::View* before, views::View* now) override;
// views::ViewObserver
void OnViewAddedToWidget(views::View* view) override;
void OnViewRemovedFromWidget(views::View* view) override;
// Updates the enable/disable state of entry points. If hide_if_needed is
// true, instead of just disabling the entrypoint, we will also hide the
// entrypoint from the user.
void UpdateEntryPointsState(bool hide_if_needed);
// Updates the Lens Overlay page action state.
void UpdatePageActionState();
// Returns the ActionItem corresponding to our pinnable toolbar entrypoint.
actions::ActionItem* GetToolbarEntrypoint();
@ -65,6 +86,10 @@ class LensOverlayEntryPointController : public FullscreenObserver,
// Owns this.
raw_ptr<BrowserWindowInterface> browser_window_interface_;
PrefChangeRegistrar pref_change_registrar_;
raw_ptr<views::View> location_bar_;
};
} // namespace lens

@ -448,4 +448,8 @@ BASE_FEATURE(kInlineFullscreenPerfExperiment,
"InlineFullscreenPerfExperiment",
base::FEATURE_ENABLED_BY_DEFAULT);
BASE_FEATURE(kPageActionsMigration,
"PageActionsMigration",
base::FEATURE_DISABLED_BY_DEFAULT);
} // namespace features

@ -272,6 +272,10 @@ BASE_DECLARE_FEATURE(kEnablePolicyPromotionBanner);
// checking is enabled.
BASE_DECLARE_FEATURE(kInlineFullscreenPerfExperiment);
// Controls whether the new page actions framework should be displaying page
// actions.
BASE_DECLARE_FEATURE(kPageActionsMigration);
} // namespace features
#endif // CHROME_BROWSER_UI_UI_FEATURES_H_

@ -33,9 +33,9 @@
namespace {
// TODO(tluk): Similar bespoke checks are used throughout the codebase, this
// approach is taken from BookmarkTabHelper. This should be factored out as a
// common util and other callsites converted to use this.
// TODO(crbug.com/382494946): Similar bespoke checks are used throughout the
// codebase, this approach is taken from BookmarkTabHelper. This should be
// factored out as a common util and other callsites converted to use this.
bool IsNewTabPage(content::WebContents* const web_contents) {
// Use the committed entry (or the visible entry, if the committed entry is
// the initial NavigationEntry) so the bookmarks bar disappears at the same

@ -3,11 +3,19 @@
// found in the LICENSE file.
// #include "build/build_config.h"
#include <memory>
#include "base/functional/callback_forward.h"
#include "base/test/run_until.h"
#include "chrome/browser/ui/actions/chrome_action_id.h"
#include "chrome/browser/ui/browser_element_identifiers.h"
#include "chrome/browser/ui/tabs/public/tab_features.h"
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/location_bar/lens_overlay_page_action_icon_view.h"
#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
#include "chrome/browser/ui/views/page_action/page_action_controller.h"
#include "chrome/browser/ui/views/page_action/page_action_view.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
@ -17,6 +25,7 @@
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "ui/views/interaction/element_tracker_views.h"
#include "ui/views/test/widget_test.h"
#include "url/url_constants.h"
using ::testing::MatchesRegex;
@ -66,6 +75,24 @@ class LensOverlayPageActionIconViewTestBase : public InProcessBrowserTest {
browser_view->toolbar()->location_bar());
}
page_actions::PageActionView* lens_overlay_page_action_view() {
return static_cast<page_actions::PageActionView*>(
views::test::AnyViewMatchingPredicate(
location_bar(), [=](const views::View* candidate) -> bool {
return IsViewClass<page_actions::PageActionView>(candidate) &&
static_cast<const page_actions::PageActionView*>(candidate)
->GetActionId() ==
kActionSidePanelShowLensOverlayResults;
}));
}
page_actions::PageActionController* page_action_controller() {
return browser()
->GetActiveTabInterface()
->GetTabFeatures()
->page_action_controller();
}
protected:
base::test::ScopedFeatureList scoped_feature_list_;
};
@ -74,7 +101,12 @@ class LensOverlayPageActionIconViewTest
: public LensOverlayPageActionIconViewTestBase {
public:
LensOverlayPageActionIconViewTest() {
scoped_feature_list_.InitWithFeatures({lens::features::kLensOverlay}, {});
scoped_feature_list_.InitWithFeatures(
{
lens::features::kLensOverlay,
::features::kPageActionsMigration,
},
{});
}
};
@ -94,18 +126,23 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL)));
LensOverlayPageActionIconView* icon_view = lens_overlay_icon_view();
page_actions::PageActionView* page_action_view =
lens_overlay_page_action_view();
views::FocusManager* focus_manager = icon_view->GetFocusManager();
focus_manager->ClearFocus();
EXPECT_FALSE(focus_manager->GetFocusedView());
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// Focus in the location bar should show the icon.
base::RunLoop run_loop;
icon_view->set_update_callback_for_testing(run_loop.QuitClosure());
location_bar()->FocusLocation(false);
EXPECT_TRUE(focus_manager->GetFocusedView());
run_loop.Run();
EXPECT_TRUE(icon_view->GetVisible());
EXPECT_TRUE(page_action_view->GetVisible());
}
IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
@ -124,9 +161,12 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
LensOverlayPageActionIconView* icon_view = lens_overlay_icon_view();
views::FocusManager* focus_manager = icon_view->GetFocusManager();
page_actions::PageActionView* page_action_view =
lens_overlay_page_action_view();
focus_manager->ClearFocus();
EXPECT_FALSE(focus_manager->GetFocusedView());
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// Focus in the location bar should show the icon.
base::RunLoop run_loop;
@ -135,6 +175,7 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
EXPECT_TRUE(focus_manager->GetFocusedView());
run_loop.Run();
EXPECT_TRUE(icon_view->GetVisible());
EXPECT_TRUE(page_action_view->GetVisible());
// Executing the lens overlay icon view with keyboard source should open a new
// tab.
@ -159,10 +200,13 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL)));
LensOverlayPageActionIconView* icon_view = lens_overlay_icon_view();
page_actions::PageActionView* page_action_view =
lens_overlay_page_action_view();
views::FocusManager* focus_manager = icon_view->GetFocusManager();
focus_manager->ClearFocus();
EXPECT_FALSE(focus_manager->GetFocusedView());
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// The icon should remain hidden despite focus in the location bar.
base::RunLoop run_loop;
@ -171,6 +215,7 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
EXPECT_TRUE(focus_manager->GetFocusedView());
run_loop.Run();
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
}
IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest, DoesNotShowOnNTP) {
@ -179,10 +224,13 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest, DoesNotShowOnNTP) {
browser(), GURL(chrome::kChromeUINewTabPageURL)));
LensOverlayPageActionIconView* icon_view = lens_overlay_icon_view();
page_actions::PageActionView* page_action_view =
lens_overlay_page_action_view();
views::FocusManager* focus_manager = icon_view->GetFocusManager();
focus_manager->ClearFocus();
EXPECT_FALSE(focus_manager->GetFocusedView());
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// The icon should remain hidden despite focus in the location bar.
base::RunLoop run_loop;
@ -191,6 +239,7 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest, DoesNotShowOnNTP) {
EXPECT_TRUE(focus_manager->GetFocusedView());
run_loop.Run();
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
}
IN_PROC_BROWSER_TEST_F(
@ -215,10 +264,13 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL)));
LensOverlayPageActionIconView* icon_view = lens_overlay_icon_view();
page_actions::PageActionView* page_action_view =
lens_overlay_page_action_view();
views::FocusManager* focus_manager = icon_view->GetFocusManager();
focus_manager->ClearFocus();
EXPECT_FALSE(focus_manager->GetFocusedView());
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// Focus in the location bar should show the icon.
base::RunLoop run_loop;
@ -227,16 +279,19 @@ IN_PROC_BROWSER_TEST_F(LensOverlayPageActionIconViewTest,
EXPECT_TRUE(focus_manager->GetFocusedView());
run_loop.Run();
EXPECT_TRUE(icon_view->GetVisible());
EXPECT_TRUE(page_action_view->GetVisible());
// Disable the preference, the entrypoint should immediately disappear.
browser()->profile()->GetPrefs()->SetBoolean(omnibox::kShowGoogleLensShortcut,
false);
EXPECT_FALSE(icon_view->GetVisible());
EXPECT_FALSE(page_action_view->GetVisible());
// Re-enable the preference, the entrypoint should immediately become visible.
browser()->profile()->GetPrefs()->SetBoolean(omnibox::kShowGoogleLensShortcut,
true);
EXPECT_TRUE(icon_view->GetVisible());
EXPECT_TRUE(page_action_view->GetVisible());
}
} // namespace

@ -592,6 +592,9 @@ void LocationBarView::OnWillChangeFocus(views::View* before, views::View* now) {
}
void LocationBarView::OnDidChangeFocus(views::View* before, views::View* now) {
// TODO(crbug.com/376283383): Remove this once Lens Overlay is migrated to the
// new page actions design.
// This is very blunt. There's a page action (LensOverlayPageActionView) whose
// visibility state depends on whether focus is within the location bar or
// not. Maybe that dependency should be better understood rather than "refresh

@ -52,6 +52,10 @@ PageActionView::GetActionViewInterface() {
observation_.GetSource());
}
actions::ActionId PageActionView::GetActionId() const {
return action_item_->GetActionId().value();
}
BEGIN_METADATA(PageActionView)
END_METADATA

@ -37,6 +37,8 @@ class PageActionView : public IconLabelBubbleView,
void OnPageActionModelChanged(PageActionModel* model) override;
void OnPageActionModelWillBeDeleted(PageActionModel* model) override;
actions::ActionId GetActionId() const;
private:
base::WeakPtr<actions::ActionItem> action_item_ = nullptr;
base::ScopedObservation<PageActionModel, PageActionModelObserver>

@ -10665,6 +10665,7 @@ if (!is_android && !is_chromeos_device) {
"//chrome/browser/ui/toasts",
"//chrome/browser/ui/toasts:interactive_ui_tests",
"//chrome/browser/ui/views/bubble",
"//chrome/browser/ui/views/page_action",
"//chrome/browser/ui/views/side_panel",
"//chrome/browser/ui/views/toolbar",
"//chrome/browser/user_annotations:user_annotations",

@ -26,15 +26,6 @@ code. Some code is used on Android.
`//chrome/test/BUILD.gn` or `//chrome/browser/ui/BUILD.gn:ui`.
* gn circular dependencies are disallowed. Logical
circular dependencies are allowed (for legacy reasons) but discouraged.
* [Lens
overlay](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/lens/BUILD.gn;drc=8e2c1c747f15a93c55ab2f10ebc8b32801ba129e)
is an example of a feature with no circular dependencies.
* The BUILD.gn should use public/sources separation.
* The main reason for this is to guard against future, unexpected usage
of parts of the code that were intended to be private. This makes it
difficult to change implementation details in the future.
* This directory may have a public/ subdirectory to enforce further
encapsulation, though this example does not use it.
* [cookie
controls](https://chromium-review.googlesource.com/c/chromium/src/+/5771416/5/chrome/browser/ui/cookie_controls/BUILD.gn)
is an example of a feature with logical circular dependencies.
@ -52,6 +43,17 @@ code. Some code is used on Android.
still logical circular dependencies from the cc files. This
discrepancy is because C++ allows headers to forward declare
dependencies, which do not need to be reflected in gn.
* [Lens
overlay](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/lens/BUILD.gn;drc=8e2c1c747f15a93c55ab2f10ebc8b32801ba129e)
is an example with *almost* no circular dependencies.
* It has a logical circular dependency on `//chrome/browser/browser/ui:ui`,
which will no longer be necessary once NTP is also modularized (crbug.com/382237520).
* The BUILD.gn should use public/sources separation.
* The main reason for this is to guard against future, unexpected usage
of parts of the code that were intended to be private. This makes it
difficult to change implementation details in the future.
* This directory may have a public/ subdirectory to enforce further
encapsulation, though this example does not use it.
* This directory may have its own namespace.
* Corollary: There are several global functions that facilitate dependency
inversion. It will not be possible to call them from modularized features