Skip to content

Commit

Permalink
[omnibox] Clean up AutocompleteProvider::Type enum.
Browse files Browse the repository at this point in the history
Refactor, no behavior change.

- Reorders the values consistent with the omnibox event proto. Likewise
  reorders the switch cases in `AutocompleteProvider::TypeToString()`
  and `AutocompleteProvider::AsOmniboxEventProviderType()`.

- Removes `_PROVIDER` suffix from `TYPE_HISTORY_CLUSTER_PROVIDER` for
  consistency with the other types.

- Adds 'Type::' qualifier to the callsites that didn't already have it.

- Removes 'TYPE_' suffix from the keys.

- Renames the keys to camel case, e.g. `TYPE_HISTORY_QUICK` ->
  `kHistoryQuick`.

- Makes it a typed enum.

The reorder means their integer values are changing. That's ok, the only
places that cared for the exact values are
`OmniboxFieldTrial::GetProviderMaxMatches()` and
`OmniboxFieldTrial::GetDisabledProviderTypes()` - the 1st is used by ML
experiments (stable) & the 2nd has no experiments.

some files that have low coverage to begin with.

Low-Coverage-Reason: TRIVIAL_CHANGE This is a refactor CL that touches
Change-Id: Ic5b3cdcfdf809bca507c21bc4ecca1bfc2bc236a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4644709
Auto-Submit: manuk hovanesian <manukh@chromium.org>
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215942}
  • Loading branch information
manukh authored and Chromium LUCI CQ committed Oct 27, 2023
1 parent 70b83d9 commit e16eb57
Show file tree
Hide file tree
Showing 73 changed files with 683 additions and 503 deletions.
11 changes: 7 additions & 4 deletions chrome/browser/ash/app_list/search/omnibox/omnibox_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include "components/favicon/core/favicon_service.h"
#include "components/omnibox/browser/autocomplete_classifier.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/prefs/pref_service.h"
#include "third_party/metrics_proto/omnibox_event.pb.h"
#include "third_party/metrics_proto/omnibox_focus_type.pb.h"
Expand All @@ -44,13 +46,14 @@ bool IsAnswer(const AutocompleteMatch& match) {
match.type == AutocompleteMatchType::CALCULATOR;
}

int ProviderTypes() {
AutocompleteProviderType ProviderTypes() {
// We use all the default providers except for the document provider, which
// suggests Drive files on enterprise devices. This is disabled to avoid
// duplication with search results from DriveFS.
int providers = AutocompleteClassifier::DefaultOmniboxProviders() &
~AutocompleteProvider::TYPE_DOCUMENT;
providers |= AutocompleteProvider::TYPE_OPEN_TAB;
AutocompleteProviderType providers =
AutocompleteClassifier::DefaultOmniboxProviders() &
~AutocompleteProviderType::kDocument;
providers |= AutocompleteProviderType::kOpenTab;
return providers;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "components/omnibox/browser/autocomplete_controller.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/fake_autocomplete_provider_client.h"
#include "components/omnibox/browser/suggestion_answer.h"
#include "components/prefs/scoped_user_pref_update.h"
Expand Down Expand Up @@ -84,7 +86,7 @@ class MockAutoCompleteController : public AutocompleteController {
MockAutoCompleteController()
: AutocompleteController(
std::make_unique<FakeAutocompleteProviderClient>(),
0) {}
AutocompleteProviderType::kNone) {}
MockAutoCompleteController(const MockAutoCompleteController&) = delete;
MockAutoCompleteController& operator=(const MockAutoCompleteController&) =
delete;
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/autocomplete/search_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/history_url_provider.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/remote_suggestions_service.h"
Expand Down Expand Up @@ -1081,7 +1082,7 @@ TEST_F(SearchProviderTest, KeywordOrderingAndDescriptions) {
AutocompleteController controller(
std::make_unique<TestAutocompleteProviderClient>(
profile_.get(), &test_url_loader_factory_),
AutocompleteProvider::TYPE_SEARCH);
AutocompleteProviderType::kSearch);
AutocompleteInput input(u"k t", metrics::OmniboxEventProto::OTHER,
ChromeAutocompleteSchemeClassifier(profile_.get()));
controller.Start(input);
Expand Down
10 changes: 6 additions & 4 deletions chrome/browser/chromeos/launcher_search/search_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "components/omnibox/browser/autocomplete_controller.h"
#include "components/omnibox/browser/autocomplete_match_type.h"
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/favicon_cache.h"
#include "components/omnibox/browser/suggestion_answer.h"
#include "components/search_engines/search_terms_data.h"
Expand Down Expand Up @@ -187,16 +188,17 @@ SearchResultPtr CreateBaseResult(const AutocompleteMatch& match,

} // namespace

int ProviderTypes() {
AutocompleteProviderType ProviderTypes() {
// We use all the default providers except for the document provider, which
// suggests Drive files on enterprise devices. This is disabled to avoid
// duplication with search results from DriveFS.
int providers = AutocompleteClassifier::DefaultOmniboxProviders() &
~AutocompleteProvider::TYPE_DOCUMENT;
AutocompleteProviderType providers =
AutocompleteClassifier::DefaultOmniboxProviders() &
~AutocompleteProviderType::kDocument;

// The open tab provider is not included in the default providers, so add it
// in manually.
providers |= AutocompleteProvider::TYPE_OPEN_TAB;
providers |= AutocompleteProviderType::kOpenTab;

return providers;
}
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/chromeos/launcher_search/search_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "chromeos/crosapi/mojom/launcher_search.mojom.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"

class AutocompleteController;
class FaviconCache;
Expand All @@ -21,9 +23,9 @@ class BookmarkModel;

namespace crosapi {

// Returns a bitmask of the AutocompleteProvider types to be used by Launcher
// Returns a bitmask of the `AutocompleteProvider` types to be used by Launcher
// search.
int ProviderTypes();
AutocompleteProviderType ProviderTypes();

// Returns the UI page transition that corresponds to the given crosapi page
// transition.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "components/omnibox/browser/autocomplete_classifier.h"
#include "components/omnibox/browser/autocomplete_match_type.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/suggestion_answer.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
Expand All @@ -20,9 +21,9 @@ namespace crosapi {
namespace {

TEST(SearchUtilTest, ProviderTypes) {
const int types = ProviderTypes();
EXPECT_FALSE(types & AutocompleteProvider::TYPE_DOCUMENT);
EXPECT_TRUE(types & AutocompleteProvider::TYPE_OPEN_TAB);
const AutocompleteProviderType types = ProviderTypes();
EXPECT_FALSE(!!(types & AutocompleteProviderType::kDocument));
EXPECT_TRUE(!!(types & AutocompleteProviderType::kOpenTab));
}

// Tests result conversion for a default answer result.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "components/omnibox/browser/autocomplete_controller.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/autocomplete_result.h"
#include "components/omnibox/browser/omnibox_controller.h"
#include "components/omnibox/browser/omnibox_edit_model.h"
Expand Down Expand Up @@ -222,14 +223,14 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, SendSuggestions) {
EXPECT_EQ(u"alpha input", result.match_at(0).fill_into_edit);
EXPECT_EQ(AutocompleteMatchType::SEARCH_OTHER_ENGINE,
result.match_at(0).type);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(0).provider->type());

// First suggestion, complete with rich description.
{
EXPECT_EQ(u"alpha", result.match_at(1).keyword);
EXPECT_EQ(u"alpha input first", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());

std::u16string rich_description =
Expand All @@ -255,14 +256,14 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, SendSuggestions) {

EXPECT_EQ(u"alpha", result.match_at(2).keyword);
EXPECT_EQ(u"alpha input second", result.match_at(2).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(2).provider->type());
EXPECT_EQ(simple_description, result.match_at(2).contents);
VerifyMatchComponents(expected_components, result.match_at(2));

EXPECT_EQ(u"alpha", result.match_at(3).keyword);
EXPECT_EQ(u"alpha input third", result.match_at(3).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(3).provider->type());
EXPECT_EQ(simple_description, result.match_at(3).contents);
VerifyMatchComponents(expected_components, result.match_at(3));
Expand All @@ -271,7 +272,7 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, SendSuggestions) {
// Final option, search what you typed.
AutocompleteMatch match = result.match_at(4);
EXPECT_EQ(AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, match.type);
EXPECT_EQ(AutocompleteProvider::TYPE_SEARCH,
EXPECT_EQ(AutocompleteProviderType::kSearch,
result.match_at(4).provider->type());
EXPECT_FALSE(match.deletable);
}
Expand Down Expand Up @@ -405,7 +406,7 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, IncognitoSplitMode) {
// Second result: incognito-specific.
EXPECT_EQ(u"alpha", result.match_at(1).keyword);
EXPECT_EQ(u"alpha input incognito", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());

// Third result: search what you typed.
Expand Down Expand Up @@ -546,22 +547,22 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, MAYBE_DeleteOmniboxSuggestionResult) {
ASSERT_EQ(5u, result.size()) << AutocompleteResultAsString(result);

EXPECT_EQ(u"alpha input", result.match_at(0).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(0).provider->type());
EXPECT_FALSE(result.match_at(0).deletable);

EXPECT_EQ(u"alpha input first", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());
EXPECT_FALSE(result.match_at(1).deletable);

EXPECT_EQ(u"alpha input second", result.match_at(2).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(2).provider->type());
EXPECT_TRUE(result.match_at(2).deletable);

EXPECT_EQ(u"alpha input third", result.match_at(3).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(3).provider->type());
EXPECT_FALSE(result.match_at(3).deletable);

Expand Down Expand Up @@ -656,15 +657,15 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest,
ASSERT_EQ(4U, result.size()) << AutocompleteResultAsString(result);

EXPECT_EQ(u"kw d", result.match_at(0).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(0).provider->type());

EXPECT_EQ(u"kw d first", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());

EXPECT_EQ(u"kw d second", result.match_at(2).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(2).provider->type());

EXPECT_EQ(u"kw d", result.match_at(3).fill_into_edit);
Expand Down Expand Up @@ -700,7 +701,7 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest,
result.match_at(0).type);

EXPECT_EQ(u"kw d", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());
}
}
Expand Down Expand Up @@ -814,7 +815,7 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, MAYBE_SetDefaultSuggestion) {
{
const AutocompleteMatch& match = result.match_at(0);
EXPECT_EQ(AutocompleteMatchType::SEARCH_OTHER_ENGINE, match.type);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD, match.provider->type());
EXPECT_EQ(AutocompleteProviderType::kKeyword, match.provider->type());

// The "description" given by the extension is shown as the "contents" in
// the AutocompleteMatch. The XML-marked string is
Expand Down Expand Up @@ -883,16 +884,16 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, PassEmptySuggestions) {
ASSERT_EQ(3u, result.size()) << AutocompleteResultAsString(result);

EXPECT_EQ(u"alpha d", result.match_at(0).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(0).provider->type());

EXPECT_EQ(u"alpha foo", result.match_at(1).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(1).provider->type());

AutocompleteMatch match = result.match_at(2);
EXPECT_EQ(AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, match.type);
EXPECT_EQ(AutocompleteProvider::TYPE_SEARCH,
EXPECT_EQ(AutocompleteProviderType::kSearch,
result.match_at(2).provider->type());
}

Expand All @@ -911,12 +912,12 @@ IN_PROC_BROWSER_TEST_P(OmniboxApiTest, PassEmptySuggestions) {
ASSERT_EQ(2u, result.size()) << AutocompleteResultAsString(result);

EXPECT_EQ(u"alpha ", result.match_at(0).fill_into_edit);
EXPECT_EQ(AutocompleteProvider::TYPE_KEYWORD,
EXPECT_EQ(AutocompleteProviderType::kKeyword,
result.match_at(0).provider->type());

AutocompleteMatch match = result.match_at(1);
EXPECT_EQ(AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, match.type);
EXPECT_EQ(AutocompleteProvider::TYPE_SEARCH,
EXPECT_EQ(AutocompleteProviderType::kSearch,
result.match_at(1).provider->type());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/omnibox/browser/autocomplete_controller.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
#include "components/omnibox/browser/omnibox_controller.h"
#include "components/omnibox/browser/omnibox_view.h"
Expand All @@ -31,7 +33,7 @@ class MockAutocompleteController : public AutocompleteController {
public:
MockAutocompleteController(
std::unique_ptr<AutocompleteProviderClient> provider_client,
int provider_types)
AutocompleteProviderType provider_types)
: AutocompleteController(std::move(provider_client), provider_types) {}
~MockAutocompleteController() override = default;
MockAutocompleteController(const MockAutocompleteController&) = delete;
Expand Down Expand Up @@ -60,7 +62,7 @@ class ZeroSuggestPrefetchTabHelperBrowserTest : public InProcessBrowserTest {

auto controller =
std::make_unique<testing::NiceMock<MockAutocompleteController>>(
std::move(client_), 0);
std::move(client_), AutocompleteProviderType::kNone);
controller_ = controller.get();
browser()
->window()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "chrome/common/chrome_features.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/omnibox/browser/actions/tab_switch_action.h"
#include "components/omnibox/browser/autocomplete_provider_type.h"
#include "components/omnibox/browser/fake_autocomplete_provider.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/omnibox_popup_selection.h"
Expand Down Expand Up @@ -547,7 +548,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupViewViewsTest,

IN_PROC_BROWSER_TEST_F(OmniboxPopupViewViewsTest, DeleteSuggestion) {
scoped_refptr<FakeAutocompleteProvider> provider =
new FakeAutocompleteProvider(AutocompleteProvider::TYPE_SEARCH);
new FakeAutocompleteProvider(AutocompleteProviderType::kSearch);
controller()->autocomplete_controller()->providers_.push_back(provider);

ACMatches matches;
Expand Down
Binary file not shown.
12 changes: 12 additions & 0 deletions components/omnibox/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,17 @@ static_library("vector_icons") {
]
}

static_library("autocomplete_provider_type") {
sources = [
"autocomplete_provider_type.cc",
"autocomplete_provider_type.h",
]
deps = [
"//base",
"//third_party/metrics_proto",
]
}

static_library("browser") {
sources = [
"actions/omnibox_action.cc",
Expand Down Expand Up @@ -311,6 +322,7 @@ static_library("browser") {
]

public_deps = [
":autocomplete_provider_type",
":buildflags",
":location_bar",
":mojo_bindings",
Expand Down

0 comments on commit e16eb57

Please sign in to comment.