Skip to content

Commit

Permalink
Revert "[omnibox] Split WebUI implementation out from OmniboxPopupVie…
Browse files Browse the repository at this point in the history 
…wViews"

This reverts commit e3cd8b9.

Reason for revert:
The newly added tests have been failing on several different lsan/msan/asan bots, and I was able to reproduce it locally. Here is an example: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-lacros-asan-lsan-rel/5245/overview

Original change's description:
> [omnibox] Split WebUI implementation out from OmniboxPopupViewViews
>
> This CL refactors OmniboxPopupViewViews to make OmniboxPopupViewWebUI
> a fully separate subclass, eliminating all reference to "webui" except
> for some safety checks which can be eliminated after the new class
> is fully settled.
>
> In addition, this CL takes a few opportunities for code cleanup and
> simplification. For example, method `GetSelectedResultView` was removed
> because it had only one call site so inlining was cleaner than creating
> a separate implementation to return nullptr in OmniboxPopupViewWebUI.
>
> Bug: 1445142
> Change-Id: I6d37efc8b21ea36e6df3f897a1f732bcbea2d476
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4602452
> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
> Commit-Queue: Orin Jaworski <orinj@chromium.org>
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1157941}

Bug: 1445142
Change-Id: I54e8cbad1821c5b7994c858223fed7c4b2149ecd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4618013
Reviewed-by: Joey Arhar <jarhar@google.com>
Commit-Queue: Joey Arhar <jarhar@google.com>
Owners-Override: Joey Arhar <jarhar@google.com>
Cr-Commit-Position: refs/heads/main@{#1158275}
  • Loading branch information
@josepharhar
josepharhar authored and Chromium LUCI CQ committed Jun 15, 2023
1 parent 0dd94b9 commit 7d4be95
Show file tree
Hide file tree
Showing 10 changed files with 157 additions and 509 deletions.
2 changes: 0 additions & 2 deletions chrome/browser/ui/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5027,8 +5027,6 @@ static_library("ui") {
"views/omnibox/omnibox_mouse_enter_exit_handler.h",
"views/omnibox/omnibox_popup_view_views.cc",
"views/omnibox/omnibox_popup_view_views.h",
"views/omnibox/omnibox_popup_view_webui.cc",
"views/omnibox/omnibox_popup_view_webui.h",
"views/omnibox/omnibox_result_view.cc",
"views/omnibox/omnibox_result_view.h",
"views/omnibox/omnibox_row_view.cc",
Expand Down
248 changes: 135 additions & 113 deletions chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/ui/views/omnibox/omnibox_row_view.h"
#include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
#include "chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.h"
#include "chrome/browser/ui/views/omnibox/webui_omnibox_popup_view.h"
#include "chrome/browser/ui/views/theme_copying_widget.h"
#include "chrome/browser/ui/views/user_education/browser_feature_promo_controller.h"
#include "components/omnibox/browser/omnibox_edit_model.h"
Expand Down Expand Up @@ -215,6 +216,37 @@ OmniboxPopupSelection OmniboxPopupViewViews::GetSelection() const {
return edit_model_->GetPopupSelection();
}

OmniboxResultView* OmniboxPopupViewViews::result_view_at(size_t i) {
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup))
<< "With the WebUI omnibox popup enabled, the code should not try to "
"fetch the child result view.";

// TODO(tommycli): https://crbug.com/1063071
// Making this method public was a mistake. Outside callers have no idea about
// our internal state, and there's now a crash in this area. For now, let's
// return nullptr, but the ultimate fix is orinj's OmniboxPopupModel refactor.
if (i >= children().size()) {
return nullptr;
}

return static_cast<OmniboxRowView*>(children()[i])->result_view();
}

OmniboxResultView* OmniboxPopupViewViews::GetSelectedResultView() {
// Do not return the native result view if the WebUI omnibox popup is enabled.
// TODO(crbug.com/1396174): Ideally outside callers should not try to access
// child views, but rather should interact with the OmniboxPopupModel instead.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
return nullptr;
}

size_t selected_line = GetSelection().line;
if (selected_line == OmniboxPopupSelection::kNoMatch) {
return nullptr;
}
return result_view_at(selected_line);
}

bool OmniboxPopupViewViews::IsOpen() const {
return popup_ != nullptr;
}
Expand All @@ -232,8 +264,10 @@ void OmniboxPopupViewViews::InvalidateLine(size_t line) {
void OmniboxPopupViewViews::OnSelectionChanged(
OmniboxPopupSelection old_selection,
OmniboxPopupSelection new_selection) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
webui_view_->OnSelectedLineChanged(old_selection.line, new_selection.line);
return;
}

// Do not invalidate the same line twice, in order to avoid redundant
// accessibility events.
Expand Down Expand Up @@ -301,14 +335,83 @@ void OmniboxPopupViewViews::UpdatePopupAppearance() {
popup_created = true;
}

UpdateChildViews();
// Update the match cached by each row, in the process of doing so make sure
// we have enough row views.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
if (!webui_view_) {
webui_view_ = AddChildView(std::make_unique<WebUIOmniboxPopupView>(
location_bar_view_->profile()));
}
} else {
const size_t result_size = edit_model_->result().size();
std::u16string previous_row_header = u"";
PrefService* const pref_service = GetPrefService();
for (size_t i = 0; i < result_size; ++i) {
// Create child views lazily. Since especially the first result view may
// be expensive to create due to loading font data, this saves time and
// memory during browser startup. https://crbug.com/1021323
if (children().size() == i) {
AddChildView(std::make_unique<OmniboxRowView>(
i, edit_model_,
std::make_unique<OmniboxResultView>(this, edit_model_, i),
pref_service));
}

OmniboxRowView* const row_view =
static_cast<OmniboxRowView*>(children()[i]);
row_view->SetVisible(true);

// Show the header if it's distinct from the previous match's header.
const AutocompleteMatch& match = GetMatchAtIndex(i);
std::u16string current_row_header =
match.suggestion_group_id.has_value()
? edit_model_->result().GetHeaderForSuggestionGroup(
match.suggestion_group_id.value())
: u"";
if (!current_row_header.empty() &&
current_row_header != previous_row_header) {
row_view->ShowHeader(match.suggestion_group_id.value(),
current_row_header);
} else {
row_view->HideHeader();
}
previous_row_header = current_row_header;

OmniboxResultView* const result_view = row_view->result_view();
result_view->SetMatch(match);

// Set visibility of the result view based on whether the group is hidden.
bool match_hidden = pref_service &&
match.suggestion_group_id.has_value() &&
edit_model_->result().IsSuggestionGroupHidden(
pref_service, match.suggestion_group_id.value());
result_view->SetVisible(!match_hidden);

const SkBitmap* bitmap = edit_model_->GetPopupRichSuggestionBitmap(i);
if (bitmap) {
result_view->SetRichSuggestionImage(
gfx::ImageSkia::CreateFrom1xBitmap(*bitmap));
}
}
// If we have more views than matches, hide the surplus ones.
for (auto i = children().begin() + result_size; i != children().end();
++i) {
(*i)->SetVisible(false);
}
}

popup_->SetTargetBounds(GetTargetBounds());

if (popup_created) {
popup_->ShowAnimated();

OnPopupCreated();
// Popup is now expanded and first item will be selected.
NotifyAccessibilityEvent(ax::mojom::Event::kExpandedChanged, true);
if (!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
if (OmniboxResultView* result_view = result_view_at(0)) {
FireAXEventsForNewActiveDescendant(result_view);
}
}

#if BUILDFLAG(IS_MAC)
// It's not great for promos to overlap the omnibox if the user opens the
Expand All @@ -326,8 +429,10 @@ void OmniboxPopupViewViews::UpdatePopupAppearance() {
}

void OmniboxPopupViewViews::ProvideButtonFocusHint(size_t line) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));
// TODO(crbug.com/1396174): Not implemented for WebUI omnibox popup yet.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
return;
}

DCHECK(GetSelection().IsButtonFocused());

Expand All @@ -341,8 +446,10 @@ void OmniboxPopupViewViews::ProvideButtonFocusHint(size_t line) {
}

void OmniboxPopupViewViews::OnMatchIconUpdated(size_t match_index) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));
// TODO(crbug.com/1396174): Not implemented for WebUI omnibox popup yet.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
return;
}

if (OmniboxResultView* result_view = result_view_at(match_index)) {
result_view->OnMatchIconUpdated();
Expand All @@ -360,9 +467,6 @@ void OmniboxPopupViewViews::GetPopupAccessibleNodeData(

void OmniboxPopupViewViews::AddPopupAccessibleNodeData(
ui::AXNodeData* node_data) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));

// Establish a "CONTROLS" relationship between the omnibox and the
// the popup. This allows a screen reader to understand the relationship
// between the omnibox and the list of suggestions, and determine which
Expand All @@ -371,13 +475,10 @@ void OmniboxPopupViewViews::AddPopupAccessibleNodeData(
int32_t popup_view_id = GetViewAccessibility().GetUniqueId().Get();
node_data->AddIntListAttribute(ax::mojom::IntListAttribute::kControlsIds,
{popup_view_id});
size_t selected_line = GetSelection().line;
if (selected_line != OmniboxPopupSelection::kNoMatch) {
if (OmniboxResultView* result_view = result_view_at(selected_line)) {
node_data->AddIntAttribute(
ax::mojom::IntAttribute::kActivedescendantId,
result_view->GetViewAccessibility().GetUniqueId().Get());
}
if (OmniboxResultView* selected_result_view = GetSelectedResultView()) {
node_data->AddIntAttribute(
ax::mojom::IntAttribute::kActivedescendantId,
selected_result_view->GetViewAccessibility().GetUniqueId().Get());
}
}

Expand All @@ -393,8 +494,10 @@ std::u16string OmniboxPopupViewViews::GetAccessibleButtonTextForResult(
}

bool OmniboxPopupViewViews::OnMouseDragged(const ui::MouseEvent& event) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));
// TODO(crbug.com/1396174): Not implemented for WebUI omnibox popup yet.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
return true;
}

size_t index = GetIndexForPoint(event.location());

Expand Down Expand Up @@ -469,91 +572,21 @@ void OmniboxPopupViewViews::OnWidgetBoundsChanged(views::Widget* widget,
UpdatePopupAppearance();
}

void OmniboxPopupViewViews::UpdateChildViews() {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));

// Update the match cached by each row, in the process of doing so make sure
// we have enough row views.
const size_t result_size = edit_model_->result().size();
std::u16string previous_row_header = u"";
PrefService* const pref_service = GetPrefService();
for (size_t i = 0; i < result_size; ++i) {
// Create child views lazily. Since especially the first result view may
// be expensive to create due to loading font data, this saves time and
// memory during browser startup. https://crbug.com/1021323
if (children().size() == i) {
AddChildView(std::make_unique<OmniboxRowView>(
i, edit_model_,
std::make_unique<OmniboxResultView>(this, edit_model_, i),
pref_service));
}

OmniboxRowView* const row_view =
static_cast<OmniboxRowView*>(children()[i]);
row_view->SetVisible(true);

// Show the header if it's distinct from the previous match's header.
const AutocompleteMatch& match = GetMatchAtIndex(i);
std::u16string current_row_header =
match.suggestion_group_id.has_value()
? edit_model_->result().GetHeaderForSuggestionGroup(
match.suggestion_group_id.value())
: u"";
if (!current_row_header.empty() &&
current_row_header != previous_row_header) {
row_view->ShowHeader(match.suggestion_group_id.value(),
current_row_header);
} else {
row_view->HideHeader();
}
previous_row_header = current_row_header;

OmniboxResultView* const result_view = row_view->result_view();
result_view->SetMatch(match);

// Set visibility of the result view based on whether the group is hidden.
bool match_hidden = pref_service && match.suggestion_group_id.has_value() &&
edit_model_->result().IsSuggestionGroupHidden(
pref_service, match.suggestion_group_id.value());
result_view->SetVisible(!match_hidden);

const SkBitmap* bitmap = edit_model_->GetPopupRichSuggestionBitmap(i);
if (bitmap) {
result_view->SetRichSuggestionImage(
gfx::ImageSkia::CreateFrom1xBitmap(*bitmap));
}
}
// If we have more views than matches, hide the surplus ones.
for (auto i = children().begin() + result_size; i != children().end(); ++i) {
(*i)->SetVisible(false);
}
}

void OmniboxPopupViewViews::OnPopupCreated() {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));

// Popup is now expanded and first item will be selected.
NotifyAccessibilityEvent(ax::mojom::Event::kExpandedChanged, true);
OmniboxResultView* result_view = result_view_at(0);
if (result_view) {
FireAXEventsForNewActiveDescendant(result_view);
}
}

gfx::Rect OmniboxPopupViewViews::GetTargetBounds() const {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));

int popup_height = 0;

DCHECK_GE(children().size(), edit_model_->result().size());
popup_height = std::accumulate(
children().cbegin(), children().cbegin() + edit_model_->result().size(),
0, [](int height, const auto* v) {
return height + v->GetPreferredSize().height();
});
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup)) {
if (webui_view_) {
popup_height = webui_view_->GetPreferredSize().height();
}
} else {
DCHECK_GE(children().size(), edit_model_->result().size());
popup_height = std::accumulate(
children().cbegin(), children().cbegin() + edit_model_->result().size(),
0, [](int height, const auto* v) {
return height + v->GetPreferredSize().height();
});
}

// Add enough space on the top and bottom so it looks like there is the same
// amount of space between the text and the popup border as there is in the
Expand All @@ -576,17 +609,6 @@ gfx::Rect OmniboxPopupViewViews::GetTargetBounds() const {
return content_rect;
}

OmniboxResultView* OmniboxPopupViewViews::result_view_at(size_t i) {
// TODO(crbug.com/1445142): Remove this DCHECK after refactor is complete.
DCHECK(!base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup));

if (i >= children().size()) {
return nullptr;
}

return static_cast<OmniboxRowView*>(children()[i])->result_view();
}

bool OmniboxPopupViewViews::HasMatchAt(size_t index) const {
return index < edit_model_->result().size();
}
Expand Down

0 comments on commit 7d4be95

Please sign in to comment.