[omnibox] [doc|drive] Remember scores for sync, cached matches.

Drive responses have noticeable latency. To avoid matches switching
styling between drive-like (e.g. 'Title - Date - Owner - Google Docs')
and non-drive-like (e.g. 'Title - URL'), the provider caches previous
matches and returns them synchronously before the async matches from the
backend are ready.

Cached docs that no longer match the input, should not be displayed;
e.g. the match 'doc x' should not be displayed for input 'doc y' even
though it may be cached from the previous input 'doc '. Therefore,
cached matches previously had their scores set to 0; if still relevant,
they will inherit a non-0 score from a duplicate history or bookmark
match; otherwise, they'll keep a score of 0 and be hidden.

This had 2 problems that resulted in matches either appearing &
disappearing or swapping places during the sync (on keystroke) and async
(on backend response) phases: 1) There isn't always a duplicate match to
inherit a score from. 2) Ranking per history/bookmark scoring could
differ from the ranking per the max of the history/bookmark & document
scoring.

To address the above 2 issues, cached matches no longer have their
scores set to 0 during the async phase. To preserve the original intent,
cached matches will continue to have their scores set to 0 during the
sync phase.

Also groups the code controlling whether to run the document provider
into |IsDocumentProviderAllowed()|, should have no effect.

Also updates doc |stripped_destination_url| to use
|AutocompleteMatch::GURLToStrippedGURL()| (which invokes
|DocumentProvider::GetURLForDeduping()|) instead of calling the latter
directly. The former resorts to a generic striped URL computation if the
latter doc-specific computation doesn't succeed. This ensures a
non-empty |stripped_destination_url| which avoids overriding cache
entries.

Change-Id: I7f8b5c4935df40c0c8a937725f4fb99d93bdd637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2634131
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845704}

components/omnibox/browser/document_provider.cc

@@ -365,8 +365,10 @@ bool DocumentProvider::IsDocumentProviderAllowed(
       template_url_service->GetDefaultSearchProvider();
   if (default_provider == nullptr ||
       default_provider->GetEngineType(
-          template_url_service->search_terms_data()) != SEARCH_ENGINE_GOOGLE)
+          template_url_service->search_terms_data()) != SEARCH_ENGINE_GOOGLE) {
     return false;
+  }
+
   if (OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
       input.prefer_keyword()) {
     // If a keyword provider matches, and we're explicitly in keyword mode,
@@ -376,14 +378,31 @@ bool DocumentProvider::IsDocumentProviderAllowed(
     const TemplateURL* keyword_provider =
         KeywordProvider::GetSubstitutingTemplateURLForInput(
             template_url_service, &keyword_input);
-    if (keyword_provider == nullptr)
-      return true;
-    // True if not explicitly in keyword mode, or a Drive suggestion.
-    return !IsExplicitlyInKeywordMode(input, keyword_provider->keyword()) ||
-           base::StartsWith(input.text(),
-                            base::ASCIIToUTF16("drive.google.com"),
-                            base::CompareCase::SENSITIVE);
+    if (keyword_provider &&
+        IsExplicitlyInKeywordMode(input, keyword_provider->keyword()) &&
+        !base::StartsWith(input.text(), base::ASCIIToUTF16("drive.google.com"),
+                          base::CompareCase::SENSITIVE)) {
+      return false;
+    }
+  }
+
+  // There should be no document suggestions fetched for on-focus suggestion
+  // requests, or if the input is empty.
+  if (input.focus_type() != OmniboxFocusType::DEFAULT ||
+      input.type() == metrics::OmniboxInputType::EMPTY) {
+    return false;
   }
+
+  // Experiment: don't issue queries for inputs under some length.
+  if (!WithinBounds(input.text().length(), min_query_length_,
+                    max_query_length_)) {
+    return false;
+  }
+
+  // Don't issue queries for input likely to be a URL.
+  if (IsInputLikelyURL(input))
+    return false;
+
   return true;
 }
 
@@ -417,31 +436,15 @@ void DocumentProvider::Start(const AutocompleteInput& input,
 
   // Perform various checks - feature is enabled, user is allowed to use the
   // feature, we're not under backoff, etc.
-  if (!IsDocumentProviderAllowed(client_, input)) {
+  if (!IsDocumentProviderAllowed(client_, input))
     return;
-  }
-
-  // There should be no document suggestions fetched for on-focus suggestion
-  // requests, or if the input is empty.
-  if (input.focus_type() != OmniboxFocusType::DEFAULT ||
-      input.type() == metrics::OmniboxInputType::EMPTY) {
-    return;
-  }
-
-  // Experiment: don't issue queries for inputs under some length.
-  if (!WithinBounds(input.text().length(), min_query_length_,
-                    max_query_length_))
-    return;
-
-  // Don't issue queries for input likely to be a URL.
-  if (IsInputLikelyURL(input)) {
-    return;
-  }
 
   input_ = input;
 
-  // Return cached suggestions synchronously.
+  // Return cached suggestions synchronously after setting the relevance of any
+  // beyond |provider_max_matches_| to 0.
   CopyCachedMatchesToMatches();
+  DemoteMatchesBeyondMax();
 
   if (!input.want_asynchronous_matches()) {
     return;
@@ -600,11 +603,30 @@ bool DocumentProvider::UpdateResults(const std::string& json_data) {
   if (!response)
     return false;
 
+  // 1) Fill |matches_| with <N> new server matches.
   matches_ = ParseDocumentSearchResults(*response);
+  // 2) Clear cached matches' scores to ensure cached matches for all but the
+  // previous input can only be shown if deduped. E.g., this allows matches for
+  // the input 'pari' to be displayed synchronously for the input 'paris', but
+  // be hidden if the user clears their input and starts anew 'london'.
+  SetCachedMatchesScoresTo0();
+  // 3) Push the <N> new matches to the cache.
   for (auto it = matches_.rbegin(); it != matches_.rend(); ++it)
     matches_cache_.Put(it->stripped_destination_url, *it);
+  // 4) Copy the cached matches to |matches_|, skipping the most recent <N>
+  // cached matches since they were already added in step (1). Pass
+  // |set_scores_to_0| as true as we don't trust cached scores since they may no
+  // longer match the current input; if the cached matches were still relevant,
+  // they would have been returned from the server again.
   CopyCachedMatchesToMatches(matches_.size());
+  // 5) Only now can we shrink the cache to |cache_size_|. Doing this
+  // automatically when pushing the new matches to the cache would reduce it's
+  // effective size, especially if the server returns close to |cache_size_|
+  // matches.
   matches_cache_.ShrinkToSize(cache_size_);
+  // 6) Limit matches to |provider_max_matches_| unless used for deduping; i.e.
+  // set the scores of matches beyond the limit to 0.
+  DemoteMatchesBeyondMax();
 
   return !matches_.empty();
 }
@@ -779,18 +801,14 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
     int server_score = 0;
     result->GetInteger("score", &server_score);
     int score = 0;
-    // Set |score| only if we haven't surpassed |provider_max_matches_| yet.
-    // Otherwise, score the remaining matches 0 to avoid displaying them except
-    // when deduped with history, shortcut, or bookmark matches.
-    if (matches.size() < provider_max_matches_) {
+
     if (use_client_score && use_server_score)
       score = std::min(client_score, server_score);
     else
       score = use_client_score ? client_score : server_score;
 
     if (cap_score_per_rank) {
-        int score_cap =
-            i < score_caps.size() ? score_caps[i] : score_caps.back();
+      int score_cap = i < score_caps.size() ? score_caps[i] : score_caps.back();
       score = std::min(score, score_cap);
     }
 
@@ -803,7 +821,6 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
     if (!use_client_score && score >= previous_score)
       score = std::max(previous_score - 1, 0);
     previous_score = score;
-    }
 
     AutocompleteMatch match(this, score, false,
                             AutocompleteMatchType::DOCUMENT_SUGGESTION);
@@ -813,10 +830,17 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
     match.destination_url = GURL(url);
     base::string16 original_url;
     if (result->GetString("originalUrl", &original_url)) {
-      GURL stripped_url = GetURLForDeduping(GURL(original_url));
-      if (stripped_url.is_valid())
-        match.stripped_destination_url = stripped_url;
+      // |AutocompleteMatch::GURLToStrippedGURL()| will try to use
+      // |GetURLForDeduping()| to extract a doc ID and generate a canonical doc
+      // URL; this is ideal as it handles different URL formats pointing to the
+      // same doc. Otherwise, it'll resort to the typical stripped URL
+      // generation that can still be used for generic deduping and as a key to
+      // |matches_cache_|.
+      match.stripped_destination_url = AutocompleteMatch::GURLToStrippedGURL(
+          GURL(original_url), input_, client_->GetTemplateURLService(),
+          base::string16());
     }
+
     match.contents = AutocompleteMatch::SanitizeString(title);
     match.contents_class = Classify(match.contents, input_.text());
     const base::DictionaryValue* metadata = nullptr;
@@ -874,9 +898,8 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
 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) {
+                matches_cache_.end(), [&](const auto& cache_key_match_pair) {
                   auto match = cache_key_match_pair.second;
-                  match.relevance = 0;
                   match.allowed_to_be_default_match = false;
                   match.TryRichAutocompletion(
                       base::UTF8ToUTF16(match.destination_url.spec()),
@@ -888,6 +911,18 @@ void DocumentProvider::CopyCachedMatchesToMatches(
                 });
 }
 
+void DocumentProvider::SetCachedMatchesScoresTo0() {
+  std::for_each(matches_cache_.begin(), matches_cache_.end(),
+                [&](auto& cache_key_match_pair) {
+                  cache_key_match_pair.second.relevance = 0;
+                });
+}
+
+void DocumentProvider::DemoteMatchesBeyondMax() {
+  for (size_t i = provider_max_matches_; i < matches_.size(); ++i)
+    matches_[i].relevance = 0;
+}
+
 // static
 ACMatchClassifications DocumentProvider::Classify(
     const base::string16& text,

components/omnibox/browser/document_provider.h

@@ -76,19 +76,7 @@ class DocumentProvider : public AutocompleteProvider {
   static const GURL GetURLForDeduping(const GURL& url);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, CheckFeatureBehindFlag);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeaturePrerequisiteNoIncognito);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeaturePrerequisiteNoSync);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeaturePrerequisiteClientSettingOff);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeaturePrerequisiteDefaultSearch);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeatureNotInExplicitKeywordMode);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
-                           CheckFeaturePrerequisiteServerBackoff);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, IsDocumentProviderAllowed);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, IsInputLikelyURL);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, ParseDocumentSearchResults);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
@@ -104,8 +92,8 @@ class DocumentProvider : public AutocompleteProvider {
                            ParseDocumentSearchResultsWithBadResponse);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, GenerateLastModifiedString);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Scoring);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Caching);
-  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, MinQueryLength);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, CachingForAsyncMatches);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, CachingForSyncMatches);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, StartCallsStop);
 
   using MatchesCache = base::MRUCache<GURL, AutocompleteMatch>;
@@ -151,11 +139,25 @@ class DocumentProvider : public AutocompleteProvider {
   // 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.
+  //   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
+  //   |CopyCachedMatchesToMatches()| in order to avoid duplicate matches.
   void CopyCachedMatchesToMatches(size_t skip_n_most_recent_matches = 0);
 
+  // Sets the scores of all cached matches to 0. This is invoked before pushing
+  // the latest async response returns so that the scores aren't preserved for
+  // further inputs. E.g., the input 'london' shouldn't display cached docs from
+  // a previous input 'paris'. This can't be done by automatically (i.e. set
+  // scores to 0 before pushing to the cache), as the scores are needed for the
+  // async pass if the user continued their input.
+  void SetCachedMatchesScoresTo0();
+
+  // Sets the scores of matches beyond the first |provider_max_matches_| to 0.
+  // This ensures the doc provider doesn't exceed it's allocated suggestions
+  // while also allowing docs from other providers to be deduped and styled like
+  // docs from the doc provider.
+  void DemoteMatchesBeyondMax();
+
   // 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-
   // day if a file was modified on the same date.