[omnibox]: Cache doc suggestions' formatting.

1) The drive server may intermittently provide a particular suggestion
while typing. E.g. the input 'fluffy' may suggest a doc titled 'fluffy
kitten', while the input 'fluffy k' may not.

2) Drive suggests return asynchronously.

As a consequence of the above 2, when the drive server and bookmark or
history providers suggest the same document, the suggestion displayed
flickers between the preferred drive format (e.g. 'title - date -
doc_type') and the typical URL format (e.g. 'title - url').

This CL caches the 20 most recent drive suggestions to alleviate the
issue. Cached suggestions are scored 0 to avoid affecting the
autocomplete results displayed and their rankings. I.e., this CL is
cosmetic.

Bug: 941548
Change-Id: I8498642b89db463c968c66e3f2181da83e7b623f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1576177
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682892}

components/omnibox/browser/document_provider.cc

@@ -281,8 +281,9 @@ int BoostOwned(const int score,
 // static
 DocumentProvider* DocumentProvider::Create(
     AutocompleteProviderClient* client,
-    AutocompleteProviderListener* listener) {
-  return new DocumentProvider(client, listener);
+    AutocompleteProviderListener* listener,
+    size_t cache_size) {
+  return new DocumentProvider(client, listener, cache_size);
 }
 
 // static
@@ -379,15 +380,17 @@ void DocumentProvider::Start(const AutocompleteInput& input,
     return;
   }
 
-  // We currently only provide asynchronous matches.
+  Stop(false, false);
+
+  input_ = input;
+
+  // Return cached suggestions synchronously.
+  CopyCachedMatchesToMatches();
+
   if (!input.want_asynchronous_matches()) {
     return;
   }
 
-  Stop(true, false);
-
-  input_ = input;
-
   // Create a request for suggestions, routing completion to
   base::BindOnce(&DocumentProvider::OnDocumentSuggestionsLoaderAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
@@ -456,13 +459,16 @@ void DocumentProvider::ResetSession() {
 }
 
 DocumentProvider::DocumentProvider(AutocompleteProviderClient* client,
-                                   AutocompleteProviderListener* listener)
+                                   AutocompleteProviderListener* listener,
+                                   size_t cache_size)
     : AutocompleteProvider(AutocompleteProvider::TYPE_DOCUMENT),
       field_trial_triggered_(false),
       field_trial_triggered_in_session_(false),
       backoff_for_session_(false),
       client_(client),
-      listener_(listener) {}
+      listener_(listener),
+      cache_size_(cache_size),
+      matches_cache_(MatchesCache::NO_AUTO_EVICT) {}
 
 DocumentProvider::~DocumentProvider() {}
 
@@ -491,7 +497,13 @@ bool DocumentProvider::UpdateResults(const std::string& json_data) {
   if (!response)
     return false;
 
-  return ParseDocumentSearchResults(*response, &matches_);
+  matches_ = ParseDocumentSearchResults(*response);
+  for (auto it = matches_.rbegin(); it != matches_.rend(); ++it)
+    matches_cache_.Put(it->stripped_destination_url, *it);
+  CopyCachedMatchesToMatches(matches_.size());
+  matches_cache_.ShrinkToSize(cache_size_);
+
+  return !matches_.empty();
 }
 
 void DocumentProvider::OnDocumentSuggestionsLoaderAvailable(
@@ -544,12 +556,13 @@ base::string16 GetProductDescriptionString(const std::string& mimetype) {
   return l10n_util::GetStringUTF16(IDS_DRIVE_SUGGESTION_GENERAL);
 }
 
-bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
-                                                  ACMatches* matches) {
+ACMatches DocumentProvider::ParseDocumentSearchResults(
+    const base::Value& root_val) {
+  ACMatches matches;
   const base::DictionaryValue* root_dict = nullptr;
   const base::ListValue* results_list = nullptr;
   if (!root_val.GetAsDictionary(&root_dict)) {
-    return false;
+    return matches;
   }
 
   // The server may ask the client to back off, in which case we back off for
@@ -557,12 +570,12 @@ bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
   // TODO(skare): Respect retryDelay if provided, ideally by calling via gRPC.
   if (ResponseContainsBackoffSignal(root_dict)) {
     backoff_for_session_ = true;
-    return false;
+    return matches;
   }
 
   // Otherwise parse the results.
   if (!root_dict->GetList("results", &results_list)) {
-    return false;
+    return matches;
   }
   size_t num_results = results_list->GetSize();
   UMA_HISTOGRAM_COUNTS_1M("Omnibox.DocumentSuggest.ResultCount", num_results);
@@ -597,17 +610,15 @@ bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
   bool in_counterfactual_group = base::GetFieldTrialParamByFeatureAsBool(
       omnibox::kDocumentProvider, "DocumentProviderCounterfactualArm", false);
 
-  // Clear the previous results now that new results are available.
-  matches->clear();
   // Ensure server's suggestions are added with monotonically decreasing scores.
   int previous_score = INT_MAX;
   for (size_t i = 0; i < num_results; i++) {
-    if (matches->size() >= provider_max_matches_) {
+    if (matches.size() >= provider_max_matches_) {
       break;
     }
     const base::DictionaryValue* result = nullptr;
     if (!results_list->GetDictionary(i, &result)) {
-      return false;
+      return matches;
     }
     base::string16 title;
     base::string16 url;
@@ -686,12 +697,25 @@ bool DocumentProvider::ParseDocumentSearchResults(const base::Value& root_val,
     if (snippet)
       match.RecordAdditionalInfo("snippet", *snippet);
     if (!in_counterfactual_group) {
-      matches->push_back(match);
+      matches.push_back(match);
     }
     field_trial_triggered_ = true;
     field_trial_triggered_in_session_ = true;
   }
-  return true;
+  return matches;
+}
+
+void DocumentProvider::CopyCachedMatchesToMatches(
+    size_t skip_n_most_recent_matches) {
+  std::for_each(std::next(matches_cache_.begin(), skip_n_most_recent_matches),
+                matches_cache_.end(), [this](const auto& cache_key_match_pair) {
+                  auto match = cache_key_match_pair.second;
+                  match.relevance = 0;
+                  match.contents_class =
+                      DocumentProvider::Classify(match.contents, input_.text());
+                  match.RecordAdditionalInfo("from cache", "true");
+                  matches_.push_back(match);
+                });
 }
 
 // static

components/omnibox/browser/document_provider.h

@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/compiler_specific.h"
+#include "base/containers/mru_cache.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "components/history/core/browser/history_types.h"
@@ -42,7 +43,8 @@ class DocumentProvider : public AutocompleteProvider {
  public:
   // Creates and returns an instance of this provider.
   static DocumentProvider* Create(AutocompleteProviderClient* client,
-                                  AutocompleteProviderListener* listener);
+                                  AutocompleteProviderListener* listener,
+                                  size_t cache_size = 20);
 
   // AutocompleteProvider:
   void Start(const AutocompleteInput& input, bool minimal_changes) override;
@@ -54,6 +56,18 @@ class DocumentProvider : public AutocompleteProvider {
   // Registers a client-side preference to enable document suggestions.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+  // Returns a set of classifications that highlight all the occurrences of
+  // |input_text| at word breaks in |text|. E.g., given |input_text|
+  // "rain if you dare" and |text| "how to tell if your kitten is a rainbow",
+  // will return the classifications:
+  //             __ ___              ____
+  // how to tell if your kitten is a rainbow
+  // ^           ^ ^^   ^            ^  ^
+  // NONE        M |M   |            |  NONE
+  //               NONE NONE         MATCH
+  static ACMatchClassifications Classify(const base::string16& input_text,
+                                         const base::string16& text);
+
   // Builds a GURL to use for deduping against other document/history
   // suggestions. Multiple URLs may refer to the same document.
   // Returns an empty GURL if not a recognized Docs URL.
@@ -89,9 +103,13 @@ class DocumentProvider : public AutocompleteProvider {
                            ParseDocumentSearchResultsWithIneligibleFlag);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, GenerateLastModifiedString);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Scoring);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Caching);
+
+  using MatchesCache = base::MRUCache<GURL, AutocompleteMatch>;
 
   DocumentProvider(AutocompleteProviderClient* client,
-                   AutocompleteProviderListener* listener);
+                   AutocompleteProviderListener* listener,
+                   size_t cache_size);
 
   ~DocumentProvider() override;
 
@@ -118,8 +136,15 @@ class DocumentProvider : public AutocompleteProvider {
 
   // Parses document search result JSON.
   // Returns true if |matches| was populated with fresh suggestions.
-  bool ParseDocumentSearchResults(const base::Value& root_val,
-                                  ACMatches* matches);
+  ACMatches ParseDocumentSearchResults(const base::Value& root_val);
+
+  // Appends |matches_cache_| to |matches_|. Updates their classifications
+  // according to |input_.text()| and sets their relevance to 0.
+  // |skip_n_most_recent_matches| indicates the number of cached matches already
+  // in |matches_|. E.g. if the drive server responded with 3 docs, these 3 docs
+  // are added both to |matches_| and |matches_cache| prior to invoking
+  // |AddCachedMatches| in order to avoid duplicate matches.
+  void CopyCachedMatchesToMatches(size_t skip_n_most_recent_matches = 0);
 
   // Generates the localized last-modified timestamp to present to the user.
   // Full date for old files, mm/dd within the same calendar year, or time-of-
@@ -129,18 +154,6 @@ class DocumentProvider : public AutocompleteProvider {
       const std::string& modified_timestamp_string,
       base::Time now);
 
-  // Returns a set of classifications that highlight all the occurrences of
-  // |input_text| at word breaks in |text|. E.g., given |input_text|
-  // "rain if you dare" and |text| "how to tell if your kitten is a rainbow",
-  // will return the classifications:
-  //             __ ___              ____
-  // how to tell if your kitten is a rainbow
-  // ^           ^ ^^   ^            ^  ^
-  // NONE        M |M   |            |  NONE
-  //               NONE NONE         MATCH
-  static ACMatchClassifications Classify(const base::string16& input_text,
-                                         const base::string16& text);
-
   // Whether a field trial has triggered for this query and this session,
   // respectively. Works similarly to BaseSearchProvider, though this class does
   // not inherit from it.
@@ -164,6 +177,16 @@ class DocumentProvider : public AutocompleteProvider {
   // Loader used to retrieve results.
   std::unique_ptr<network::SimpleURLLoader> loader_;
 
+  // Because the drive server is async and may intermittently provide a
+  // particular suggestion for consecutive inputs, without caching, doc
+  // suggestions flicker between drive format (title - date - doc_type) and URL
+  // format (title - URL) suggestions from the history and bookmark providers.
+  // Appending cached doc suggestions with relevance 0 ensures cached
+  // suggestions only display if deduped with a non-cached suggestion and do not
+  // affect which autocomplete results are displayed and their ranks.
+  const size_t cache_size_;
+  MatchesCache matches_cache_;
+
   // For callbacks that may be run after destruction. Must be declared last.
   base::WeakPtrFactory<DocumentProvider> weak_ptr_factory_{this};
 

components/omnibox/browser/document_provider_unittest.cc

@@ -97,7 +97,7 @@ void DocumentProviderTest::SetUp() {
   default_template_url_ = turl_model->Add(std::make_unique<TemplateURL>(data));
   turl_model->SetUserSelectedDefaultSearchProvider(default_template_url_);
 
-  provider_ = DocumentProvider::Create(client_.get(), this);
+  provider_ = DocumentProvider::Create(client_.get(), this, 4);
 }
 
 void DocumentProviderTest::OnProviderUpdate(bool updated_matches) {
@@ -259,8 +259,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) {
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*response);
   EXPECT_EQ(matches.size(), 2u);
 
   EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1"));
@@ -320,8 +319,7 @@ TEST_F(DocumentProviderTest, ProductDescriptionStringsAndAccessibleLabels) {
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*response);
   EXPECT_EQ(matches.size(), 3u);
 
   // match.destination_url is used as the match's temporary text in the Omnibox.
@@ -376,8 +374,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) {
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*response);
   EXPECT_EQ(matches.size(), 3u);
 
   // Server is suggesting relevances of [1234, 1234, 1234]
@@ -432,8 +429,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*response);
   EXPECT_EQ(matches.size(), 3u);
 
   // Server is suggesting relevances of [1233, 1234, 1233, 1000, 1000]
@@ -490,8 +486,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) {
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*response);
   EXPECT_EQ(matches.size(), 3u);
 
   // Server is suggesting relevances of [1, 1, 1]
@@ -540,8 +535,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithBackoff) {
   ASSERT_TRUE(backoff_response);
   ASSERT_TRUE(backoff_response->is_dict());
 
-  ACMatches matches;
-  provider_->ParseDocumentSearchResults(*backoff_response, &matches);
+  ACMatches matches = provider_->ParseDocumentSearchResults(*backoff_response);
   ASSERT_TRUE(provider_->backoff_for_session_);
 }
 
@@ -574,7 +568,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithIneligibleFlag) {
       kMismatchedMessageJSON, base::JSON_ALLOW_TRAILING_COMMAS);
   ASSERT_TRUE(bad_response);
   ASSERT_TRUE(bad_response->is_dict());
-  provider_->ParseDocumentSearchResults(*bad_response, &matches);
+  matches = provider_->ParseDocumentSearchResults(*bad_response);
   ASSERT_FALSE(provider_->backoff_for_session_);
 
   // Now parse a response that does trigger backoff.
@@ -582,7 +576,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithIneligibleFlag) {
       kIneligibleJSONResponse, base::JSON_ALLOW_TRAILING_COMMAS);
   ASSERT_TRUE(backoff_response);
   ASSERT_TRUE(backoff_response->is_dict());
-  provider_->ParseDocumentSearchResults(*backoff_response, &matches);
+  matches = provider_->ParseDocumentSearchResults(*backoff_response);
   ASSERT_TRUE(provider_->backoff_for_session_);
 }
 
@@ -700,8 +694,7 @@ TEST_F(DocumentProviderTest, Scoring) {
                                                     parameters);
     base::Optional<base::Value> response = base::JSONReader::Read(response_str);
     provider_->input_.UpdateText(base::UTF8ToUTF16(input_text), 0, {});
-    ACMatches matches;
-    provider_->ParseDocumentSearchResults(*response, &matches);
+    ACMatches matches = provider_->ParseDocumentSearchResults(*response);
 
     EXPECT_EQ(matches.size(), expected_scores.size())
         << "invocation " << invocation;
@@ -789,3 +782,103 @@ TEST_F(DocumentProviderTest, Scoring) {
         ]})",
       "rain bow", {669, 669, 793});
 }
+
+TEST_F(DocumentProviderTest, Caching) {
+  auto MakeTestResponse = [](const std::vector<std::string>& doc_ids) {
+    std::string results = "";
+    for (auto doc_id : doc_ids)
+      results += base::StringPrintf(
+          R"({
+              "title": "Document %s",
+              "score": 1150,
+              "url": "https://drive.google.com/open?id=%s",
+              "originalUrl": "https://drive.google.com/open?id=%s",
+            },)",
+          doc_id.c_str(), doc_id.c_str(), doc_id.c_str());
+    return base::StringPrintf(R"({"results": [%s]})", results.c_str());
+  };
+
+  auto GetTestProviderMatches = [this](const std::string& input_text,
+                                       const std::string& response_str) {
+    provider_->input_.UpdateText(base::UTF8ToUTF16(input_text), 0, {});
+    provider_->UpdateResults(response_str);
+    return provider_->matches_;
+  };
+
+  // Partially fill the cache as setup for following tests.
+  auto matches = GetTestProviderMatches("", MakeTestResponse({"0", "1", "2"}));
+  EXPECT_EQ(matches.size(), size_t(3));
+  EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0"));
+  EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 1"));
+  EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 2"));
+
+  // Cache should remove duplicates.
+  matches = GetTestProviderMatches("", MakeTestResponse({"1", "2", "3"}));
+  EXPECT_EQ(matches.size(), size_t(4));
+  EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 1"));
+  EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 2"));
+  EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 3"));
+  EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0"));
+
+  // Cache size (4) should not restrict number of matches from the current
+  // response.
+  matches = GetTestProviderMatches("", MakeTestResponse({"3", "4", "5"}));
+  EXPECT_EQ(matches.size(), size_t(6));
+  EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 3"));
+  EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4"));
+  EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 5"));
+  EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 1"));
+  EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 2"));
+  EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 0"));
+
+  // Cache size (4) should restrict number of cached matches appended.
+  matches = GetTestProviderMatches("", MakeTestResponse({"0", "4", "6"}));
+  EXPECT_EQ(matches.size(), size_t(6));
+  EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0"));
+  EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4"));
+  EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 6"));
+  EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 3"));
+  EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 5"));
+  EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 1"));
+
+  // Cached results should update match |additional_info|, |relevance|, and
+  // |contents_class|.
+  matches = GetTestProviderMatches("doc", MakeTestResponse({"5", "4", "7"}));
+  EXPECT_EQ(matches.size(), size_t(6));
+  EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 5"));
+  EXPECT_EQ(matches[0].GetAdditionalInfo("from cache"), "");
+  EXPECT_EQ(matches[0].relevance, 1150);
+  EXPECT_THAT(matches[0].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+  EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4"));
+  EXPECT_EQ(matches[1].GetAdditionalInfo("from cache"), "");
+  EXPECT_EQ(matches[1].relevance, 1149);
+  EXPECT_THAT(matches[1].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+  EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 7"));
+  EXPECT_EQ(matches[2].GetAdditionalInfo("from cache"), "");
+  EXPECT_EQ(matches[2].relevance, 1148);
+  EXPECT_THAT(matches[2].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+  EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0"));
+  EXPECT_EQ(matches[3].GetAdditionalInfo("from cache"), "true");
+  EXPECT_EQ(matches[3].relevance, 0);
+  EXPECT_THAT(matches[3].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+  EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 6"));
+  EXPECT_EQ(matches[4].GetAdditionalInfo("from cache"), "true");
+  EXPECT_EQ(matches[4].relevance, 0);
+  EXPECT_THAT(matches[4].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+  EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 3"));
+  EXPECT_EQ(matches[5].GetAdditionalInfo("from cache"), "true");
+  EXPECT_EQ(matches[5].relevance, 0);
+  EXPECT_THAT(matches[5].contents_class,
+              testing::ElementsAre(ACMatchClassification{0, 2},
+                                   ACMatchClassification{3, 0}));
+}