[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 <mahmadi@chromium.org> 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 <mahmadi@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/main@{#1539750}
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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 =
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
+10
-3
@@ -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) &&
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -54,8 +54,8 @@ std::optional<GURL> 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;
|
||||
}
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -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 <chrome/browser/ui/omnibox/omnibox_popup_view.h> // IWYU pragma: export
|
||||
|
||||
#undef controller_
|
||||
|
||||
#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_OMNIBOX_OMNIBOX_POPUP_VIEW_H_
|
||||
@@ -7,6 +7,7 @@
|
||||
|
||||
#include <optional>
|
||||
|
||||
#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());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user