Skip to content

Commit

Permalink
[omnibox] Remove the kSearchProviderWarmUpOnFocus Feature.
Browse files Browse the repository at this point in the history 
This feature was default enabled in 2017 (https://crrev.com/c/576615)
and has not been disabled by any experiment config since M70
(see http://source/search?q=OmniboxWarmUpSearchProviderOnFocus).

Change-Id: I081fac774145de90474492088b9aad7015d8d2ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3031805
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#902061}
  • Loading branch information
@justindonnelly
justindonnelly authored and Chromium LUCI CQ committed Jul 15, 2021
1 parent 615f0aa commit 02252b6
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 83 deletions.
87 changes: 22 additions & 65 deletions chrome/browser/autocomplete/search_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ class TestAutocompleteProviderClient : public ChromeAutocompleteProviderClient {

class SearchProviderFeatureTestComponent {
public:
SearchProviderFeatureTestComponent(
const absl::optional<bool> warm_up_on_focus,
explicit SearchProviderFeatureTestComponent(
const bool command_line_overrides);

~SearchProviderFeatureTestComponent() {
Expand All @@ -155,16 +154,7 @@ class SearchProviderFeatureTestComponent {
};

SearchProviderFeatureTestComponent::SearchProviderFeatureTestComponent(
const absl::optional<bool> warm_up_on_focus,
const bool command_line_overrides) {
if (warm_up_on_focus.has_value()) {
if (warm_up_on_focus.value())
feature_list_.InitAndEnableFeature(omnibox::kSearchProviderWarmUpOnFocus);
else
feature_list_.InitAndDisableFeature(
omnibox::kSearchProviderWarmUpOnFocus);
}

if (command_line_overrides) {
base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
switches::kGoogleBaseURL, "http://www.bar.com/");
Expand Down Expand Up @@ -220,10 +210,8 @@ class BaseSearchProviderTest : public testing::Test,
bool allowed_to_be_default_match;
};

BaseSearchProviderTest(
const absl::optional<bool> warm_up_on_focus = absl::nullopt,
const bool command_line_overrides = false)
: feature_test_component_(warm_up_on_focus, command_line_overrides) {
explicit BaseSearchProviderTest(const bool command_line_overrides = false)
: feature_test_component_(command_line_overrides) {
// We need both the history service and template url model loaded.
TestingProfile::Builder profile_builder;
profile_builder.AddTestingFactory(
Expand Down Expand Up @@ -343,10 +331,8 @@ class BaseSearchProviderTest : public testing::Test,
// Test environment with valid suggest and search URL.
class SearchProviderTest : public BaseSearchProviderTest {
public:
SearchProviderTest(
const absl::optional<bool> warm_up_on_focus = absl::nullopt,
const bool command_line_overrides = false)
: BaseSearchProviderTest(warm_up_on_focus, command_line_overrides) {}
explicit SearchProviderTest(const bool command_line_overrides = false)
: BaseSearchProviderTest(command_line_overrides) {}

void SetUp() override {
CustomizableSetUp(
Expand Down Expand Up @@ -3727,65 +3713,36 @@ TEST_F(InvalidSearchProviderTest, DoesNotSendSuggestRequests) {
EXPECT_FALSE(test_url_loader_factory_.IsPending("http://defaulturl/query"));
}

// SearchProviderWarmUpTest --------------------------------------------------
//
// Like SearchProviderTest. The only addition is that it's a
// TestWithParam<bool>, where the boolean parameter represents whether the
// omnibox::kSearchProviderWarmUpOnFocus feature flag should be enabled.
class SearchProviderWarmUpTest : public SearchProviderTest,
public testing::WithParamInterface<bool> {
public:
SearchProviderWarmUpTest() : SearchProviderTest(GetParam(), false) {}

SearchProviderWarmUpTest(SearchProviderWarmUpTest const&) = delete;
SearchProviderWarmUpTest& operator=(SearchProviderWarmUpTest const&) = delete;
};

TEST_P(SearchProviderWarmUpTest, SendsWarmUpRequestOnFocus) {
TEST_F(SearchProviderTest, SendsWarmUpRequestOnFocus) {
AutocompleteInput input(u"f", metrics::OmniboxEventProto::OTHER,
ChromeAutocompleteSchemeClassifier(profile_.get()));
input.set_prefer_keyword(true);
input.set_focus_type(OmniboxFocusType::ON_FOCUS);

if (!GetParam()) { // The warm-up feature ought to be disabled.
// The provider immediately terminates with no matches.
provider_->Start(input, false);
// RunUntilIdle so that SearchProvider has a chance to create the
// URLFetchers (if it wants to, which it shouldn't in this case).
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(provider_->done());
EXPECT_TRUE(provider_->matches().empty());
} else { // The warm-up feature ought to be enabled.
provider_->Start(input, false);
// RunUntilIdle so that SearchProvider create the URLFetcher.
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(provider_->done());
EXPECT_TRUE(provider_->matches().empty());
// Make sure the default provider's suggest service was queried with an
// empty query.
EXPECT_TRUE(test_url_loader_factory_.IsPending("http://defaultturl2/"));
// Even if the fetcher returns results, we should still have no suggestions
// (though the provider should now be done).
test_url_loader_factory_.AddResponse("http://defaultturl2/",
R"(["",["a", "b"],[],[],{}])");
RunTillProviderDone();
EXPECT_TRUE(provider_->done());
EXPECT_TRUE(provider_->matches().empty());
}
provider_->Start(input, false);
// RunUntilIdle so that SearchProvider create the URLFetcher.
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(provider_->done());
EXPECT_TRUE(provider_->matches().empty());
// Make sure the default provider's suggest service was queried with an
// empty query.
EXPECT_TRUE(test_url_loader_factory_.IsPending("http://defaultturl2/"));
// Even if the fetcher returns results, we should still have no suggestions
// (though the provider should now be done).
test_url_loader_factory_.AddResponse("http://defaultturl2/",
R"(["",["a", "b"],[],[],{}])");
RunTillProviderDone();
EXPECT_TRUE(provider_->done());
EXPECT_TRUE(provider_->matches().empty());
}

INSTANTIATE_TEST_CASE_P(SearchProviderTest,
SearchProviderWarmUpTest,
testing::Values(false, true));

// SearchProviderCommandLineOverrideTest -------------------------------------
//
// Like SearchProviderTest. The only addition is that it sets additional
// command line flags in SearchProviderFeatureTestComponent.
class SearchProviderCommandLineOverrideTest : public SearchProviderTest {
public:
SearchProviderCommandLineOverrideTest()
: SearchProviderTest(absl::nullopt, true) {}
SearchProviderCommandLineOverrideTest() : SearchProviderTest(true) {}

SearchProviderCommandLineOverrideTest(
SearchProviderCommandLineOverrideTest const&) = delete;
Expand Down
19 changes: 11 additions & 8 deletions components/omnibox/browser/search_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,14 +233,17 @@ void SearchProvider::Start(const AutocompleteInput& input,
matches_.clear();
set_field_trial_triggered(false);

// Unless warming up the suggest server on focus, SearchProvider doesn't do
// do anything useful for on-focus inputs or empty inputs. Exit early.
if (!base::FeatureList::IsEnabled(omnibox::kSearchProviderWarmUpOnFocus) &&
(input.focus_type() != OmniboxFocusType::DEFAULT ||
input.type() == metrics::OmniboxInputType::EMPTY)) {
Stop(true, false);
return;
}
// At this point, we could exit early if the input is on-focus or empty,
// because offering suggestions in those scenarios is handled by
// ZeroSuggestProvider. But we continue here anyway in order to send a request
// to warm up the suggest server. It's possible this warmup request could be
// combined or deduped with the request from ZeroSuggestProvider but that
// provider doesn't always run, based on a variety of factors (sign in state,
// experiments, input type (on-focus vs. on-clobber)). Ensuring that we always
// send a request here allows the suggest server to, for example, load
// per-user models into memory. Having a per-user model in memory allows the
// suggest server to respond more quickly with personalized suggestions as the
// user types.

keyword_input_ = input;
const TemplateURL* keyword_provider =
Expand Down
2 changes: 1 addition & 1 deletion components/omnibox/browser/search_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,13 @@ class SearchProvider : public BaseSearchProvider,
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, AnswersCache);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, RemoveExtraAnswers);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoesNotProvideOnFocus);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SendsWarmUpRequestOnFocus);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpScheme);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
DontTrimHttpSchemeIfInputHasScheme);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
DontTrimHttpsSchemeIfInputHasScheme);
FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpsScheme);
FRIEND_TEST_ALL_PREFIXES(SearchProviderWarmUpTest, SendsWarmUpRequestOnFocus);

// Manages the providers (TemplateURLs) used by SearchProvider. Two providers
// may be used:
Expand Down
8 changes: 0 additions & 8 deletions components/omnibox/common/omnibox_features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,6 @@ const base::Feature kExperimentalKeywordMode{"OmniboxExperimentalKeywordMode",
const base::Feature kImageSearchSuggestionThumbnail{
"ImageSearchSuggestionThumbnail", enabled_by_default_android_only};

// Feature to enable the search provider to send a request to the suggest
// server on focus. This allows the suggest server to warm up, by, for
// example, loading per-user models into memory. Having a per-user model
// in memory allows the suggest server to respond more quickly with
// personalized suggestions as the user types.
const base::Feature kSearchProviderWarmUpOnFocus{
"OmniboxWarmUpSearchProviderOnFocus", enabled_by_default_desktop_android};

// Feature used to display the title of the current URL match.
const base::Feature kDisplayTitleForCurrentUrl{
"OmniboxDisplayTitleForCurrentUrl", base::FEATURE_ENABLED_BY_DEFAULT};
Expand Down
1 change: 0 additions & 1 deletion components/omnibox/common/omnibox_features.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ namespace omnibox {
extern const base::Feature kOmniboxTabSwitchSuggestions;
extern const base::Feature kExperimentalKeywordMode;
extern const base::Feature kImageSearchSuggestionThumbnail;
extern const base::Feature kSearchProviderWarmUpOnFocus;
extern const base::Feature kDisplayTitleForCurrentUrl;
extern const base::Feature kOmniboxRemoveSuggestionsFromClipboard;

Expand Down

0 comments on commit 02252b6

Please sign in to comment.