Skip to content

GitLab

T
tangled
Commit 43e0be28 authored by mpearson's avatar mpearson Committed by Commit bot
Browse files
Options

Remove Omnibox Disallow Inlining Field Trial 

The results are summarized here: goto/disallow-inlining-trial-results

This is effectively a revert of
https://src.chromium.org/viewvc/chrome?revision=273966&view=revision
and
http://src.chromium.org/viewvc/chrome?view=revision&revision=278811

BUG=

Review URL: https://codereview.chromium.org/879053002

Cr-Commit-Position: refs/heads/master@{#313375}
parent e5937efe
Hide whitespace changes
Inline Side-by-side
Showing with 22 additions and 272 deletions
chrome/browser/autocomplete/history_url_provider.cc
View file @ 43e0be28
......@@ -567,7 +567,7 @@ void HistoryURLProvider::Start(const AutocompleteInput& input,
if (url_db) {
DoAutocomplete(NULL, url_db, params.get());
matches_.clear();
PromoteMatchesIfNecessary(*params);
PromoteMatchIfNecessary(*params);
// NOTE: We don't reset |params| here since at least the |promote_type|
// field on it will be read by the second pass -- see comments in
// DoAutocomplete().
......@@ -765,7 +765,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
// searching has been disabled by policy. In the cases where we've parsed as
// UNKNOWN, we'll still show an accidental search infobar if need be.
VisitClassifier classifier(this, params->input, db);
params->have_what_you_typed_match =
bool have_what_you_typed_match =
(params->input.type() != metrics::OmniboxInputType::QUERY) &&
((params->input.type() != metrics::OmniboxInputType::UNKNOWN) ||
(classifier.type() == VisitClassifier::UNVISITED_INTRANET) ||
......@@ -773,8 +773,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
(AutocompleteInput::NumNonHostComponents(params->input.parts()) > 0) ||
!params->default_search_provider);
const bool have_shorter_suggestion_suitable_for_inline_autocomplete =
PromoteOrCreateShorterSuggestion(
db, params->have_what_you_typed_match, params);
PromoteOrCreateShorterSuggestion(db, have_what_you_typed_match, params);
// Check whether what the user typed appears in history.
const bool can_check_history_for_exact_match =
......@@ -808,7 +807,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
params->promote_type = HistoryURLProviderParams::FRONT_HISTORY_MATCH;
} else {
// Failed to promote any URLs. Use the What You Typed match, if we have it.
params->promote_type = params->have_what_you_typed_match ?
params->promote_type = have_what_you_typed_match ?
HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH :
HistoryURLProviderParams::NEITHER;
}
......@@ -826,19 +825,15 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
}
}
void HistoryURLProvider::PromoteMatchesIfNecessary(
void HistoryURLProvider::PromoteMatchIfNecessary(
const HistoryURLProviderParams& params) {
if (params.promote_type == HistoryURLProviderParams::FRONT_HISTORY_MATCH) {
matches_.push_back(HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
if (OmniboxFieldTrial::AddUWYTMatchEvenIfPromotedURLs() &&
params.have_what_you_typed_match) {
matches_.push_back(params.what_you_typed_match);
}
} else if (params.promote_type ==
HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) {
matches_.push_back(params.what_you_typed_match);
}
if (params.promote_type == HistoryURLProviderParams::NEITHER)
return;
matches_.push_back(
(params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ?
params.what_you_typed_match :
HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
}
void HistoryURLProvider::QueryComplete(
......@@ -859,7 +854,7 @@ void HistoryURLProvider::QueryComplete(
// match in it, whereas |params->matches| will be empty.
if (!params->failed) {
matches_.clear();
PromoteMatchesIfNecessary(*params);
PromoteMatchIfNecessary(*params);
// Determine relevance of highest scoring match, if any.
int relevance = matches_.empty() ?
......
chrome/browser/autocomplete/history_url_provider.h
View file @ 43e0be28
......@@ -157,11 +157,6 @@ struct HistoryURLProviderParams {
// this to. See comments in DoAutocomplete().
PromoteType promote_type;
// True if |what_you_typed_match| is eligible for display. If this is true,
// PromoteMatchesIfNecessary() may choose to place |what_you_typed_match| on
// |matches_| even when |promote_type| is not WHAT_YOU_TYPED_MATCH.
bool have_what_you_typed_match;
// Languages we should pass to gfx::GetCleanStringFromUrl.
std::string languages;
......@@ -250,10 +245,10 @@ class HistoryURLProvider : public HistoryProvider {
history::URLDatabase* db,
HistoryURLProviderParams* params);
// May promote the what you typed match, the first history match in
// parmas->matches, or both to the front of |matches_|, depending on the
// values of params->promote_type and params->have_what_you_typed_match.
void PromoteMatchesIfNecessary(const HistoryURLProviderParams& params);
// May promote either the what you typed match or first history match in
// params->matches to the front of |matches_|, depending on the value of
// params->promote_type.
void PromoteMatchIfNecessary(const HistoryURLProviderParams& params);
// Dispatches the results to the autocomplete controller. Called on the
// main thread by ExecuteWithDB when the results are available.
......
components/omnibox/autocomplete_result.cc
View file @ 43e0be28
......@@ -8,7 +8,6 @@
#include <iterator>
#include "base/logging.h"
#include "base/metrics/histogram.h"
#include "base/strings/utf_string_conversions.h"
#include "components/metrics/proto/omnibox_event.pb.h"
#include "components/metrics/proto/omnibox_input_type.pb.h"
......@@ -97,18 +96,6 @@ bool DestinationSort::operator()(const AutocompleteMatch& elem1,
return elem1.stripped_destination_url < elem2.stripped_destination_url;
}
// Returns true if |match| is allowed to the default match taking into account
// whether we're supposed to (and able to) demote all matches with inline
// autocompletions.
bool AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
const AutocompleteMatch& match,
const bool has_legal_default_match_without_completion) {
return match.allowed_to_be_default_match &&
(!OmniboxFieldTrial::DisableInlining() ||
!has_legal_default_match_without_completion ||
match.inline_autocompletion.empty());
}
}; // namespace
// static
......@@ -196,37 +183,16 @@ void AutocompleteResult::SortAndCull(
DedupMatchesByDestination(input.current_page_classification(), true,
&matches_);
// If the result set has at least one legal default match without an inline
// autocompletion, then in the disable inlining experiment it will be okay
// to demote all matches with inline autocompletions. On the other hand, if
// the experiment is active but there is no legal match without an inline
// autocompletion, then we'll pretend the experiment is not active and not
// demote the matches with an inline autocompletion. In other words, an
// alternate name for this variable is
// allowed_to_demote_matches_with_inline_autocompletion.
bool has_legal_default_match_without_completion = false;
for (AutocompleteResult::iterator it = matches_.begin();
(it != matches_.end()) && !has_legal_default_match_without_completion;
++it) {
if (it->allowed_to_be_default_match && it->inline_autocompletion.empty())
has_legal_default_match_without_completion = true;
}
UMA_HISTOGRAM_BOOLEAN("Omnibox.HasLegalDefaultMatchWithoutCompletion",
has_legal_default_match_without_completion);
// Sort and trim to the most relevant kMaxMatches matches.
size_t max_num_matches = std::min(kMaxMatches, matches_.size());
CompareWithDemoteByType comparing_object(input.current_page_classification());
std::sort(matches_.begin(), matches_.end(), comparing_object);
if (!matches_.empty() &&
!AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
*matches_.begin(), has_legal_default_match_without_completion)) {
if (!matches_.empty() && !matches_.begin()->allowed_to_be_default_match) {
// Top match is not allowed to be the default match. Find the most
// relevant legal match and shift it to the front.
for (AutocompleteResult::iterator it = matches_.begin() + 1;
it != matches_.end(); ++it) {
if (AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
*it, has_legal_default_match_without_completion)) {
if (it->allowed_to_be_default_match) {
std::rotate(matches_.begin(), it, it + 1);
break;
}
......
components/omnibox/autocomplete_result_unittest.cc
View file @ 43e0be28
......@@ -557,172 +557,6 @@ TEST_F(AutocompleteResultTest, SortAndCullReorderForDefaultMatch) {
}
}
TEST_F(AutocompleteResultTest, SortAndCullWithDisableInlining) {
TestData data[] = {
{ 0, 0, 1300 },
{ 1, 0, 1200 },
{ 2, 0, 1100 },
{ 3, 0, 1000 }
};
{
// Check that with the field trial disabled, we keep keep the first match
// first even if it has an inline autocompletion.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
AssertResultMatches(result, data, 4);
}
// Enable the field trial to disable inlining.
{
std::map<std::string, std::string> params;
params[OmniboxFieldTrial::kDisableInliningRule] = "true";
ASSERT_TRUE(variations::AssociateVariationParams(
OmniboxFieldTrial::kBundledExperimentFieldTrialName, "D", params));
}
base::FieldTrialList::CreateFieldTrial(
OmniboxFieldTrial::kBundledExperimentFieldTrialName, "D");
{
// Now the first match should be demoted past the second.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
ASSERT_EQ(4U, result.size());
EXPECT_EQ("http://b/", result.match_at(0)->destination_url.spec());
EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
EXPECT_EQ("http://c/", result.match_at(2)->destination_url.spec());
EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
}
{
// But if there was no inline autocompletion on the first match, then
// the order should stay the same. This is true even if there are
// inline autocompletions elsewhere.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[2].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
AssertResultMatches(result, data, 4);
}
{
// Try a more complicated situation.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].allowed_to_be_default_match = false;
matches[1].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
ASSERT_EQ(4U, result.size());
EXPECT_EQ("http://c/", result.match_at(0)->destination_url.spec());
EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
EXPECT_EQ("http://b/", result.match_at(2)->destination_url.spec());
EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
}
{
// Try another complicated situation.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
matches[1].allowed_to_be_default_match = false;
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
ASSERT_EQ(4U, result.size());
EXPECT_EQ("http://c/", result.match_at(0)->destination_url.spec());
EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
EXPECT_EQ("http://b/", result.match_at(2)->destination_url.spec());
EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
}
{
// Check that disaster doesn't strike if we can't demote the top inline
// autocompletion because every match either has a completion or isn't
// allowed to be the default match. In this case, we should leave
// everything untouched.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
matches[1].allowed_to_be_default_match = false;
matches[2].allowed_to_be_default_match = false;
matches[3].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
AssertResultMatches(result, data, 4);
}
{
// Check a similar situation, except in this case the top match is not
// allowed to the default match, so it still needs to be demoted so we
// get a legal default match first. That match will have an inline
// autocompletion because we don't have any better options.
ACMatches matches;
PopulateAutocompleteMatches(data, arraysize(data), &matches);
matches[0].allowed_to_be_default_match = false;
matches[1].inline_autocompletion = base::ASCIIToUTF16("completion");
matches[2].allowed_to_be_default_match = false;
matches[3].inline_autocompletion = base::ASCIIToUTF16("completion");
AutocompleteResult result;
result.AppendMatches(matches);
AutocompleteInput input(base::string16(), base::string16::npos,
std::string(), GURL(),
OmniboxEventProto::HOME_PAGE, false, false, false,
true,
TestSchemeClassifier());
result.SortAndCull(input, template_url_service_.get());
ASSERT_EQ(4U, result.size());
EXPECT_EQ("http://b/", result.match_at(0)->destination_url.spec());
EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
EXPECT_EQ("http://c/", result.match_at(2)->destination_url.spec());
EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
}
}
TEST_F(AutocompleteResultTest, ShouldHideTopMatch) {
base::FieldTrialList::CreateFieldTrial("InstantExtended",
"Group1 hide_verbatim:1");
......
components/omnibox/omnibox_field_trial.cc
View file @ 43e0be28
......@@ -301,12 +301,6 @@ bool OmniboxFieldTrial::HQPAllowMatchInSchemeValue() {
kHQPAllowMatchInSchemeRule) == "true";
}
bool OmniboxFieldTrial::DisableInlining() {
return variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kDisableInliningRule) == "true";
}
bool OmniboxFieldTrial::EnableAnswersInSuggest() {
const base::CommandLine* cl = base::CommandLine::ForCurrentProcess();
if (cl->HasSwitch(switches::kDisableAnswersInSuggest))
......@@ -319,12 +313,6 @@ bool OmniboxFieldTrial::EnableAnswersInSuggest() {
kAnswersInSuggestRule) == "true";
}
bool OmniboxFieldTrial::AddUWYTMatchEvenIfPromotedURLs() {
return variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kAddUWYTMatchEvenIfPromotedURLsRule) == "true";
}
bool OmniboxFieldTrial::DisplayHintTextWhenPossible() {
return variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
......@@ -367,10 +355,7 @@ const char OmniboxFieldTrial::kHQPAllowMatchInSchemeRule[] =
"HQPAllowMatchInScheme";
const char OmniboxFieldTrial::kZeroSuggestRule[] = "ZeroSuggest";
const char OmniboxFieldTrial::kZeroSuggestVariantRule[] = "ZeroSuggestVariant";
const char OmniboxFieldTrial::kDisableInliningRule[] = "DisableInlining";
const char OmniboxFieldTrial::kAnswersInSuggestRule[] = "AnswersInSuggest";
const char OmniboxFieldTrial::kAddUWYTMatchEvenIfPromotedURLsRule[] =
"AddUWYTMatchEvenIfPromotedURLs";
const char OmniboxFieldTrial::kDisplayHintTextWhenPossibleRule[] =
"DisplayHintTextWhenPossible";
const char OmniboxFieldTrial::kDisableResultsCachingRule[] =
......
components/omnibox/omnibox_field_trial.h
View file @ 43e0be28
......@@ -239,18 +239,6 @@ class OmniboxFieldTrial {
// match in scheme experiment isn't active.
static bool HQPAllowMatchInSchemeValue();
// ---------------------------------------------------------
// For the DisableInlining experiment that's part of the bundled omnibox
// field trial.
// Returns true if AutocompleteResult should prevent any suggestion with
// a non-empty |inline_autocomplete| from being the default match. In
// other words, prevent an inline autocompletion from appearing as the
// top suggestion / within the omnibox itself, reordering matches as
// necessary to make this true. Returns false if the experiment isn't
// active.
static bool DisableInlining();
// ---------------------------------------------------------
// For the AnswersInSuggest experiment that's part of the bundled omnibox
// field trial.
......@@ -261,20 +249,6 @@ class OmniboxFieldTrial {
// field trial state as well as the overriding command-line flags.
static bool EnableAnswersInSuggest();
// ---------------------------------------------------------
// For the AddUWYTMatchEvenIfPromotedURLs experiment that's part of the
// bundled omnibox field trial.
// Returns true if HistoryURL Provider should add the URL-what-you-typed match
// (if valid and reasonable) even if the provider has good inline
// autocompletions to offer. Normally HistoryURL does not add the UWYT match
// if there are good inline autocompletions, as the user could simply hit
// backspace to delete the completion and get the what-you-typed match.
// However, for the disabling inlining experiment we want to have the UWYT
// always explicitly displayed at an option if possible. Returns false if
// the experiment isn't active.
static bool AddUWYTMatchEvenIfPromotedURLs();
// ---------------------------------------------------------
// For the DisplayHintTextWhenPossible experiment that's part of the
// bundled omnibox field trial.
......@@ -309,9 +283,7 @@ class OmniboxFieldTrial {
static const char kHQPAllowMatchInSchemeRule[];
static const char kZeroSuggestRule[];
static const char kZeroSuggestVariantRule[];
static const char kDisableInliningRule[];
static const char kAnswersInSuggestRule[];
static const char kAddUWYTMatchEvenIfPromotedURLsRule[];
static const char kDisplayHintTextWhenPossibleRule[];
static const char kDisableResultsCachingRule[];
static const char kMeasureSuggestPollingDelayFromLastKeystrokeRule[];
......
tools/metrics/histograms/histograms.xml
View file @ 43e0be28
......@@ -22461,6 +22461,9 @@ Therefore, the affected-histogram name has to have at least one dot in it.
</histogram>
<histogram name="Omnibox.HasLegalDefaultMatchWithoutCompletion" enum="Boolean">
<obsolete>
Deprecated 2015-01-27
</obsolete>
<owner>mpearson@chromium.org</owner>
<summary>
Whether there was at least one legal default match without an
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