From abe5c21074e51a7cb368545fed37d7f8b5a3e5b4 Mon Sep 17 00:00:00 2001 From: Claudio DeSouza Date: Wed, 5 Nov 2025 22:33:32 +0000 Subject: [PATCH] [cr143] Correct access to Omnibox UI classes Chromium has changed how access to the `OmniboxEditModel` instance occurs, now routing it from the LocationBar, through the OmniboxController reference. This change corrects several tests and places in the codebase to relfect that. It has also been necessary to add friend access to `OmniboxPopupView` for `BraveOmniboxResultView` as `OmniboxResultView` relies on that. Chromium changes: https://chromium.googlesource.com/chromium/src/+/d976b3a19103999113e51ae709c2632181b747f8 commit d976b3a19103999113e51ae709c2632181b747f8 Author: Moe Ahmadi Date: Mon Nov 3 19:00:59 2025 -0800 [omnibox] Refactors ownership and access to Omnibox UI classes This CL refactors the ownership and dependencies between key Omnibox UI classes, i.e., OmniboxController, OmniboxEditModel, OmniboxView, and LocationBarView. - Moves ownership of OmniboxController from OmniboxView to LocationBarView and ensures it outlives OmniboxView. - Makes OmniboxView available to OmniboxController and OmniboxEditModel post-construction, similar to how OmniboxPopupView is set and unset. - Removes public access to OmniboxController and OmniboxEditModel through OmniboxView. - Enforces access to OmniboxEditModel through OmniboxController. The overall goal is to improve the comprehension and adaptability of the Omnibox UI classes for longer-term UI architectural improvements: Transitioning UI components to WebUI necessitates an asynchronous interaction model, as direct synchronous access to View objects across process boundaries is not possible. This CL minimizes coupling of the Controller/Model with the View at construction time and minimizes the API surface of OmniboxView by removing direct access to the Controller and Model, preparing for a future where view updates are driven by state change observation, rather than direct manipulation. By moving ownership of OmniboxController to LocationBarView and extending its lifetime, this CL allows it to mediate between the OmniboxEditModel, the OmniboxView, and the OmniboxPopupView(s), managing their state and visibility. The existing OmniboxEditModel contains logic that more appropriately belongs in a Controller. By enforcing all access to the OmniboxEditModel through the OmniboxController, this CL makes it easier to identify and incrementally migrate controller-like responsibilities out of the Model and into the Controller, leading to a better separation of concerns and aligning with MVC principles. BYPASS_LARGE_CHANGE_WARNING: Largely mechanical refactoring Bug: 40251974 Change-Id: Id159acd262f444cc1ad3d83bd80d63b89c104c55 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7110919 Commit-Queue: Moe Ahmadi Reviewed-by: Erik Chen Cr-Commit-Position: refs/heads/main@{#1539750} --- browser/ai_chat/ai_chat_policy_browsertest.cc | 8 +++--- .../ai_chat_profiles_enabled_browsertest.cc | 3 +-- .../location_bar/brave_location_bar_view.cc | 4 ++- .../brave_location_bar_view_browsertest.cc | 23 +++++++++++------ .../promotion_button_controller.cc | 13 +++++++--- .../omnibox/brave_omnibox_result_view.cc | 4 +-- .../views/omnibox/brave_omnibox_view_views.cc | 6 ++--- .../omnibox_autocomplete_browsertest.cc | 19 ++++++++------ .../browser/ui/omnibox/omnibox_popup_view.h | 19 ++++++++++++++ .../location_bar_view_browsertest.cc | 25 ++++++++++++++++--- 10 files changed, 91 insertions(+), 33 deletions(-) create mode 100644 chromium_src/chrome/browser/ui/omnibox/omnibox_popup_view.h diff --git a/browser/ai_chat/ai_chat_policy_browsertest.cc b/browser/ai_chat/ai_chat_policy_browsertest.cc index f921edef0b0..4835e7865a2 100644 --- a/browser/ai_chat/ai_chat_policy_browsertest.cc +++ b/browser/ai_chat/ai_chat_policy_browsertest.cc @@ -77,9 +77,11 @@ class AIChatPolicyTest : public InProcessBrowserTest, } AutocompleteController* GetAutocompleteController() { - OmniboxView* omnibox = - browser()->window()->GetLocationBar()->GetOmniboxView(); - return omnibox->controller()->autocomplete_controller(); + return browser() + ->window() + ->GetLocationBar() + ->GetOmniboxController() + ->autocomplete_controller(); } protected: diff --git a/browser/ai_chat/ai_chat_profiles_enabled_browsertest.cc b/browser/ai_chat/ai_chat_profiles_enabled_browsertest.cc index 951b4dd1b4a..db7a8bf03c6 100644 --- a/browser/ai_chat/ai_chat_profiles_enabled_browsertest.cc +++ b/browser/ai_chat/ai_chat_profiles_enabled_browsertest.cc @@ -122,8 +122,7 @@ IN_PROC_BROWSER_TEST_P(AIChatProfilesEnabledTest, SidebarCheck) { IN_PROC_BROWSER_TEST_P(AIChatProfilesEnabledTest, Autocomplete) { auto* autocomplete_controller = browser_->window() ->GetLocationBar() - ->GetOmniboxView() - ->controller() + ->GetOmniboxController() ->autocomplete_controller(); const auto& providers = autocomplete_controller->providers(); bool is_in_providers = diff --git a/browser/ui/views/location_bar/brave_location_bar_view.cc b/browser/ui/views/location_bar/brave_location_bar_view.cc index aac3e0f4259..8353cd6a168 100644 --- a/browser/ui/views/location_bar/brave_location_bar_view.cc +++ b/browser/ui/views/location_bar/brave_location_bar_view.cc @@ -28,6 +28,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/themes/theme_service_factory.h" #include "chrome/browser/ui/layout_constants.h" +#include "chrome/browser/ui/omnibox/omnibox_controller.h" #include "chrome/browser/ui/omnibox/omnibox_edit_model.h" #include "chrome/browser/ui/omnibox/omnibox_theme.h" #include "chrome/browser/ui/tabs/features.h" @@ -279,7 +280,8 @@ void BraveLocationBarView::RefreshBackground() { if (shadow_) { const bool show_shadow = - IsMouseHovered() && !omnibox_view_->model()->is_caret_visible(); + IsMouseHovered() && + !GetOmniboxController()->edit_model()->is_caret_visible(); shadow_->SetVisible(show_shadow); return; } diff --git a/browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc b/browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc index a1e92c41d89..547a830aa59 100644 --- a/browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc +++ b/browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc @@ -19,6 +19,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/omnibox/omnibox_controller.h" #include "chrome/browser/ui/omnibox/omnibox_edit_model.h" #include "chrome/browser/ui/omnibox/omnibox_view.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" @@ -62,6 +63,14 @@ class BraveLocationBarViewBrowserTest : public InProcessBrowserTest { OmniboxViewViews* omnibox_view() { return location_bar()->omnibox_view(); } + OmniboxController* controller() { + return location_bar()->GetOmniboxController(); + } + + OmniboxEditModel* edit_model() { + return location_bar()->GetOmniboxController()->edit_model(); + } + views::View* promotion_button_view() { return location_bar()->GetSearchPromotionButton(); } @@ -117,14 +126,14 @@ IN_PROC_BROWSER_TEST_F(BraveLocationBarViewBrowserTest, location_bar()->FocusLocation(true); omnibox_view()->SetUserText(u"a"); WaitUntil(base::BindLambdaForTesting( - [&]() { return omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return edit_model()->PopupIsOpen(); })); EXPECT_TRUE(promotion_button_view()->GetVisible()); // Unfocus from the omnibox. // Omnibox popup is hidden and promotion button will be gone also. web_contents()->Focus(); WaitUntil(base::BindLambdaForTesting( - [&]() { return !omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return !edit_model()->PopupIsOpen(); })); EXPECT_FALSE(promotion_button_view()->GetVisible()); // Set brave search as a default provider and type any input. @@ -135,12 +144,12 @@ IN_PROC_BROWSER_TEST_F(BraveLocationBarViewBrowserTest, location_bar()->FocusLocation(true); omnibox_view()->SetUserText(u"a"); WaitUntil(base::BindLambdaForTesting( - [&]() { return omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return edit_model()->PopupIsOpen(); })); EXPECT_FALSE(promotion_button_view()->GetVisible()); location_bar()->Revert(); WaitUntil(base::BindLambdaForTesting( - [&]() { return !omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return !edit_model()->PopupIsOpen(); })); EXPECT_FALSE(promotion_button_view()->GetVisible()); GetTemplateURLService()->SetUserSelectedDefaultSearchProvider( @@ -151,12 +160,12 @@ IN_PROC_BROWSER_TEST_F(BraveLocationBarViewBrowserTest, brave_search_conversion::prefs::kDismissed, true); omnibox_view()->SetUserText(u"a"); WaitUntil(base::BindLambdaForTesting( - [&]() { return omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return edit_model()->PopupIsOpen(); })); EXPECT_FALSE(promotion_button_view()->GetVisible()); location_bar()->Revert(); WaitUntil(base::BindLambdaForTesting( - [&]() { return !omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return !edit_model()->PopupIsOpen(); })); EXPECT_FALSE(promotion_button_view()->GetVisible()); constexpr std::u16string search_term = u"a"; @@ -167,7 +176,7 @@ IN_PROC_BROWSER_TEST_F(BraveLocationBarViewBrowserTest, location_bar()->FocusLocation(true); omnibox_view()->SetUserText(search_term); WaitUntil(base::BindLambdaForTesting( - [&]() { return omnibox_view()->model()->PopupIsOpen(); })); + [&]() { return edit_model()->PopupIsOpen(); })); EXPECT_TRUE(promotion_button_view()->GetVisible()); // Check brave search is set as default provider and brave search is loaded in diff --git a/browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc b/browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc index 51eed107ee5..662fd87baac 100644 --- a/browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc +++ b/browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc @@ -21,6 +21,9 @@ #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_navigator.h" #include "chrome/browser/ui/browser_navigator_params.h" +#include "chrome/browser/ui/browser_window.h" +#include "chrome/browser/ui/location_bar/location_bar.h" +#include "chrome/browser/ui/omnibox/omnibox_controller.h" #include "chrome/browser/ui/omnibox/omnibox_edit_model.h" #include "chrome/browser/ui/omnibox/omnibox_view.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" @@ -134,16 +137,20 @@ bool PromotionButtonController::ShouldShowSearchPromotionButton() { // Promotion button will be shown for current search provider's // suggestion entries to make users search with brave search with that // suggestion. - if (!omnibox_view_->model()->PopupIsOpen()) { + OmniboxEditModel* edit_model = browser_->window() + ->GetLocationBar() + ->GetOmniboxController() + ->edit_model(); + if (!edit_model->PopupIsOpen()) { return false; } // Only show promotion for search query. Not url. - if (omnibox_view_->model()->CurrentTextIsURL()) { + if (edit_model->CurrentTextIsURL()) { return false; } - const AutocompleteMatch match = omnibox_view_->model()->CurrentMatch(); + const AutocompleteMatch match = edit_model->CurrentMatch(); return !IsBraveSearchPromotionMatch(match) && #if BUILDFLAG(ENABLE_AI_CHAT) !LeoProvider::IsMatchFromLeoProvider(match) && diff --git a/browser/ui/views/omnibox/brave_omnibox_result_view.cc b/browser/ui/views/omnibox/brave_omnibox_result_view.cc index 6af4fe3f601..9cb5fe3f05a 100644 --- a/browser/ui/views/omnibox/brave_omnibox_result_view.cc +++ b/browser/ui/views/omnibox/brave_omnibox_result_view.cc @@ -102,8 +102,8 @@ void BraveOmniboxResultView::OnThemeChanged() { } void BraveOmniboxResultView::OpenMatch() { - popup_view_->model()->OpenSelection(OmniboxPopupSelection(model_index_), - base::TimeTicks::Now()); + popup_view_->controller()->edit_model()->OpenSelection( + OmniboxPopupSelection(model_index_), base::TimeTicks::Now()); } void BraveOmniboxResultView::RefreshOmniboxResult() { diff --git a/browser/ui/views/omnibox/brave_omnibox_view_views.cc b/browser/ui/views/omnibox/brave_omnibox_view_views.cc index 6ee5b5f101f..09c090c7c3c 100644 --- a/browser/ui/views/omnibox/brave_omnibox_view_views.cc +++ b/browser/ui/views/omnibox/brave_omnibox_view_views.cc @@ -54,8 +54,8 @@ std::optional BraveOmniboxViewViews::GetURLToCopy() { GURL url; bool write_url = false; auto selected_text = std::u16string(GetSelectedText()); - model()->AdjustTextForCopy(GetSelectedRange().GetMin(), &selected_text, &url, - &write_url); + controller()->edit_model()->AdjustTextForCopy( + GetSelectedRange().GetMin(), &selected_text, &url, &write_url); if (!write_url) { return std::nullopt; } @@ -179,7 +179,7 @@ BraveOmniboxViewViews::GetClipboardTextForPasteAndSearch() { } AutocompleteMatch match; - model()->ClassifyString(clipboard_text, &match, nullptr); + controller()->edit_model()->ClassifyString(clipboard_text, &match, nullptr); if (!AutocompleteMatch::IsSearchType(match.type)) { return std::nullopt; } diff --git a/browser/ui/views/omnibox/omnibox_autocomplete_browsertest.cc b/browser/ui/views/omnibox/omnibox_autocomplete_browsertest.cc index 8b1bd290bec..74df6614856 100644 --- a/browser/ui/views/omnibox/omnibox_autocomplete_browsertest.cc +++ b/browser/ui/views/omnibox/omnibox_autocomplete_browsertest.cc @@ -26,7 +26,9 @@ class OmniboxAutocompleteTest : public InProcessBrowserTest { return browser_view->toolbar()->location_bar(); } OmniboxViewViews* omnibox_view() { return location_bar()->omnibox_view(); } - OmniboxEditModel* edit_model() { return omnibox_view()->model(); } + OmniboxEditModel* edit_model() { + return location_bar()->GetOmniboxController()->edit_model(); + } OmniboxPopupView* popup_view() { return BrowserView::GetBrowserViewForBrowser(browser()) ->GetLocationBarView() @@ -36,8 +38,8 @@ class OmniboxAutocompleteTest : public InProcessBrowserTest { IN_PROC_BROWSER_TEST_F(OmniboxAutocompleteTest, AutocompleteDisabledTest) { EXPECT_FALSE(popup_view()->IsOpen()); - EXPECT_TRUE(omnibox_view() - ->controller() + EXPECT_TRUE(location_bar() + ->GetOmniboxController() ->autocomplete_controller() ->result() .empty()); @@ -50,14 +52,15 @@ IN_PROC_BROWSER_TEST_F(OmniboxAutocompleteTest, AutocompleteDisabledTest) { edit_model()->StartAutocomplete(false, false); // Check popup is opened and results are not empty. - EXPECT_FALSE(omnibox_view() - ->controller() + EXPECT_FALSE(location_bar() + ->GetOmniboxController() ->autocomplete_controller() ->result() .empty()); EXPECT_TRUE(popup_view()->IsOpen()); - omnibox_view()->controller()->StopAutocomplete(/*clear_result=*/true); + location_bar()->GetOmniboxController()->StopAutocomplete( + /*clear_result=*/true); browser()->profile()->GetPrefs()->SetBoolean(omnibox::kAutocompleteEnabled, false); @@ -65,8 +68,8 @@ IN_PROC_BROWSER_TEST_F(OmniboxAutocompleteTest, AutocompleteDisabledTest) { edit_model()->StartAutocomplete(false, false); // Check popup isn't opened and result is empty. - EXPECT_TRUE(omnibox_view() - ->controller() + EXPECT_TRUE(location_bar() + ->GetOmniboxController() ->autocomplete_controller() ->result() .empty()); diff --git a/chromium_src/chrome/browser/ui/omnibox/omnibox_popup_view.h b/chromium_src/chrome/browser/ui/omnibox/omnibox_popup_view.h new file mode 100644 index 00000000000..9dd718de621 --- /dev/null +++ b/chromium_src/chrome/browser/ui/omnibox/omnibox_popup_view.h @@ -0,0 +1,19 @@ +// Copyright (c) 2025 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_POPUP_VIEW_H_ +#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_POPUP_VIEW_H_ + +class BraveOmniboxResultView; + +#define controller_ \ + controller_; \ + friend class BraveOmniboxResultView + +#include // IWYU pragma: export + +#undef controller_ + +#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_POPUP_VIEW_H_ diff --git a/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc index 0f4eae0c612..800e39e3720 100644 --- a/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc +++ b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc @@ -7,6 +7,7 @@ #include +#include "chrome/browser/ui/omnibox/omnibox_controller.h" #include "chrome/browser/ui/omnibox/omnibox_edit_model.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h" @@ -147,6 +148,10 @@ class BraveLocationBarViewColorOverridesTest : public InProcessBrowserTest { ->GetLocationBarView() ->omnibox_view(); } + + LocationBar* GetLocationBar() { + return browser()->window()->GetLocationBar(); + } }; // We override the behavior of the LocationBar when the user is editing text. @@ -157,14 +162,26 @@ IN_PROC_BROWSER_TEST_F(BraveLocationBarViewColorOverridesTest, // LocationBar. ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( browser(), GURL("https://example.com"), 1); - EXPECT_FALSE(GetOmniboxView()->model()->is_caret_visible()); - EXPECT_FALSE(GetOmniboxView()->model()->user_input_in_progress()); + EXPECT_FALSE(GetLocationBar() + ->GetOmniboxController() + ->edit_model() + ->is_caret_visible()); + EXPECT_FALSE(GetLocationBar() + ->GetOmniboxController() + ->edit_model() + ->user_input_in_progress()); auto default_color = GetOmniboxView()->GetBackgroundColor(); // Set the user text GetOmniboxView()->SetUserText(u"hello world"); - EXPECT_FALSE(GetOmniboxView()->model()->is_caret_visible()); - EXPECT_TRUE(GetOmniboxView()->model()->user_input_in_progress()); + EXPECT_FALSE(GetLocationBar() + ->GetOmniboxController() + ->edit_model() + ->is_caret_visible()); + EXPECT_TRUE(GetLocationBar() + ->GetOmniboxController() + ->edit_model() + ->user_input_in_progress()); EXPECT_EQ(default_color, GetOmniboxView()->GetBackgroundColor()); }