Commit 23d1f87d authored by Tommy Li's avatar Tommy Li Committed by Commit Bot

[omnibox] Delete ZeroSuggestVariant

We are no longer using ZeroSuggestVariant.

Instead, we are moving to using separate base::Feature flags for each
experiment.

See here for a discussion about removing this:
http://g/chrome-omnibox-team/lNT8EsY1gFI

Bug: 1124050
Change-Id: I20fb3d79ad2ab64fb7daf52729449cbad3193e75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2430227
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: default avatarMoe Ahmadi <mahmadi@chromium.org>
Reviewed-by: default avatarJustin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813359}
parent 1670d086
......@@ -87,11 +87,7 @@ bool IsRealboxEnabled() {
return base::FeatureList::IsEnabled(kRealbox) ||
base::FeatureList::IsEnabled(omnibox::kZeroSuggestionsOnNTPRealbox) ||
base::FeatureList::IsEnabled(
omnibox::kReactiveZeroSuggestionsOnNTPRealbox) ||
(base::FeatureList::IsEnabled(omnibox::kOnFocusSuggestions) &&
!OmniboxFieldTrial::GetZeroSuggestVariants(
metrics::OmniboxEventProto::NTP_REALBOX)
.empty());
omnibox::kReactiveZeroSuggestionsOnNTPRealbox);
}
} // namespace ntp_features
......@@ -105,13 +105,9 @@ bool SearchSuggestService::IsEnabled() {
if (base::FeatureList::IsEnabled(ntp_features::kSearchSuggestChips))
return true;
return OmniboxFieldTrial::GetZeroSuggestVariants(
metrics::OmniboxEventProto::NTP_REALBOX)
.empty() &&
OmniboxFieldTrial::GetZeroSuggestVariants(
metrics::OmniboxEventProto::
INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS)
.empty();
// NTP ZeroSuggest is enabled by default, so therefore this is disabled by
// default.
return false;
}
SearchSuggestService::SearchSuggestService(
......
......@@ -144,100 +144,32 @@ class SearchSuggestServiceTest : public BrowserWithTestWindowTest {
};
TEST_F(SearchSuggestServiceTest, IsEnabled) {
constexpr char kRemoteSendUrl[] = "RemoteSendUrl";
constexpr char kRemoteNoUrlLocal[] = "RemoteNoUrl,Local";
{
// The service is enabled by default.
EXPECT_TRUE(SearchSuggestService::IsEnabled());
}
{
// Enabling omnibox::kOnFocusSuggestions for SERP does not disable the
// service.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 6 is Search Engine Results Page.
feature_params["ZeroSuggestVariant:6:*"] = kRemoteSendUrl;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params}}, {});
EXPECT_TRUE(SearchSuggestService::IsEnabled());
}
{
// Enabling omnibox::kOnFocusSuggestions for NTP Omnibox disables the
// service.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 7 is NTP with Omnibox as starting focus.
feature_params["ZeroSuggestVariant:7:*"] = kRemoteNoUrlLocal;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params}}, {});
// The service is disabled by default.
EXPECT_FALSE(SearchSuggestService::IsEnabled());
}
{
// Enabling omnibox::kOnFocusSuggestions for NTP Realbox disables the
// service.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 15 is NTP Realbox as starting focus.
feature_params["ZeroSuggestVariant:15:*"] = kRemoteNoUrlLocal;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params}}, {});
EXPECT_FALSE(SearchSuggestService::IsEnabled());
}
{
// Disabling omnibox::kNewSearchFeatures does not disable the service.
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters({},
{omnibox::kNewSearchFeatures});
EXPECT_TRUE(SearchSuggestService::IsEnabled());
}
{
// Disabling ntp_features::kSearchSuggestChips does not disable the service.
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{}, {ntp_features::kSearchSuggestChips});
EXPECT_TRUE(SearchSuggestService::IsEnabled());
}
{
// Enabling ntp_features::kSearchSuggestChips enables the service despite
// the other config.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 7 is NTP with Omnibox as starting focus.
feature_params["ZeroSuggestVariant:7:*"] = kRemoteNoUrlLocal;
// Note: ZPS variant 15 is NTP Realbox as starting focus.
feature_params["ZeroSuggestVariant:15:*"] = kRemoteNoUrlLocal;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params},
{ntp_features::kSearchSuggestChips, {}}},
{});
feature_list.InitAndEnableFeature(ntp_features::kSearchSuggestChips);
EXPECT_TRUE(SearchSuggestService::IsEnabled());
}
{
// Disabling ntp_features::kDisableSearchSuggestChips does not enable the
// service.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 15 is NTP Realbox as starting focus.
feature_params["ZeroSuggestVariant:15:*"] = kRemoteNoUrlLocal;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params}},
{ntp_features::kDisableSearchSuggestChips});
feature_list.InitAndDisableFeature(
ntp_features::kDisableSearchSuggestChips);
EXPECT_FALSE(SearchSuggestService::IsEnabled());
}
{
// Enabling ntp_features::kDisableSearchSuggestChips disables the service
// despite the other configs.
base::test::ScopedFeatureList feature_list;
std::map<std::string, std::string> feature_params;
// Note: ZPS variant 7 is NTP with Omnibox as starting focus.
feature_params["ZeroSuggestVariant:7:*"] = kRemoteNoUrlLocal;
// Note: ZPS variant 15 is NTP Realbox as starting focus.
feature_params["ZeroSuggestVariant:15:*"] = kRemoteNoUrlLocal;
feature_list.InitWithFeaturesAndParameters(
{{omnibox::kOnFocusSuggestions, feature_params},
{ntp_features::kSearchSuggestChips, {}},
{ntp_features::kDisableSearchSuggestChips, {}}},
{});
feature_list.InitWithFeatures({ntp_features::kSearchSuggestChips,
ntp_features::kDisableSearchSuggestChips},
{});
EXPECT_FALSE(SearchSuggestService::IsEnabled());
}
}
......
......@@ -88,18 +88,12 @@ bool AllowLocalHistoryZeroSuggestSuggestions(const AutocompleteInput& input) {
return true;
}
return base::Contains(
OmniboxFieldTrial::GetZeroSuggestVariants(current_page_classification),
LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
return false;
#endif
}
} // namespace
// static
const char LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant[] =
"Local";
// static
LocalHistoryZeroSuggestProvider* LocalHistoryZeroSuggestProvider::Create(
AutocompleteProviderClient* client,
......
......@@ -21,11 +21,6 @@ class QueryResults;
// history when Google is the default search engine.
class LocalHistoryZeroSuggestProvider : public AutocompleteProvider {
public:
// ZeroSuggestVariant field trial param value for the local history query
// suggestions.
// Public for testing.
static const char kZeroSuggestLocalVariant[];
// Creates and returns an instance of this provider.
static LocalHistoryZeroSuggestProvider* Create(
AutocompleteProviderClient* client,
......
......@@ -78,9 +78,9 @@ class LocalHistoryZeroSuggestProviderTest
LocalHistoryZeroSuggestProvider::Create(client_.get(), this));
#if defined(OS_IOS) // Only needed for iOS.
SetZeroSuggestVariant(
metrics::OmniboxEventProto::NTP_REALBOX,
LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
scoped_feature_list_ = std::make_unique<base::test::ScopedFeatureList>();
scoped_feature_list_->InitAndEnableFeature(
omnibox::kReactiveZeroSuggestionsOnNTPRealbox);
#endif
// Add the fallback default search provider to the TemplateURLService so
......@@ -106,11 +106,6 @@ class LocalHistoryZeroSuggestProviderTest
// AutocompleteProviderListener
void OnProviderUpdate(bool updated_matches) override;
// Configures the ZeroSuggestVariant field trial param with the given value
// for the given context.
void SetZeroSuggestVariant(PageClassification page_classification,
std::string zero_suggest_variant_value);
// Fills the URLDatabase with search URLs using the provided information.
void LoadURLs(const std::vector<TestURLData>& url_data_list);
......@@ -137,17 +132,6 @@ class LocalHistoryZeroSuggestProviderTest
scoped_refptr<LocalHistoryZeroSuggestProvider> provider_;
};
void LocalHistoryZeroSuggestProviderTest::SetZeroSuggestVariant(
PageClassification page_classification,
std::string zero_suggest_variant_value) {
scoped_feature_list_ = std::make_unique<base::test::ScopedFeatureList>();
scoped_feature_list_->InitAndEnableFeatureWithParameters(
omnibox::kOnFocusSuggestions,
{{std::string(OmniboxFieldTrial::kZeroSuggestVariantRule) + ":" +
base::NumberToString(static_cast<int>(page_classification)) + ":*",
zero_suggest_variant_value}});
}
void LocalHistoryZeroSuggestProviderTest::LoadURLs(
const std::vector<TestURLData>& url_data_list) {
const Time now = Time::Now();
......@@ -281,15 +265,15 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, Incognito) {
ExpectMatches({{"hello world", 500}});
}
// Tests that suggestions are returned only if ZeroSuggestVariant is configured
// Tests that suggestions are returned only if FeatureFlags is configured
// to return local history suggestions in the NTP.
#if defined(OS_IOS)
// Flaky thread check failure: https://crbug.com/1071877
#define MAYBE_ZeroSuggestVariant DISABLED_ZeroSuggestVariant
#define MAYBE_FeatureFlags DISABLED_FeatureFlags
#else
#define MAYBE_ZeroSuggestVariant ZeroSuggestVariant
#define MAYBE_FeatureFlags FeatureFlags
#endif
TEST_F(LocalHistoryZeroSuggestProviderTest, MAYBE_ZeroSuggestVariant) {
TEST_F(LocalHistoryZeroSuggestProviderTest, MAYBE_FeatureFlags) {
LoadURLs({
{default_search_provider(), "hello world", "&foo=bar", 1},
});
......@@ -304,25 +288,6 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, MAYBE_ZeroSuggestVariant) {
ExpectMatches({});
#endif
// Verify that local history zero-prefix suggestions are enabled on the NTP
// only.
SetZeroSuggestVariant(
metrics::OmniboxEventProto::OTHER,
LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
StartProviderAndWaitUntilDone(
/*text=*/"", OmniboxFocusType::ON_FOCUS,
/*page_classification=*/metrics::OmniboxEventProto::OTHER);
ExpectMatches({});
SetZeroSuggestVariant(metrics::OmniboxEventProto::NTP_REALBOX,
/*zero_suggest_variant_value=*/"blah");
StartProviderAndWaitUntilDone();
#if !defined(OS_IOS) // Enabled by default on Desktop and Android NTP.
ExpectMatches({{"hello world", 500}});
#else
ExpectMatches({});
#endif
// Verify that reactive zero-prefix suggestions enable local history
// zero-prefix suggestions on the NTP.
scoped_feature_list_ = std::make_unique<base::test::ScopedFeatureList>();
......@@ -361,23 +326,6 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, MAYBE_ZeroSuggestVariant) {
#else
ExpectMatches({});
#endif
// Verify that configuring the ZeroSuggestVariant param with "local" for the
// NTP, enables local history zero-prefix suggestions for that context.
SetZeroSuggestVariant(
metrics::OmniboxEventProto::NTP_REALBOX,
LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
StartProviderAndWaitUntilDone();
ExpectMatches({{"hello world", 500}});
SetZeroSuggestVariant(
metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
StartProviderAndWaitUntilDone(
/*text=*/"", OmniboxFocusType::ON_FOCUS,
/*page_classification=*/
metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS);
ExpectMatches({{"hello world", 500}});
}
// Tests that search terms are extracted from the default search provider's
......
......@@ -244,53 +244,6 @@ base::Time OmniboxFieldTrial::GetLocalHistoryZeroSuggestAgeThreshold() {
return (base::Time::Now() - base::TimeDelta::FromDays(param_value_as_int));
}
// static
std::vector<std::string> OmniboxFieldTrial::GetZeroSuggestVariants(
OmniboxEventProto::PageClassification page_classification) {
if (!base::FeatureList::IsEnabled(omnibox::kNewSearchFeatures))
return {};
// Deprecated: Prefer to add a simple boolean flag that's checked directly
// in ZeroSuggestProvider instead of using this complicated mechanism.
//
// We check all these features for ZeroSuggestVariant because it's not
// possible to enable multiple features using Finch Forcing groups
// (omnibox::kOnFocusSuggestions as well as another feature). Therefore, in
// order to specify the ZeroSuggestVariant parameter in those groups we allow
// it to be associated with the feature that is being force enabled.
const base::Feature* features_to_check[] = {
// kOnFocusSuggestions must be the first checked feature, because it's
// used by about:flags to set a variety of ZeroSuggest parameters.
// We want it to be first so it overrides server-provided field trials.
&omnibox::kOnFocusSuggestions,
&omnibox::kZeroSuggestionsOnNTP,
&omnibox::kZeroSuggestionsOnNTPRealbox,
&omnibox::kZeroSuggestionsOnSERP,
};
for (const base::Feature* feature : features_to_check) {
// We check every feature on the list until we find one that matches
// |page_classification|, since GetValueForRuleInContextByFeature returns
// an empty string if no paramater matches |page_classification|.
//
// For instance, if we have:
// kOnFocusSuggestions - ZeroSuggestVariant:7:* = RemoteNoUrl
// kZeroSuggestionsOnNTPRealbox - ZeroSuggestVariant:15:* = RemoteSendUrl
//
// These can both simultaneously work, since they configure different
// page classifications. If one of them had a ZeroSuggestVariant:*:*
// wildcard rule, however, it would shadow all subsequent features.
auto parameter_value = internal::GetValueForRuleInContextByFeature(
*feature, kZeroSuggestVariantRule, page_classification);
if (!parameter_value.empty()) {
return base::SplitString(parameter_value, ",", base::TRIM_WHITESPACE,
base::SPLIT_WANT_NONEMPTY);
}
}
return {};
}
bool OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
OmniboxEventProto::PageClassification current_page_classification,
int* max_relevance) {
......@@ -905,7 +858,6 @@ const char OmniboxFieldTrial::kHQPTypedValueRule[] = "HQPTypedValue";
const char OmniboxFieldTrial::kHQPAllowMatchInTLDRule[] = "HQPAllowMatchInTLD";
const char OmniboxFieldTrial::kHQPAllowMatchInSchemeRule[] =
"HQPAllowMatchInScheme";
const char OmniboxFieldTrial::kZeroSuggestVariantRule[] = "ZeroSuggestVariant";
const char
OmniboxFieldTrial::kMeasureSuggestPollingDelayFromLastKeystrokeRule[] =
"MeasureSuggestPollingDelayFromLastKeystroke";
......
......@@ -164,14 +164,6 @@ base::TimeDelta StopTimerFieldTrialDuration();
// normalized keyword search term as a zero-prefix suggestion.
base::Time GetLocalHistoryZeroSuggestAgeThreshold();
// ---------------------------------------------------------
// For the ZeroSuggestProvider field trial.
// Returns the configured "ZeroSuggestVariant" parameter values for
// |page_classification|.
std::vector<std::string> GetZeroSuggestVariants(
metrics::OmniboxEventProto::PageClassification page_classification);
// ---------------------------------------------------------
// For the ShortcutsScoringMaxRelevance experiment that's part of the
// bundled omnibox field trial.
......
......@@ -389,33 +389,6 @@ TEST_F(OmniboxFieldTrialTest, LocalZeroSuggestAgeThreshold) {
base::TimeDelta(base::Time::Now() - age_threshold).InDays());
}
TEST_F(OmniboxFieldTrialTest, GetZeroSuggestVariantsCanUseMultipleFeatures) {
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{
{omnibox::kOnFocusSuggestions,
// 7 is NTP omnibox.
{{"ZeroSuggestVariant:7:*", "omnibox1,omnibox2"}}},
{omnibox::kZeroSuggestionsOnNTPRealbox,
// 15 is NTP realbox.
{{"ZeroSuggestVariant:15:*", "realbox1, realbox2"}}},
},
{});
std::vector<std::string> omnibox_variants =
OmniboxFieldTrial::GetZeroSuggestVariants(
OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS);
ASSERT_EQ(2U, omnibox_variants.size());
EXPECT_EQ("omnibox1", omnibox_variants[0]);
EXPECT_EQ("omnibox2", omnibox_variants[1]);
std::vector<std::string> realbox_variants =
OmniboxFieldTrial::GetZeroSuggestVariants(OmniboxEventProto::NTP_REALBOX);
ASSERT_EQ(2U, realbox_variants.size());
EXPECT_EQ("realbox1", realbox_variants[0]);
EXPECT_EQ("realbox2", realbox_variants[1]);
}
TEST_F(OmniboxFieldTrialTest, HUPNewScoringFieldTrial) {
{
std::map<std::string, std::string> params;
......
......@@ -132,12 +132,6 @@ bool RemoteNoUrlSuggestionsAreAllowed(
} // namespace
// static
const char ZeroSuggestProvider::kNoneVariant[] = "None";
const char ZeroSuggestProvider::kRemoteNoUrlVariant[] = "RemoteNoUrl";
const char ZeroSuggestProvider::kRemoteSendUrlVariant[] = "RemoteSendUrl";
const char ZeroSuggestProvider::kMostVisitedVariant[] = "MostVisited";
// static
ZeroSuggestProvider* ZeroSuggestProvider::Create(
AutocompleteProviderClient* client,
......@@ -248,9 +242,7 @@ void ZeroSuggestProvider::Stop(bool clear_cached_results,
}
void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) {
if (base::Contains(OmniboxFieldTrial::GetZeroSuggestVariants(
current_page_classification_),
kRemoteNoUrlVariant)) {
if (result_type_running_ == REMOTE_NO_URL) {
// Remove the deleted match from the cache, so it is not shown to the user
// again. Since we cannot remove just one result, blow away the cache.
client()->GetPrefs()->SetString(omnibox::kZeroSuggestCachedResults,
......@@ -664,12 +656,6 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
kOmniboxZeroSuggestEligibleHistogramName, static_cast<int>(eligibility),
static_cast<int>(ZeroSuggestEligibility::ELIGIBLE_MAX_VALUE));
const auto field_trial_variants =
OmniboxFieldTrial::GetZeroSuggestVariants(current_page_classification);
if (base::Contains(field_trial_variants, kNoneVariant))
return NONE;
if (current_page_classification == OmniboxEventProto::CHROMEOS_APP_LIST)
return REMOTE_NO_URL;
......@@ -713,18 +699,6 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
}
}
if (base::Contains(field_trial_variants, kRemoteNoUrlVariant) &&
remote_no_url_allowed) {
return REMOTE_NO_URL;
}
if (base::Contains(field_trial_variants, kRemoteSendUrlVariant) &&
can_send_current_url)
return REMOTE_SEND_URL;
if (base::Contains(field_trial_variants, kMostVisitedVariant))
return MOST_VISITED;
// For Desktop, Android, and iOS, default to REMOTE_NO_URL on the NTP, if
// allowed.
if (IsNTPPage(current_page_classification) && remote_no_url_allowed)
......
......@@ -41,13 +41,24 @@ class SimpleURLLoader;
// omnibox text and suggestions.
class ZeroSuggestProvider : public BaseSearchProvider {
public:
// ZeroSuggestVariant field trial param values corresponding to each
// ZeroSuggestProvider::ResultType.
// Public for testing.
static const char kNoneVariant[];
static const char kRemoteNoUrlVariant[];
static const char kRemoteSendUrlVariant[];
static const char kMostVisitedVariant[];
// ZeroSuggestProvider is processing one of the following type of results
// at any time. Exposed as public for testing purposes.
enum ResultType {
NONE,
// A remote endpoint (usually the default search provider) is queried for
// suggestions. The endpoint is sent the user's authentication state, but
// not sent the current URL.
REMOTE_NO_URL,
// A remote endpoint (usually the default search provider) is queried for
// suggestions. The endpoint is sent the user's authentication state and
// the current URL.
REMOTE_SEND_URL,
// Gets the most visited sites from local history.
MOST_VISITED,
};
// Creates and returns an instance of this provider.
static ZeroSuggestProvider* Create(AutocompleteProviderClient* client,
......@@ -83,6 +94,10 @@ class ZeroSuggestProvider : public BaseSearchProvider {
return results_.hidden_group_ids;
}
ResultType GetResultTypeRunningForTesting() const {
return result_type_running_;
}
private:
FRIEND_TEST_ALL_PREFIXES(ZeroSuggestProviderTest,
AllowZeroSuggestSuggestions);
......@@ -99,25 +114,6 @@ class ZeroSuggestProvider : public BaseSearchProvider {
ZeroSuggestProvider(const ZeroSuggestProvider&) = delete;
ZeroSuggestProvider& operator=(const ZeroSuggestProvider&) = delete;
// ZeroSuggestProvider is processing one of the following type of results
// at any time.
enum ResultType {
NONE,
// A remote endpoint (usually the default search provider) is queried for
// suggestions. The endpoint is sent the user's authentication state, but
// not sent the current URL.
REMOTE_NO_URL,
// A remote endpoint (usually the default search provider) is queried for
// suggestions. The endpoint is sent the user's authentication state and
// the current URL.
REMOTE_SEND_URL,
// Gets the most visited sites from local history.
MOST_VISITED,
};
// BaseSearchProvider:
const TemplateURL* GetTemplateURL(bool is_keyword) const override;
const AutocompleteInput GetInput(bool is_keyword) const override;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment