Omnibox: Reduce Bolding Flicker in SearchProvider

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 80f8648c574d0198e373d5f657c241a230546478..da0929fd6be59da3e697c18827b777bfa9c7eb26 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/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

[Mark P, 2014/01/04 00:26:17]

This function moved to CalculateContentsClass() with slight modifications:
- reflect different variable names, pointers versus not
- remove setting of match->contents  (this gets pushed earlier to
ParseSuggestResults())
- add code to handle allow_bolding_all == false case

-// 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,64 @@ SearchProvider::SuggestResult::SuggestResult(
       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_text != match_contents_) &&

[msw, 2014/01/06 21:09:00]

nit: remove the unnecessary input_text != match_contents_ string comparison. If
the strings were equal, then input_position would not be base::string16::npos
(it would be zero), and this code wouldn't trigger anyway, right?

[Mark P, 2014/01/06 22:20:16]

Right you are.  Done.

+      (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_) {
+    size_t input_position = match_contents_.find(input_text);

[msw, 2014/01/06 21:09:00]

Remove this, use the above duplicated identifier...

[Mark P, 2014/01/06 22:20:16]

Stupid resolve error re-introduced this.  I had it right before!  (I'm not a
dumbass. :-) )

+    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 < 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);
@@ -411,8 +418,8 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
     return match;
   match.keyword = template_url->keyword();
 

[msw, 2014/01/06 21:09:00]

nit: remove blank line (or swap with line above.)

[Mark P, 2014/01/06 22:20:16]

Done.

-  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();
@@ -570,6 +577,15 @@ void SearchProvider::RemoveStaleResults(const base::string16& input,
 }
 
 // static
+void SearchProvider::RecalculateBolding(
+    const base::string16& input_text, SuggestResults* suggest_results) {

[msw, 2014/01/06 21:09:00]

nit: one param per line.

[Mark P, 2014/01/06 22:20:16]

Done.

+  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) {
@@ -875,6 +891,14 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
   // Remove existing results that cannot inline autocomplete the new input.
   RemoveAllStaleResults();
 
+  // Revise the bolding of suggest results that remain so they look good against

[msw, 2014/01/06 21:09:00]

nit: consider "Update the content and classifications of remaining results." or
similar.

[Mark P, 2014/01/06 22:20:16]

Done.

+  // the current input.
+  RecalculateBolding(input_.text(), &default_results_.suggest_results);
+  if (!keyword_input_.text().empty()) {
+    RecalculateBolding(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;
@@ -1202,12 +1226,15 @@ bool SearchProvider::ParseSuggestResults(base::Value* root_val,
               suggestion_detail->GetString("q", &suggest_query_params);
         }
       }
+      // Error correction for bad data from server.

[msw, 2014/01/06 21:09:00]

|match_contents| was already initialized with |suggestion| in the general case,
so this could only be helpful in the |suggestion_details| case. This code should
be moved directly after that |match_contents| modification. More importantly,
why would the server supply a blank "title" or "t" field in the suggestion
details? That should be fixed on the server side, even if we really do need this
stupid workaround here.

[Mark P, 2014/01/06 22:20:16]

I plan to talk to anuj about this because he's the one who added code to set
match_contents to suggestion if the suggestion was blank.  I think this code is
only to handle bad data from the server; he can tell me for sure.

Note that I like having the code here regardless under the principle you've long
supported that SearchProvider should do something sane regardless of what it
gets back from the server.

[msw, 2014/01/06 22:50:21]

Keeping the workaround to fixup the broken |suggestion_detail| server behavior
may be necessary (and in that case it's fine by me), but since |match_contents|
is *initialized* to |suggestion| and only ever reassigned in the above
|suggestion_detail| block, I strongly suggest moving this assignment into that
block, immediately after the "suggestion_detail->GetString("title",
&match_contents)... " line. (ie. only fixup |match_contents| in the case where
it'll be broken; otherwise when |suggestion_details| is NULL, you're doing
"match_contents = suggestion; if (match_contents.empty()) match_contents =
suggestion;"...)

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

Moved the correcting assignment in the block, as you suggested.

+      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));
     }
   }
 
@@ -1257,8 +1284,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 +1307,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 +1657,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