Omnibox: Reduce Bolding Flicker in SearchProvider

chrome/browser/autocomplete/search_provider.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 84 matching lines @@
       if (i.IsWord()) {
         if (found_word)
           return true;
         found_word = true;
       }
     }
   }
   return false;
 }
 
-// Builds the match contents and classification for the contents, and updates
-// the given |AutocompleteMatch|.
-void SetAndClassifyMatchContents(const base::string16& query_string,
-                                 const base::string16& input_text,
-                                 const base::string16& match_contents,
-                                 AutocompleteMatch* match) {
-  match->contents = match_contents.empty() ? query_string : match_contents;
-
-  // We do intra-string highlighting for suggestions - the suggested segment
-  // will be highlighted, e.g. for input_text = "you" the suggestion may be
-  // "youtube", so we'll bold the "tube" section: you*tube*.
-  if (input_text != match_contents) {
-    size_t input_position = match->contents.find(input_text);
-    if (input_position == base::string16::npos) {
-      // The input text is not a substring of the query string, e.g. input
-      // text is "slasdot" and the query string is "slashdot", so we bold the
-      // whole thing.
-      match->contents_class.push_back(ACMatchClassification(
-          0, ACMatchClassification::MATCH));
-    } else {
-      // TODO(beng): ACMatchClassification::MATCH now seems to just mean
-      //             "bold" this. Consider modifying the terminology.
-      // We don't iterate over the string here annotating all matches because
-      // it looks odd to have every occurrence of a substring that may be as
-      // short as a single character highlighted in a query suggestion result,
-      // e.g. for input text "s" and query string "southwest airlines", it
-      // looks odd if both the first and last s are highlighted.
-      if (input_position != 0) {
-        match->contents_class.push_back(ACMatchClassification(
-            0, ACMatchClassification::MATCH));
-      }
-      match->contents_class.push_back(
-          ACMatchClassification(input_position, ACMatchClassification::NONE));
-      size_t next_fragment_position = input_position + input_text.length();
-      if (next_fragment_position < query_string.length()) {
-        match->contents_class.push_back(ACMatchClassification(
-            next_fragment_position, ACMatchClassification::MATCH));
-      }
-    }
-  } else {
-    // Otherwise, |match| is a verbatim (what-you-typed) match, either for the
-    // default provider or a keyword search provider.
-    match->contents_class.push_back(ACMatchClassification(
-        0, ACMatchClassification::NONE));
-  }
-}
-
 AutocompleteMatchType::Type GetAutocompleteMatchType(const std::string& type) {
   if (type == "ENTITY")
     return AutocompleteMatchType::SEARCH_SUGGEST_ENTITY;
   if (type == "INFINITE")
     return AutocompleteMatchType::SEARCH_SUGGEST_INFINITE;
   if (type == "PERSONALIZED")
     return AutocompleteMatchType::SEARCH_SUGGEST_PERSONALIZED;
   if (type == "PROFILE")
     return AutocompleteMatchType::SEARCH_SUGGEST_PROFILE;
   return AutocompleteMatchType::SEARCH_SUGGEST;
@@ skipping 93 matching lines @@
 SearchProvider::SuggestResult::SuggestResult(
     const base::string16& suggestion,
     AutocompleteMatchType::Type type,
     const base::string16& match_contents,
     const base::string16& annotation,
     const std::string& suggest_query_params,
     const std::string& deletion_url,
     bool from_keyword_provider,
     int relevance,
     bool relevance_from_server,
-    bool should_prefetch)
+    bool should_prefetch,
+    const base::string16& input_text)
     : Result(from_keyword_provider, relevance, relevance_from_server),
       suggestion_(suggestion),
       type_(type),
       match_contents_(match_contents),
       annotation_(annotation),
       suggest_query_params_(suggest_query_params),
       deletion_url_(deletion_url),
       should_prefetch_(should_prefetch) {
+  DCHECK(!match_contents_.empty());
+  CalculateContentsClass(true, input_text);
 }
 
 SearchProvider::SuggestResult::~SuggestResult() {
 }
 
+void SearchProvider::SuggestResult::CalculateContentsClass(
+    const bool allow_bolding_all,
+    const base::string16& input_text) {
+  size_t input_position = match_contents_.find(input_text);
+  if (!allow_bolding_all && (input_position == base::string16::npos)) {
+    // Bail if the code below to update the bolding would bold the whole
+    // string.  Note that the string may already be entirely bolded; if
+    // so, leave it as is.
+    return;
+  }
+  match_contents_class_.clear();
+  // We do intra-string highlighting for suggestions - the suggested segment
+  // will be highlighted, e.g. for input_text = "you" the suggestion may be
+  // "youtube", so we'll bold the "tube" section: you*tube*.
+  if (input_text != match_contents_) {
+    if (input_position == base::string16::npos) {
+      // The input text is not a substring of the query string, e.g. input
+      // text is "slasdot" and the query string is "slashdot", so we bold the
+      // whole thing.
+      match_contents_class_.push_back(ACMatchClassification(
+          0, ACMatchClassification::MATCH));
+    } else {
+      // TODO(beng): ACMatchClassification::MATCH now seems to just mean
+      //             "bold" this. Consider modifying the terminology.

[Anuj, 2014/01/06 23:23:38]

I think we can remove this TODO. Although it seems to mean just BOLD, the
Android implementation clearly doesn't follow this.

[Mark P, 2014/01/07 19:10:31]

I agree we can remove it.  Removed.

+      // We don't iterate over the string here annotating all matches because
+      // it looks odd to have every occurrence of a substring that may be as
+      // short as a single character highlighted in a query suggestion result,
+      // e.g. for input text "s" and query string "southwest airlines", it
+      // looks odd if both the first and last s are highlighted.
+      if (input_position != 0) {
+        match_contents_class_.push_back(ACMatchClassification(
+            0, ACMatchClassification::MATCH));
+      }
+      match_contents_class_.push_back(
+          ACMatchClassification(input_position, ACMatchClassification::NONE));
+      size_t next_fragment_position = input_position + input_text.length();
+      if (next_fragment_position < match_contents_.length()) {
+        match_contents_class_.push_back(ACMatchClassification(
+            next_fragment_position, ACMatchClassification::MATCH));
+      }
+    }
+  } else {
+    // Otherwise, match_contents_ is a verbatim (what-you-typed) match, either
+    // for the default provider or a keyword search provider.
+    match_contents_class_.push_back(ACMatchClassification(
+        0, ACMatchClassification::NONE));
+  }
+}
+
 bool SearchProvider::SuggestResult::IsInlineable(
     const base::string16& input) const {
   return StartsWith(suggestion_, input, false);
 }
 
 int SearchProvider::SuggestResult::CalculateRelevance(
     const AutocompleteInput& input,
     bool keyword_provider_requested) const {
   if (!from_keyword_provider_ && keyword_provider_requested)
     return 100;
@@ skipping 114 matching lines @@
     const TemplateURL* template_url,
     int accepted_suggestion,
     int omnibox_start_margin,
     bool append_extra_query_params) {
   AutocompleteMatch match(autocomplete_provider, suggestion.relevance(), false,
                           suggestion.type());
 
   if (!template_url)
     return match;
   match.keyword = template_url->keyword();
-
-  SetAndClassifyMatchContents(suggestion.suggestion(), input_text,
-                              suggestion.match_contents(), &match);
+  match.contents = suggestion.match_contents();
+  match.contents_class = suggestion.match_contents_class();
 
   if (!suggestion.annotation().empty())
     match.description = suggestion.annotation();
 
   match.allowed_to_be_default_match =
       (input_text == suggestion.match_contents());
 
   // When the user forced a query, we need to make sure all the fill_into_edit
   // values preserve that property.  Otherwise, if the user starts editing a
   // suggestion, non-Search results will suddenly appear.
@@ skipping 137 matching lines @@
     } else {
       // The current top result is a navigational suggestion.
       if (nav_it->IsInlineable(input))
         break;
       nav_it = navigation_results->erase(nav_it);
     }
   }
 }
 
 // static
+void SearchProvider::RecalculateMatchContentsClass(
+    const base::string16& input_text,
+    SuggestResults* suggest_results) {
+  for (SuggestResults::iterator sug_it = suggest_results->begin();
+       sug_it != suggest_results->end(); ++sug_it) {
+    sug_it->CalculateContentsClass(false, input_text);
+  }
+}
+
+// static
 int SearchProvider::CalculateRelevanceForKeywordVerbatim(
     AutocompleteInput::Type type,
     bool prefer_keyword) {
   // This function is responsible for scoring verbatim query matches
   // for non-extension keywords.  KeywordProvider::CalculateRelevance()
   // scores verbatim query matches for extension keywords, as well as
   // for keyword matches (i.e., suggestions of a keyword itself, not a
   // suggestion of a query on a keyword search engine).  These two
   // functions are currently in sync, but there's no reason we
   // couldn't decide in the future to score verbatim matches
@@ skipping 285 matching lines @@
        (!done_ &&
         input_.matches_requested() == AutocompleteInput::ALL_MATCHES)))
     return;
 
   // We can't keep running any previous query, so halt it.
   StopSuggest();
 
   // Remove existing results that cannot inline autocomplete the new input.
   RemoveAllStaleResults();
 
+  // Update the content classifications of remaining results so they look good
+  // against the current input.
+  RecalculateMatchContentsClass(
+      input_.text(), &default_results_.suggest_results);
+  if (!keyword_input_.text().empty()) {
+    RecalculateMatchContentsClass(keyword_input_.text(),
+                                  &keyword_results_.suggest_results);
+  }
+
   // We can't start a new query if we're only allowed synchronous results.
   if (input_.matches_requested() != AutocompleteInput::ALL_MATCHES)
     return;
 
   // To avoid flooding the suggest server, don't send a query until at
   // least 100 ms since the last query.
   base::TimeTicks next_suggest_time(time_suggest_request_sent_ +
       base::TimeDelta::FromMilliseconds(kMinimumTimeBetweenSuggestQueriesMs));
   base::TimeTicks now(base::TimeTicks::Now());
   if (now >= next_suggest_time) {
@@ skipping 290 matching lines @@
         base::string16 title;
         if (descriptions != NULL)
           descriptions->GetString(index, &title);
         results->navigation_results.push_back(NavigationResult(
             *this, url, title, is_keyword, relevance, true));
       }
     } else {
       AutocompleteMatchType::Type match_type = GetAutocompleteMatchType(type);
       bool should_prefetch = static_cast<int>(index) == prefetch_index;
       base::DictionaryValue* suggestion_detail = NULL;
       base::string16 match_contents = suggestion;

[Anuj, 2014/01/06 23:23:38]

Probably omit this assignment given the check you are adding below.

Also, remove the comment about bad data.

[Mark P, 2014/01/07 19:10:31]

I left the assignment because I moved the check into the if (suggestion_details)
block.
Left the comment about bad data because, now that the correction is in the
block, it's appropriate.

       base::string16 annotation;
       std::string suggest_query_params;
       std::string deletion_url;
 
       if (suggestion_details) {
         suggestion_details->GetDictionary(index, &suggestion_detail);
         if (suggestion_detail) {
           suggestion_detail->GetString("du", &deletion_url);
           suggestion_detail->GetString("title", &match_contents) ||
               suggestion_detail->GetString("t", &match_contents);
           suggestion_detail->GetString("annotation", &annotation) ||
               suggestion_detail->GetString("a", &annotation);
           suggestion_detail->GetString("query_params", &suggest_query_params) ||
               suggestion_detail->GetString("q", &suggest_query_params);
         }
       }
+      // Error correction for bad data from server.
+      if (match_contents.empty())
+        match_contents = suggestion;
 
       // TODO(kochi): Improve calculator suggestion presentation.
       results->suggest_results.push_back(SuggestResult(
           suggestion, match_type, match_contents, annotation,
           suggest_query_params, deletion_url, is_keyword, relevance, true,
-          should_prefetch));
+          should_prefetch, input_text));
     }
   }
 
   // Ignore suggested scores for non-keyword matches in keyword mode; if the
   // server is allowed to score these, it could interfere with the user's
   // ability to get good keyword results.
   const bool abandon_suggested_scores =
       !is_keyword && !providers_.keyword_provider().empty();
   // Apply calculated relevance scores to suggestions if a valid list was
   // not provided or we're abandoning suggested scores entirely.
@@ skipping 29 matching lines @@
 
   bool relevance_from_server;
   int verbatim_relevance = GetVerbatimRelevance(&relevance_from_server);
   int did_not_accept_default_suggestion =
       default_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
   if (verbatim_relevance > 0) {
     SuggestResult verbatim(
         input_.text(), AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
-        input_.text(), base::string16(), std::string(), std::string(),
-        false, verbatim_relevance, relevance_from_server, false);
+        input_.text(), base::string16(), std::string(), std::string(), false,
+        verbatim_relevance, relevance_from_server, false, input_.text());
     AddMatchToMap(verbatim, input_.text(), std::string(),
                   did_not_accept_default_suggestion, &map);
   }
   if (!keyword_input_.text().empty()) {
     const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
     // Note: in this provider, SEARCH_OTHER_ENGINE must correspond
     // to the keyword verbatim search query.  Do not create other matches
     // of type SEARCH_OTHER_ENGINE.
     if (keyword_url &&
         (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION)) {
       bool keyword_relevance_from_server;
       const int keyword_verbatim_relevance =
           GetKeywordVerbatimRelevance(&keyword_relevance_from_server);
       if (keyword_verbatim_relevance > 0) {
         SuggestResult verbatim(
             keyword_input_.text(), AutocompleteMatchType::SEARCH_OTHER_ENGINE,
             keyword_input_.text(), base::string16(), std::string(),
             std::string(), true, keyword_verbatim_relevance,
-            keyword_relevance_from_server, false);
+            keyword_relevance_from_server, false, keyword_input_.text());
         AddMatchToMap(verbatim, keyword_input_.text(), std::string(),
                       did_not_accept_keyword_suggestion, &map);
       }
     }
   }
   AddHistoryResultsToMap(keyword_history_results_, true,
                          did_not_accept_keyword_suggestion, &map);
   AddHistoryResultsToMap(default_history_results_, false,
                          did_not_accept_default_suggestion, &map);
 
@@ skipping 329 matching lines @@
       prevent_inline_autocomplete =
           !AutocompleteMatch::IsSearchType(match.type);
     }
 
     int relevance = CalculateRelevanceForHistory(
         i->time, is_keyword, !prevent_inline_autocomplete,
         prevent_search_history_inlining);
     scored_results.push_back(SuggestResult(
         i->term, AutocompleteMatchType::SEARCH_HISTORY, i->term,
         base::string16(), std::string(), std::string(), is_keyword, relevance,
-        false, false));
+        false, false, input_text));
   }
 
   // History returns results sorted for us.  However, we may have docked some
   // results' scores, so things are no longer in order.  Do a stable sort to get
   // things back in order without otherwise disturbing results with equal
   // scores, then force the scores to be unique, so that the order in which
   // they're shown is deterministic.
   std::stable_sort(scored_results.begin(), scored_results.end(),
                    CompareScoredResults());
   int last_relevance = 0;
@@ skipping 391 matching lines @@
   if (!OmniboxFieldTrial::InZeroSuggestFieldTrial() ||
       service == NULL ||
       !service->IsSyncEnabledAndLoggedIn() ||
       !sync_prefs.GetPreferredDataTypes(syncer::UserTypes()).Has(
           syncer::PROXY_TABS) ||
       service->GetEncryptedDataTypes().Has(syncer::SESSIONS))
     return false;
 
   return true;
 }