[omnibox] Enable preserve default from fakebox if the input is a URL.
Previously, if the user was starting from the fakebox, type demotion
could result in a query suggestion being the default even when the
user's input was classified as a URL. With this change, we use the
preserve-default mechanism to prevent type demotion in this case. This
almost guarantees that a URL suggestion will be default because the top
local and server suggestions for a URL input are always and likely will
always be URL suggestions.
This change also makes it so that we no longer use type demotion of
relevance when deduping but instead always choose the natural highest
relevance candidate for each suggestion. This ensures that we actually
get the highest relevance suggestion when applying preserve-default.
[email protected]
(cherry picked from commit ff56e15623b53b112e860986f6ef874f5437926e)
Change-Id: I3b7094a54ea6438306c0e069a83f4e0db0939090
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1723807
Commit-Queue: Justin Donnelly <[email protected]>
Reviewed-by: Tommy Li <[email protected]>
Reviewed-by: Mark Pearson <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#684984}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749580
Reviewed-by: manuk hovanesian <[email protected]>
Cr-Commit-Position: refs/branch-heads/3865@{#329}
Cr-Branched-From: 0cdcc6158160790658d1f033d3db873603250124-refs/heads/master@{#681094}
diff --git a/components/omnibox/browser/autocomplete_result.cc b/components/omnibox/browser/autocomplete_result.cc
index 965ddde..9fafcd38 100644
--- a/components/omnibox/browser/autocomplete_result.cc
+++ b/components/omnibox/browser/autocomplete_result.cc
@@ -183,7 +183,7 @@
std::sort(matches_.begin(), matches_.end(), comparing_object);
// Top match is not allowed to be the default match. Find the most
// relevant legal match and shift it to the front.
- auto it = FindTopMatch(input.current_page_classification(), &matches_);
+ auto it = FindTopMatch(input, &matches_);
if (it != matches_.end()) {
const size_t cookie = it->subrelevance;
auto next = std::next(it);
@@ -374,17 +374,31 @@
// static
ACMatches::const_iterator AutocompleteResult::FindTopMatch(
- OmniboxEventProto::PageClassification page_classification,
+ const AutocompleteInput& input,
const ACMatches& matches) {
- return FindTopMatch(page_classification, const_cast<ACMatches*>(&matches));
+ return FindTopMatch(input, const_cast<ACMatches*>(&matches));
}
// static
ACMatches::iterator AutocompleteResult::FindTopMatch(
- OmniboxEventProto::PageClassification page_classification,
+ const AutocompleteInput& input,
ACMatches* matches) {
- if (page_classification !=
- OmniboxEventProto::INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS &&
+ // The matches may be sorted by type-demoted relevance. If we want to choose
+ // the highest-relevance, allowed-to-be-default match while ignoring type
+ // demotion, as we do when IsPreserveDefaultMatchScoreEnabled is true, we need
+ // to explicitly find the highest relevance match rather than just accepting
+ // the first allowed-to-be--default match in the list.
+ // The goal of this behavior is to ensure that in situations where the user
+ // expects to see a commonly visited URL as the default match, the URL is not
+ // supressed by type demotion.
+ // However, even if IsPreserveDefaultMatchScoreEnabled is true, we don't care
+ // about this URL behavior when the user is using the fakebox, which is
+ // intended to work more like a search-only box. Unless the user's input is a
+ // URL in which case we still want to ensure they can get a URL as the default
+ // match.
+ if ((input.current_page_classification() !=
+ OmniboxEventProto::INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS ||
+ input.type() == metrics::OmniboxInputType::URL) &&
OmniboxFieldTrial::IsPreserveDefaultMatchScoreEnabled()) {
auto best = matches->end();
for (auto it = matches->begin(); it != matches->end(); ++it) {
@@ -475,7 +489,7 @@
// Find the best match.
auto best_match = duplicate_matches.begin();
for (auto i = std::next(best_match); i != duplicate_matches.end(); ++i) {
- best_match = BetterMatch(i, best_match, page_classification);
+ best_match = BetterDuplicate(i, best_match);
}
// Rotate the chosen match to be first, if necessary, so we know to keep it.
@@ -537,16 +551,11 @@
}
// static
-std::list<ACMatches::iterator>::iterator AutocompleteResult::BetterMatch(
+std::list<ACMatches::iterator>::iterator AutocompleteResult::BetterDuplicate(
std::list<ACMatches::iterator>::iterator first,
- std::list<ACMatches::iterator>::iterator second,
- metrics::OmniboxEventProto::PageClassification page_classification) {
+ std::list<ACMatches::iterator>::iterator second) {
std::list<ACMatches::iterator>::iterator preferred_match;
std::list<ACMatches::iterator>::iterator non_preferred_match;
- // This object implements greater than.
- CompareWithDemoteByType<AutocompleteMatch> compare_demote_by_type(
- page_classification);
-
// The following logic enforces constraints we care about regarding the
// the characteristics of the candidate matches. In order of priority:
//
@@ -609,13 +618,15 @@
preferred_match = second;
non_preferred_match = first;
} else {
- // By default, simply prefer the match with the higher type-adjusted score.
- return compare_demote_by_type(**first, **second) ? first : second;
+ // By default, simply prefer the match with the higher relevance. Note that
+ // we do not apply type-based demotion here (CompareWithDemoteByType)
+ // because we only apply demotion when ordering the final set of matches.
+ return (*first)->relevance >= (*second)->relevance ? first : second;
}
// If a match is preferred despite having a lower score, boost its score
// to that of the other match.
- if (compare_demote_by_type(**non_preferred_match, **preferred_match)) {
+ if ((*non_preferred_match)->relevance > (*preferred_match)->relevance) {
(*preferred_match)
->RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom,
(*preferred_match)->relevance);
diff --git a/components/omnibox/browser/autocomplete_result.h b/components/omnibox/browser/autocomplete_result.h
index e83c103..1e407ca 100644
--- a/components/omnibox/browser/autocomplete_result.h
+++ b/components/omnibox/browser/autocomplete_result.h
@@ -97,12 +97,10 @@
// Returns the first match in |matches| which might be chosen as default.
// If |kOmniboxPreserveDefaultMatchScore| is enabled and the page is not
// the fake box, the scores are not demoted by type.
- static ACMatches::const_iterator FindTopMatch(
- metrics::OmniboxEventProto::PageClassification page_classification,
- const ACMatches& matches);
- static ACMatches::iterator FindTopMatch(
- metrics::OmniboxEventProto::PageClassification page_classification,
- ACMatches* matches);
+ static ACMatches::const_iterator FindTopMatch(const AutocompleteInput& input,
+ const ACMatches& matches);
+ static ACMatches::iterator FindTopMatch(const AutocompleteInput& input,
+ ACMatches* matches);
const GURL& alternate_nav_url() const { return alternate_nav_url_; }
@@ -158,14 +156,13 @@
typedef ACMatches::iterator::difference_type matches_difference_type;
#endif
- // Examines |first| and |second| and returns the one that is preferred based
- // on the constraints we want to enforce when deduping. Note that this may
- // modify the relevance, allowed_to_be_default_match, or inline_autocompletion
- // values of the returned match.
- static std::list<ACMatches::iterator>::iterator BetterMatch(
+ // Examines |first| and |second| and returns the match that is preferred when
+ // choosing between candidate duplicates. Note that this may modify the
+ // relevance, allowed_to_be_default_match, or inline_autocompletion values of
+ // the returned match.
+ static std::list<ACMatches::iterator>::iterator BetterDuplicate(
std::list<ACMatches::iterator>::iterator first,
- std::list<ACMatches::iterator>::iterator second,
- metrics::OmniboxEventProto::PageClassification page_classification);
+ std::list<ACMatches::iterator>::iterator second);
// Returns true if |matches| contains a match with the same destination as
// |match|.
diff --git a/components/omnibox/browser/autocomplete_result_unittest.cc b/components/omnibox/browser/autocomplete_result_unittest.cc
index edc9f596..57563f4 100644
--- a/components/omnibox/browser/autocomplete_result_unittest.cc
+++ b/components/omnibox/browser/autocomplete_result_unittest.cc
@@ -761,6 +761,26 @@
EXPECT_EQ("http://history-title/",
result.match_at(3)->destination_url.spec());
}
+
+ {
+ // Re-sort with a page classification of fake-box and an input that's a URL,
+ // and make sure history-title is once again the default match.
+ AutocompleteInput input(
+ base::ASCIIToUTF16("www.example.com"),
+ OmniboxEventProto::INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS,
+ TestSchemeClassifier());
+ AutocompleteResult result;
+ result.AppendMatches(input, matches);
+ result.SortAndCull(input, template_url_service_.get());
+
+ size_t expected_order[] = {1, 0, 2, 3};
+
+ ASSERT_EQ(base::size(expected_order), result.size());
+ for (size_t i = 0; i < base::size(expected_order); ++i) {
+ EXPECT_EQ(data[expected_order[i]].destination_url,
+ result.match_at(i)->destination_url.spec());
+ }
+ }
}
TEST_F(AutocompleteResultTest, SortAndCullWithMatchDupsAndDemotionsByType) {
diff --git a/components/omnibox/browser/search_provider.cc b/components/omnibox/browser/search_provider.cc
index 595d325..736b5368 100644
--- a/components/omnibox/browser/search_provider.cc
+++ b/components/omnibox/browser/search_provider.cc
@@ -555,8 +555,8 @@
(keyword_url != nullptr) &&
(keyword_url->type() == TemplateURL::OMNIBOX_API_EXTENSION);
if ((keyword_url != nullptr) && !is_extension_keyword &&
- (AutocompleteResult::FindTopMatch(input_.current_page_classification(),
- matches_) == matches_.end())) {
+ (AutocompleteResult::FindTopMatch(input_, matches_) ==
+ matches_.end())) {
// In non-extension keyword mode, disregard the keyword verbatim suggested
// relevance if necessary, so at least one match is allowed to be default.
// (In extension keyword mode this is not necessary because the extension
@@ -577,9 +577,8 @@
keyword_results_.verbatim_relevance = -1;
ConvertResultsToAutocompleteMatches();
}
- if (!is_extension_keyword &&
- (AutocompleteResult::FindTopMatch(input_.current_page_classification(),
- matches_) == matches_.end())) {
+ if (!is_extension_keyword && (AutocompleteResult::FindTopMatch(
+ input_, matches_) == matches_.end())) {
// Guarantee that SearchProvider returns a legal default match (except
// when in extension-based keyword mode). The omnibox always needs at
// least one legal default match, and it relies on SearchProvider in
@@ -595,16 +594,14 @@
}
DCHECK(!IsTopMatchSearchWithURLInput());
DCHECK(is_extension_keyword || (AutocompleteResult::FindTopMatch(
- input_.current_page_classification(),
- matches_) != matches_.end()));
+ input_, matches_) != matches_.end()));
}
}
void SearchProvider::RecordTopSuggestion() {
top_query_suggestion_fill_into_edit_ = base::string16();
top_navigation_suggestion_ = GURL();
- auto first_match = AutocompleteResult::FindTopMatch(
- input_.current_page_classification(), matches_);
+ auto first_match = AutocompleteResult::FindTopMatch(input_, matches_);
if (first_match != matches_.end()) {
// Identify if this match came from a query suggestion or a navsuggestion.
// In either case, extracts the identifying feature of the suggestion
@@ -1105,8 +1102,7 @@
// Guarantee that if there's a legal default match anywhere in the result
// set that it'll get returned. The rotate() call does this by moving the
// default match to the front of the list.
- auto default_match = AutocompleteResult::FindTopMatch(
- input_.current_page_classification(), &matches);
+ auto default_match = AutocompleteResult::FindTopMatch(input_, &matches);
if (default_match != matches.end())
std::rotate(matches.begin(), default_match, default_match + 1);
@@ -1166,8 +1162,7 @@
}
bool SearchProvider::IsTopMatchSearchWithURLInput() const {
- auto first_match = AutocompleteResult::FindTopMatch(
- input_.current_page_classification(), matches_);
+ auto first_match = AutocompleteResult::FindTopMatch(input_, matches_);
return (input_.type() == metrics::OmniboxInputType::URL) &&
(first_match != matches_.end()) &&
(first_match->relevance > CalculateRelevanceForVerbatim()) &&
