[Omnibox] Break ties on Document Suggestion results.

Bug: 923611
Change-Id: Ia99b06a381c955ebad0cb7b9fd5b104549c61fc2
Reviewed-on: https://chromium-review.googlesource.com/c/1423740
Commit-Queue: Travis Skare <skare@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625823}

components/omnibox/browser/document_provider.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
@@ -402,6 +403,10 @@ bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
 
   // Clear the previous results now that new results are available.
   matches->clear();
+  // Ensure server's suggestions are added with monotonically decreasing scores.
+  // When previous_score is >= 0, it enforces a maximum score for subsequent
+  // results.
+  int previous_score = -1;
   for (size_t i = 0; i < num_results; i++) {
     if (matches->size() >= AutocompleteProvider::kMaxMatches) {
       break;
@@ -433,8 +438,13 @@ bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
     }
     int server_score;
     if (result->GetInteger("score", &server_score)) {
+      if (previous_score >= 0 && server_score >= previous_score) {
+        server_score = previous_score - 1;
+      }
       relevance = server_score;
+      previous_score = relevance;
     }
+    relevance = std::max(relevance, 0);
     AutocompleteMatch match(this, relevance, false,
                             AutocompleteMatchType::DOCUMENT_SUGGESTION);
     // Use full URL for displayed text and navigation. Use "originalUrl" for

components/omnibox/browser/document_provider.h

@@ -68,6 +68,12 @@ class DocumentProvider : public AutocompleteProvider {
                            CheckFeaturePrerequisiteServerBackoff);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, IsInputLikelyURL);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, ParseDocumentSearchResults);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
+                           ParseDocumentSearchResultsBreakTies);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
+                           ParseDocumentSearchResultsBreakTiesCascade);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
+                           ParseDocumentSearchResultsBreakTiesZeroLimit);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
                            ParseDocumentSearchResultsWithBackoff);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,

components/omnibox/browser/document_provider_unittest.cc

@@ -265,6 +265,168 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) {
   ASSERT_FALSE(provider_->backoff_for_session_);
 }
 
+TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) {
+  const char kGoodJSONResponseWithTies[] = R"({
+      "results": [
+        {
+          "title": "Document 1",
+          "url": "https://documentprovider.tld/doc?id=1",
+          "score": 1234,
+          "originalUrl": "https://shortened.url"
+        },
+        {
+          "title": "Document 2",
+          "score": 1234,
+          "url": "https://documentprovider.tld/doc?id=2"
+        },
+        {
+          "title": "Document 3",
+          "score": 1234,
+          "url": "https://documentprovider.tld/doc?id=3"
+        }
+      ]
+     })";
+
+  std::unique_ptr<base::DictionaryValue> response = base::DictionaryValue::From(
+      base::JSONReader::Read(kGoodJSONResponseWithTies));
+  ASSERT_TRUE(response != nullptr);
+
+  ACMatches matches;
+  provider_->ParseDocumentSearchResults(*response, &matches);
+  EXPECT_EQ(matches.size(), 3u);
+
+  // Server is suggesting relevances of [1234, 1234, 1234]
+  // We should break ties to [1234, 1233, 1232]
+  EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1"));
+  EXPECT_EQ(matches[0].destination_url,
+            GURL("https://documentprovider.tld/doc?id=1"));
+  EXPECT_EQ(matches[0].relevance, 1234);  // As the server specified.
+  EXPECT_EQ(matches[0].stripped_destination_url, GURL("https://shortened.url"));
+
+  EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("Document 2"));
+  EXPECT_EQ(matches[1].destination_url,
+            GURL("https://documentprovider.tld/doc?id=2"));
+  EXPECT_EQ(matches[1].relevance, 1233);  // Tie demoted
+  EXPECT_TRUE(matches[1].stripped_destination_url.is_empty());
+
+  EXPECT_EQ(matches[2].contents, base::ASCIIToUTF16("Document 3"));
+  EXPECT_EQ(matches[2].destination_url,
+            GURL("https://documentprovider.tld/doc?id=3"));
+  EXPECT_EQ(matches[2].relevance, 1232);  // Tie demoted, twice.
+  EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
+
+  ASSERT_FALSE(provider_->backoff_for_session_);
+}
+
+TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
+  const char kGoodJSONResponseWithTies[] = R"({
+      "results": [
+        {
+          "title": "Document 1",
+          "url": "https://documentprovider.tld/doc?id=1",
+          "score": 1234,
+          "originalUrl": "https://shortened.url"
+        },
+        {
+          "title": "Document 2",
+          "score": 1234,
+          "url": "https://documentprovider.tld/doc?id=2"
+        },
+        {
+          "title": "Document 3",
+          "score": 1233,
+          "url": "https://documentprovider.tld/doc?id=3"
+        }
+      ]
+     })";
+
+  std::unique_ptr<base::DictionaryValue> response = base::DictionaryValue::From(
+      base::JSONReader::Read(kGoodJSONResponseWithTies));
+  ASSERT_TRUE(response != nullptr);
+
+  ACMatches matches;
+  provider_->ParseDocumentSearchResults(*response, &matches);
+  EXPECT_EQ(matches.size(), 3u);
+
+  // Server is suggesting relevances of [1233, 1234, 1233, 1000, 1000]
+  // We should break ties to [1234, 1233, 1232, 1000, 999]
+  EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1"));
+  EXPECT_EQ(matches[0].destination_url,
+            GURL("https://documentprovider.tld/doc?id=1"));
+  EXPECT_EQ(matches[0].relevance, 1234);  // As the server specified.
+  EXPECT_EQ(matches[0].stripped_destination_url, GURL("https://shortened.url"));
+
+  EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("Document 2"));
+  EXPECT_EQ(matches[1].destination_url,
+            GURL("https://documentprovider.tld/doc?id=2"));
+  EXPECT_EQ(matches[1].relevance, 1233);  // Tie demoted
+  EXPECT_TRUE(matches[1].stripped_destination_url.is_empty());
+
+  EXPECT_EQ(matches[2].contents, base::ASCIIToUTF16("Document 3"));
+  EXPECT_EQ(matches[2].destination_url,
+            GURL("https://documentprovider.tld/doc?id=3"));
+  // Document 2's demotion caused an implicit tie.
+  // Ensure we demote this one as well.
+  EXPECT_EQ(matches[2].relevance, 1232);
+  EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
+
+  ASSERT_FALSE(provider_->backoff_for_session_);
+}
+
+TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) {
+  const char kGoodJSONResponseWithTies[] = R"({
+      "results": [
+        {
+          "title": "Document 1",
+          "url": "https://documentprovider.tld/doc?id=1",
+          "score": 1,
+          "originalUrl": "https://shortened.url"
+        },
+        {
+          "title": "Document 2",
+          "score": 1,
+          "url": "https://documentprovider.tld/doc?id=2"
+        },
+        {
+          "title": "Document 3",
+          "score": 1,
+          "url": "https://documentprovider.tld/doc?id=3"
+        }
+      ]
+     })";
+
+  std::unique_ptr<base::DictionaryValue> response = base::DictionaryValue::From(
+      base::JSONReader::Read(kGoodJSONResponseWithTies));
+  ASSERT_TRUE(response != nullptr);
+
+  ACMatches matches;
+  provider_->ParseDocumentSearchResults(*response, &matches);
+  EXPECT_EQ(matches.size(), 3u);
+
+  // Server is suggesting relevances of [1, 1, 1]
+  // We should break ties, but not below zero, to [1, 0, 0]
+  EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1"));
+  EXPECT_EQ(matches[0].destination_url,
+            GURL("https://documentprovider.tld/doc?id=1"));
+  EXPECT_EQ(matches[0].relevance, 1);  // As the server specified.
+  EXPECT_EQ(matches[0].stripped_destination_url, GURL("https://shortened.url"));
+
+  EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("Document 2"));
+  EXPECT_EQ(matches[1].destination_url,
+            GURL("https://documentprovider.tld/doc?id=2"));
+  EXPECT_EQ(matches[1].relevance, 0);  // Tie demoted
+  EXPECT_TRUE(matches[1].stripped_destination_url.is_empty());
+
+  EXPECT_EQ(matches[2].contents, base::ASCIIToUTF16("Document 3"));
+  EXPECT_EQ(matches[2].destination_url,
+            GURL("https://documentprovider.tld/doc?id=3"));
+  // Tie is demoted further.
+  EXPECT_EQ(matches[2].relevance, 0);
+  EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
+
+  ASSERT_FALSE(provider_->backoff_for_session_);
+}
+
 TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithBackoff) {
   // Response where the server wishes to trigger backoff.
   const char kBackoffJSONResponse[] = R"({