Omnibox: Reduce Bolding Flicker in SearchProvider

Do this by computing bolding at the time results are fetched from the
suggest server and keeping that same bolding regardless of what
keys the user presses later.

At the moment, this change only affects query suggestions, not
navsuggestions.

BUG=259486

Review URL: https://codereview.chromium.org/23164011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243998 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autocomplete/search_provider.cc

@@ -102,53 +102,6 @@ bool HasMultipleWords(const base::string16& text) {
   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;
@@ -262,7 +215,8 @@ SearchProvider::SuggestResult::SuggestResult(
     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),
@@ -271,11 +225,60 @@ SearchProvider::SuggestResult::SuggestResult(
       suggest_query_params_(suggest_query_params),
       deletion_url_(deletion_url),
       should_prefetch_(should_prefetch) {
+  DCHECK(!match_contents_.empty());
+  ClassifyMatchContents(true, input_text);
 }
 
 SearchProvider::SuggestResult::~SuggestResult() {
 }
 
+void SearchProvider::SuggestResult::ClassifyMatchContents(
+    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 {
+      // 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);
@@ -410,9 +413,8 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
   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();
@@ -569,6 +571,15 @@ void SearchProvider::RemoveStaleResults(const base::string16& input,
   }
 }
 
+// static
+void SearchProvider::UpdateMatchContentsClass(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->ClassifyMatchContents(false, input_text);
+  }
+}
+
 // static
 int SearchProvider::CalculateRelevanceForKeywordVerbatim(
     AutocompleteInput::Type type,
@@ -875,6 +886,14 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
   // 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.
+  UpdateMatchContentsClass(input_.text(), &default_results_.suggest_results);
+  if (!keyword_input_.text().empty()) {
+    UpdateMatchContentsClass(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;
@@ -1196,6 +1215,9 @@ bool SearchProvider::ParseSuggestResults(base::Value* root_val,
           suggestion_detail->GetString("du", &deletion_url);
           suggestion_detail->GetString("title", &match_contents) ||
               suggestion_detail->GetString("t", &match_contents);
+          // Error correction for bad data from server.
+          if (match_contents.empty())
+            match_contents = suggestion;
           suggestion_detail->GetString("annotation", &annotation) ||
               suggestion_detail->GetString("a", &annotation);
           suggestion_detail->GetString("query_params", &suggest_query_params) ||
@@ -1207,7 +1229,7 @@ bool SearchProvider::ParseSuggestResults(base::Value* root_val,
       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));
     }
   }
 
@@ -1257,8 +1279,8 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   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);
   }
@@ -1280,7 +1302,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
             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);
       }
@@ -1630,7 +1652,7 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
     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

chrome/browser/autocomplete/search_provider.h

@@ -215,12 +215,16 @@ class SearchProvider : public AutocompleteProvider,
                   bool from_keyword_provider,
                   int relevance,
                   bool relevance_from_server,
-                  bool should_prefetch);
+                  bool should_prefetch,
+                  const base::string16& input_text);
     virtual ~SuggestResult();
 
     const base::string16& suggestion() const { return suggestion_; }
     AutocompleteMatchType::Type type() const { return type_; }
     const base::string16& match_contents() const { return match_contents_; }
+    const ACMatchClassifications& match_contents_class() const {
+      return match_contents_class_;
+    }
     const base::string16& annotation() const { return annotation_; }
     const std::string& suggest_query_params() const {
       return suggest_query_params_;
@@ -228,6 +232,13 @@ class SearchProvider : public AutocompleteProvider,
     const std::string& deletion_url() const { return deletion_url_; }
     bool should_prefetch() const { return should_prefetch_; }
 
+    // Fills in |match_contents_class| to reflect how |match_contents_| should
+    // be displayed and bolded against the current |input_text|.  If
+    // |allow_bolding_all| is false and |match_contents_class_| would have all
+    // of |match_contents_| bolded, do nothing.
+    void ClassifyMatchContents(const bool allow_bolding_all,
+                               const base::string16& input_text);
+
     // Result:
     virtual bool IsInlineable(const base::string16& input) const OVERRIDE;
     virtual int CalculateRelevance(
@@ -240,8 +251,9 @@ class SearchProvider : public AutocompleteProvider,
 
     AutocompleteMatchType::Type type_;
 
-    // The contents to be displayed in the autocomplete match.
+    // The contents to be displayed and its style info.
     base::string16 match_contents_;
+    ACMatchClassifications match_contents_class_;
 
     // Optional annotation for the |match_contents_| for disambiguation.
     // This may be displayed in the autocomplete match contents, but is defined
@@ -370,6 +382,11 @@ class SearchProvider : public AutocompleteProvider,
                                  SuggestResults* suggest_results,
                                  NavigationResults* navigation_results);
 
+  // Recalculates the match contents class of |suggest_results| to better
+  // display against the current input.
+  static void UpdateMatchContentsClass(const base::string16& input_text,
+                                       SuggestResults* suggest_results);
+
   // Calculates the relevance score for the keyword verbatim result (if the
   // input matches one of the profile's keyword).
   static int CalculateRelevanceForKeywordVerbatim(AutocompleteInput::Type type,

chrome/browser/autocomplete/search_provider_unittest.cc

@@ -3390,9 +3390,9 @@ TEST_F(SearchProviderTest, RemoveStaleResultsTest) {
         provider_->default_results_.suggest_results.push_back(
             SearchProvider::SuggestResult(
                 ASCIIToUTF16(suggestion), AutocompleteMatchType::SEARCH_SUGGEST,
-                base::string16(), base::string16(), std::string(),
+                ASCIIToUTF16(suggestion), base::string16(), std::string(),
                 std::string(), false, cases[i].results[j].relevance, false,
-                false));
+                false, ASCIIToUTF16(cases[i].omnibox_input)));
       }
     }
 

chrome/browser/autocomplete/zero_suggest_provider.cc

@@ -245,6 +245,8 @@ void ZeroSuggestProvider::FillResults(
 
   base::string16 result, title;
   std::string type;
+  const base::string16 current_query_string16 =
+      base::ASCIIToUTF16(current_query_);
   for (size_t index = 0; results->GetString(index, &result); ++index) {
     // Google search may return empty suggestions for weird input characters,
     // they make no sense at all and can cause problems in our code.
@@ -270,7 +272,7 @@ void ZeroSuggestProvider::FillResults(
       suggest_results->push_back(SearchProvider::SuggestResult(
           result, AutocompleteMatchType::SEARCH_SUGGEST, result,
           base::string16(), std::string(), std::string(), false, relevance,
-          relevances != NULL, false));
+          relevances != NULL, false, current_query_string16));
     }
   }
 }
@@ -291,10 +293,10 @@ void ZeroSuggestProvider::AddMatchToMap(int relevance,
                                         const base::string16& query_string,
                                         int accepted_suggestion,
                                         SearchProvider::MatchMap* map) {
+  // Pass in query_string as the input_text to avoid bolding.
   SearchProvider::SuggestResult suggestion(
       query_string, type, query_string, base::string16(), std::string(),
-      std::string(), false, relevance, true, false);
-  // Pass in query_string as the input_text since we don't want any bolding.
+      std::string(), false, relevance, true, false, query_string);
   // TODO(samarth|melevin): use the actual omnibox margin here as well instead
   // of passing in -1.
   AutocompleteMatch match = SearchProvider::CreateSearchSuggestion(