[omnibox] Add unit tests for AutocompleteResult::IsBetterMatch.

Also fix an issue with the score boosting.

Bug: 913299
Change-Id: Ia44b6e8658a37a75a1f64776306d764dc73d25ae
Reviewed-on: https://chromium-review.googlesource.com/c/1370394Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615658}

components/omnibox/browser/autocomplete_match.h

@@ -41,6 +41,9 @@ struct VectorIcon;
 const char kACMatchPropertySuggestionText[] = "match suggestion text";
 const char kACMatchPropertyContentsPrefix[] = "match contents prefix";
 const char kACMatchPropertyContentsStartIndex[] = "match contents start index";
+// A match attribute when a default match's score has been boosted with a higher
+// scoring non-default match.
+const char kACMatchPropertyScoreBoostedFrom[] = "score_boosted_from";
 
 // AutocompleteMatch ----------------------------------------------------------
 

components/omnibox/browser/autocomplete_result.cc

@@ -41,10 +41,6 @@ struct MatchGURLHash {
   }
 };
 
-// A match attribute when a default match's score has been boosted
-// with a higher scoring non-default match.
-static const char kScoreBoostedFrom[] = "score_boosted_from";
-
 // static
 size_t AutocompleteResult::GetMaxMatches() {
   constexpr size_t kDefaultMaxAutocompleteMatches = 6;
@@ -417,42 +413,6 @@ GURL AutocompleteResult::ComputeAlternateNavUrl(
              : GURL();
 }
 
-// static
-bool AutocompleteResult::IsBetterMatch(
-    AutocompleteMatch& first,
-    AutocompleteMatch& second,
-    metrics::OmniboxEventProto::PageClassification page_classification) {
-  // This object implements greater than.
-  CompareWithDemoteByType<AutocompleteMatch> compare_demote_by_type(
-      page_classification);
-
-  if (first.type == ACMatchType::SEARCH_SUGGEST_ENTITY &&
-      second.type != ACMatchType::SEARCH_SUGGEST_ENTITY) {
-    // If |first| is an entity suggestion and |second| isn't, |first| is
-    // considered better. If its type-adjusted relevance is lower, boost it to
-    // the value of |second|.
-    if (compare_demote_by_type(second, first)) {
-      first.RecordAdditionalInfo(kScoreBoostedFrom, second.relevance);
-      first.relevance = second.relevance;
-    }
-    return true;
-  } else if (first.type != ACMatchType::SEARCH_SUGGEST_ENTITY &&
-             second.type == ACMatchType::SEARCH_SUGGEST_ENTITY) {
-    // Likewise, if |second| is an entity suggestion and |first| isn't, first
-    // is not considered better, even if it has a higher type-adjusted
-    // relevance. If it does have a higher relevance, boost |second|.
-    if (compare_demote_by_type(first, second)) {
-      second.RecordAdditionalInfo(kScoreBoostedFrom, first.relevance);
-      second.relevance = first.relevance;
-    }
-    return false;
-  }
-
-  // In the case that both values are entity suggestions or both aren't, |first|
-  // is simply considered better if it's type-adjusted relevance is higher.
-  return compare_demote_by_type(first, second);
-}
-
 void AutocompleteResult::SortAndDedupMatches(
     metrics::OmniboxEventProto::PageClassification page_classification,
     ACMatches* matches) {
@@ -490,7 +450,8 @@ void AutocompleteResult::SortAndDedupMatches(
     }
     if (best_match != best_default && best_default != duplicate_matches.end()) {
       (*best_default)
-          ->RecordAdditionalInfo(kScoreBoostedFrom, (*best_default)->relevance);
+          ->RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom,
+                                 (*best_default)->relevance);
       (*best_default)->relevance = (*best_match)->relevance;
       best_match = best_default;
     }
@@ -551,6 +512,44 @@ size_t AutocompleteResult::EstimateMemoryUsage() const {
   return res;
 }
 
+// static
+bool AutocompleteResult::IsBetterMatch(
+    AutocompleteMatch& first,
+    AutocompleteMatch& second,
+    metrics::OmniboxEventProto::PageClassification page_classification) {
+  // This object implements greater than.
+  CompareWithDemoteByType<AutocompleteMatch> compare_demote_by_type(
+      page_classification);
+
+  if (first.type == ACMatchType::SEARCH_SUGGEST_ENTITY &&
+      second.type != ACMatchType::SEARCH_SUGGEST_ENTITY) {
+    // If |first| is an entity suggestion and |second| isn't, |first| is
+    // considered better. If its type-adjusted relevance is lower, boost it to
+    // the value of |second|.
+    if (compare_demote_by_type(second, first)) {
+      first.RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom,
+                                 first.relevance);
+      first.relevance = second.relevance;
+    }
+    return true;
+  } else if (first.type != ACMatchType::SEARCH_SUGGEST_ENTITY &&
+             second.type == ACMatchType::SEARCH_SUGGEST_ENTITY) {
+    // Likewise, if |second| is an entity suggestion and |first| isn't, first
+    // is not considered better, even if it has a higher type-adjusted
+    // relevance. If it does have a higher relevance, boost |second|.
+    if (compare_demote_by_type(first, second)) {
+      second.RecordAdditionalInfo(kACMatchPropertyScoreBoostedFrom,
+                                  second.relevance);
+      second.relevance = first.relevance;
+    }
+    return false;
+  }
+
+  // In the case that both values are entity suggestions or both aren't, |first|
+  // is simply considered better if it's type-adjusted relevance is higher.
+  return compare_demote_by_type(first, second);
+}
+
 // static
 bool AutocompleteResult::HasMatchByDestination(const AutocompleteMatch& match,
                                                const ACMatches& matches) {

components/omnibox/browser/autocomplete_result.h

@@ -135,8 +135,22 @@ class AutocompleteResult {
   size_t EstimateMemoryUsage() const;
 
  private:
-  friend class AutocompleteProviderTest;
   FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest, ConvertsOpenTabsCorrectly);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchEntityWithHigherRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchEntityWithLowerRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchEntityWithEqualRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchNonEntityWithHigherRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchNonEntityWithLowerRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchNonEntityWithEqualRelevance);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest, IsBetterMatchBothEntities);
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest,
+                           IsBetterMatchBothNonEntities);
 
   typedef std::map<AutocompleteProvider*, ACMatches> ProviderToMatches;
 

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -1002,3 +1002,163 @@ TEST_F(AutocompleteResultTest, ConvertsOpenTabsCorrectly) {
   EXPECT_TRUE(result.match_at(1)->has_tab_match);
   EXPECT_FALSE(result.match_at(2)->has_tab_match);
 }
+
+namespace {
+
+void CheckRelevanceExpectations(const AutocompleteMatch& first,
+                                const AutocompleteMatch& second,
+                                int first_expected_relevance,
+                                int second_expected_relevance,
+                                const char* first_expected_boosted_from,
+                                const char* second_expected_boosted_from) {
+  EXPECT_EQ(first_expected_relevance, first.relevance);
+  EXPECT_EQ(second_expected_relevance, second.relevance);
+  EXPECT_EQ(std::string(first_expected_boosted_from),
+            first.GetAdditionalInfo(kACMatchPropertyScoreBoostedFrom));
+  EXPECT_EQ(std::string(second_expected_boosted_from),
+            second.GetAdditionalInfo(kACMatchPropertyScoreBoostedFrom));
+}
+
+}  // namespace
+
+TEST_F(AutocompleteResultTest, IsBetterMatchEntityWithHigherRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  second.relevance = 600;
+
+  // Expect the entity suggestion to be better and its relevance unchanged.
+  // HOME_PAGE is used here because it doesn't trigger the special logic in
+  // OmniboxFieldTrial::GetDemotionsByType. There should otherwise be no
+  // demotions since the field trial params are cleared in the test setup.
+  EXPECT_TRUE(AutocompleteResult::IsBetterMatch(first, second,
+                                                OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 600, "", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchEntityWithLowerRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  first.relevance = 600;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  second.relevance = 1000;
+
+  // Expect the entity suggestion to be better and its relevance to have been
+  // boosted to that of the non-entity suggestion.
+  EXPECT_TRUE(AutocompleteResult::IsBetterMatch(first, second,
+                                                OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 1000, "600", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchEntityWithEqualRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  second.relevance = 1000;
+
+  // Expect the entity suggestion to be better and the relevance scores
+  // unchanged.
+  EXPECT_TRUE(AutocompleteResult::IsBetterMatch(first, second,
+                                                OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 1000, "", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchNonEntityWithHigherRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  second.relevance = 600;
+
+  // Expect the non-entity suggestion to *not* be better and the relevance of
+  // the entity suggestion to have been boosted.
+  EXPECT_FALSE(AutocompleteResult::IsBetterMatch(first, second,
+                                                 OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 1000, "", "600");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchNonEntityWithLowerRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  first.relevance = 600;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  second.relevance = 1000;
+
+  // Expect the non-entity suggestion to *not* be better and the relevance
+  // scores unchanged.
+  EXPECT_FALSE(AutocompleteResult::IsBetterMatch(first, second,
+                                                 OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 600, 1000, "", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchNonEntityWithEqualRelevance) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  second.relevance = 1000;
+
+  // Expect the non-entity suggestion to *not* be better and the relevance
+  // scores unchanged.
+  EXPECT_FALSE(AutocompleteResult::IsBetterMatch(first, second,
+                                                 OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 1000, "", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchBothEntities) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
+  second.relevance = 600;
+
+  // Expect the first suggestion to be better since its relevance is higher and
+  // the relevance scores unchanged.
+  EXPECT_TRUE(AutocompleteResult::IsBetterMatch(first, second,
+                                                OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 600, "", "");
+
+  // Expect the reversed condition to be false and the relevance scores
+  // unchanged.
+  EXPECT_FALSE(AutocompleteResult::IsBetterMatch(second, first,
+                                                 OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 600, "", "");
+}
+
+TEST_F(AutocompleteResultTest, IsBetterMatchBothNonEntities) {
+  AutocompleteMatch first;
+  first.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  first.relevance = 1000;
+
+  AutocompleteMatch second;
+  second.type = AutocompleteMatchType::SEARCH_SUGGEST;
+  second.relevance = 600;
+
+  // Expect the first suggestion to be better since its relevance is higher and
+  // the relevance scores unchanged.
+  EXPECT_TRUE(AutocompleteResult::IsBetterMatch(first, second,
+                                                OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 600, "", "");
+
+  // Expect the reversed condition to be false and the relevance scores
+  // unchanged.
+  EXPECT_FALSE(AutocompleteResult::IsBetterMatch(second, first,
+                                                 OmniboxEventProto::HOME_PAGE));
+  CheckRelevanceExpectations(first, second, 1000, 600, "", "");
+}