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 c561490ad1301b5468d61b353253b4df8253852d..ec1ba0a09e8f9c91f7fad8e06fc30460a18cdc1c 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -97,6 +97,29 @@ bool HasMultipleWords(const string16& text) {
   return false;
 }
 
+// When displaying a match for |formatted_url| against omnibox input
+// |input_text|, determine:
+// * if Chrome should strip the http scheme
+// * where |input_text| matches in |formatted_url| (if it does)
+// * where |input_text| matches as an inline autocompletion in |formatted_url_|
+//   (if it does)
+void GetTrimHttpAndOffsets(const string16& formatted_url,
+                           const string16& input_text,
+                           bool* trim_http,
+                           size_t* match_start,
+                           size_t* inline_autocomplete_offset) {
+  // Look for the user's input inside the formatted_url as it would be without

[Mark P, 2013/08/16 23:25:58]

This logic was moved here from NavigationToMatch().

+  // trimming the scheme, so we can find matches at the beginning of the
+  // scheme.
+  const URLPrefix* prefix = URLPrefix::BestURLPrefix(formatted_url, input_text);
+  (*match_start) = (prefix == NULL) ?
+      formatted_url.find(input_text) : prefix->prefix.length();
+  (*trim_http) = !AutocompleteProvider::HasHTTPScheme(input_text) &&
+      (!prefix || (match_start != 0));
+  (*inline_autocomplete_offset) = (prefix == NULL) ?
+      string16::npos : (*match_start + input_text.length());
+}
+
 }  // namespace
 
 
@@ -133,12 +156,15 @@ SearchProvider::Result::~Result() {
 
 // SearchProvider::SuggestResult ----------------------------------------------
 
-SearchProvider::SuggestResult::SuggestResult(const string16& suggestion,
-                                             bool from_keyword_provider,
-                                             int relevance,
-                                             bool relevance_from_server)
+SearchProvider::SuggestResult::SuggestResult(
+    const string16& suggestion,
+    bool from_keyword_provider,
+    int relevance,
+    bool relevance_from_server,
+    const string16& input_text)
     : Result(from_keyword_provider, relevance, relevance_from_server),
       suggestion_(suggestion) {
+  CalculateContents(true, input_text);
 }
 
 SearchProvider::SuggestResult::~SuggestResult() {
@@ -156,6 +182,58 @@ int SearchProvider::SuggestResult::CalculateRelevance(
   return ((input.type() == AutocompleteInput::URL) ? 300 : 600);
 }
 
+void SearchProvider::SuggestResult::CalculateContents(

[Mark P, 2013/08/16 23:25:58]

This is still basically a cut-and-paste from CreateSearchSuggestion, with the
change to handle allow_bolding_all and also a replacement of match.contents to
contents_ and match.contents_class to contents_class_.

+    bool allow_bolding_all,
+    const string16& input_text) {
+  size_t input_position = suggestion_.find(input_text);
+  if (!allow_bolding_all && (input_text != suggestion_) &&
+      (input_position == 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;
+  }
+  contents_.assign(suggestion_);
+  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 != suggestion_) {
+    if (input_position == 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.
+      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) {
+        contents_class_.push_back(
+            ACMatchClassification(0, ACMatchClassification::MATCH));
+      }
+      contents_class_.push_back(
+          ACMatchClassification(input_position, ACMatchClassification::NONE));
+      size_t next_fragment_position = input_position + input_text.length();
+      if (next_fragment_position < suggestion_.length()) {
+        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.
+    contents_class_.push_back(
+        ACMatchClassification(0, ACMatchClassification::NONE));
+
+  }
+}
 
 // SearchProvider::NavigationResult -------------------------------------------
 
@@ -165,13 +243,16 @@ SearchProvider::NavigationResult::NavigationResult(
     const string16& description,
     bool from_keyword_provider,
     int relevance,
-    bool relevance_from_server)
+    bool relevance_from_server,
+    const string16& input_text,
+    const std::string& languages)
     : Result(from_keyword_provider, relevance, relevance_from_server),
       url_(url),
       formatted_url_(AutocompleteInput::FormattedStringWithEquivalentMeaning(
           url, provider.StringForURLDisplay(url, true, false))),
       description_(description) {
   DCHECK(url_.is_valid());
+  CalculateContents(true, input_text, languages);
 }
 
 SearchProvider::NavigationResult::~NavigationResult() {
@@ -188,6 +269,34 @@ int SearchProvider::NavigationResult::CalculateRelevance(
   return (from_keyword_provider_ || !keyword_provider_requested) ? 800 : 150;
 }
 
+void SearchProvider::NavigationResult::CalculateContents(
+    bool allow_bolding_nothing,
+    const string16& input_text,
+    const std::string languages) {
+  bool trim_http;
+  size_t match_start, inline_autocomplete_offset;
+  GetTrimHttpAndOffsets(formatted_url_, input_text, &trim_http, &match_start,
+                        &inline_autocomplete_offset);
+  if (!allow_bolding_nothing && (match_start == string16::npos)) {
+    // Bail if the code below to update the bolding would make the stirng
+    // entirely not bolded.  Note that the string may already be entirely
+    // not bolded; if so, leave it as is.
+    return;
+  }
+  const net::FormatUrlTypes format_types =

[Mark P, 2013/08/16 23:25:58]

This logic here and below was moved here from NavigationToMatch().

+      net::kFormatUrlOmitAll & ~(trim_http ? 0 : net::kFormatUrlOmitHTTP);
+  contents_ = net::FormatUrl(url_, languages,
+      format_types, net::UnescapeRule::SPACES, NULL, NULL, &match_start);
+  // If the first match in the untrimmed string was inside a scheme that we
+  // trimmed, look for a subsequent match.
+  if (match_start == string16::npos)
+    match_start = contents_.find(input_text);
+  // Safe if |match_start| is npos; also safe if the input is longer than the
+  // remaining contents after |match_start|.
+  AutocompleteMatch::ClassifyLocationInString(match_start, input_text.length(),
+      contents_.length(), ACMatchClassification::URL,
+      &contents_class_);
+}
 
 // SearchProvider::CompareScoredResults ---------------------------------------
 
@@ -259,81 +368,45 @@ SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
 // static
 AutocompleteMatch SearchProvider::CreateSearchSuggestion(

[Mark P, 2013/08/16 23:25:58]

When this review is almost complete, I will move this to the right place in this
file according to header order.  I've left it as is for the moment for ease of
review.

     AutocompleteProvider* autocomplete_provider,
-    int relevance,
+    const SuggestResult& result,
     AutocompleteMatch::Type type,
     const TemplateURL* template_url,
-    const string16& query_string,
     const string16& input_text,
     const AutocompleteInput& input,
-    bool is_keyword,
     int accepted_suggestion,
     int omnibox_start_margin,
     bool append_extra_query_params) {
-  AutocompleteMatch match(autocomplete_provider, relevance, false, type);
+  AutocompleteMatch match(
+      autocomplete_provider, result.relevance(), false, type);
 
   if (!template_url)
     return match;
   match.keyword = template_url->keyword();
 
-  match.contents.assign(query_string);
-  // 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 != query_string) {
-    size_t input_position = match.contents.find(input_text);
-    if (input_position == 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));
+  match.contents = result.contents();
+  match.contents_class = result.contents_class();
+  if (input_text == result.suggestion())
     match.allowed_to_be_default_match = true;
-  }
 
   // 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.
   if (input.type() == AutocompleteInput::FORCED_QUERY)
     match.fill_into_edit.assign(ASCIIToUTF16("?"));
-  if (is_keyword)
+  if (result.from_keyword_provider())
     match.fill_into_edit.append(match.keyword + char16(' '));
   if (!input.prevent_inline_autocomplete() &&
-      StartsWith(query_string, input_text, false)) {
-    match.inline_autocompletion = query_string.substr(input_text.length());
+      StartsWith(result.suggestion(), input_text, false)) {
+    match.inline_autocompletion =
+        result.suggestion().substr(input_text.length());
     match.allowed_to_be_default_match = true;
   }
-  match.fill_into_edit.append(query_string);
+  match.fill_into_edit.append(result.suggestion());
 
   const TemplateURLRef& search_url = template_url->url_ref();
   DCHECK(search_url.SupportsReplacement());
   match.search_terms_args.reset(
-      new TemplateURLRef::SearchTermsArgs(query_string));
+      new TemplateURLRef::SearchTermsArgs(result.suggestion()));
   match.search_terms_args->original_query = input_text;
   match.search_terms_args->accepted_suggestion = accepted_suggestion;
   match.search_terms_args->omnibox_start_margin = omnibox_start_margin;
@@ -346,7 +419,7 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
       GURL(search_url.ReplaceSearchTerms(*match.search_terms_args.get()));
 
   // Search results don't look like URLs.
-  match.transition = is_keyword ?
+  match.transition = result.from_keyword_provider() ?
       content::PAGE_TRANSITION_KEYWORD : content::PAGE_TRANSITION_GENERATED;
 
   return match;
@@ -430,6 +503,20 @@ void SearchProvider::RemoveStaleResults(const string16& input,
 }
 
 // static
+void SearchProvider::RecalculateBolding(const string16& input_text,
+                                        const std::string languages,
+                                        Results* results) {
+  for (SuggestResults::iterator sug_it = results->suggest_results.begin();
+       sug_it != results->suggest_results.end(); ++sug_it) {
+    sug_it->CalculateContents(false, input_text);
+  }
+  for (NavigationResults::iterator nav_it = results->navigation_results.begin();
+       nav_it != results->navigation_results.end(); ++nav_it) {
+    nav_it->CalculateContents(false, input_text, languages);
+  }
+}
+
+// static
 int SearchProvider::CalculateRelevanceForKeywordVerbatim(
     AutocompleteInput::Type type,
     bool prefer_keyword) {
@@ -685,6 +772,13 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
   // Remove existing results that cannot inline autocomplete the new input.
   RemoveAllStaleResults();
 
+  // Revise the bolding of all results that remain so they look good against
+  // the current input.
+  const std::string& languages =
+      profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
+  RecalculateBolding(input_.text(), languages, &default_results_);
+  RecalculateBolding(keyword_input_.text(), languages, &keyword_results_);
+
   // We can't start a new query if we're only allowed synchronous results.
   if (input_.matches_requested() != AutocompleteInput::ALL_MATCHES)
     return;
@@ -906,6 +1000,8 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
   string16 result, title;
   std::string type;
   int relevance = -1;
+  const std::string& languages =
+      profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
   for (size_t index = 0; results_list->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.
@@ -922,12 +1018,13 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
         if (descriptions != NULL)
           descriptions->GetString(index, &title);
         results->navigation_results.push_back(NavigationResult(
-            *this, url, title, is_keyword, relevance, true));
+            *this, url, title, is_keyword, relevance, true, input_text,
+            languages));
       }
     } else {
       // TODO(kochi): Improve calculator result presentation.
       results->suggest_results.push_back(
-          SuggestResult(result, is_keyword, relevance, true));
+          SuggestResult(result, is_keyword, relevance, true, input_text));
     }
   }
 
@@ -964,10 +1061,11 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
   if (verbatim_relevance > 0) {
-    AddMatchToMap(input_.text(), input_.text(), verbatim_relevance,
-                  relevance_from_server,
+    SuggestResult verbatim(input_.text(), false, verbatim_relevance,
+                           relevance_from_server, input_.text());
+    AddMatchToMap(verbatim, input_.text(),
                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
-                  did_not_accept_default_suggestion, false, &map);
+                  did_not_accept_default_suggestion, &map);
   }
   if (!keyword_input_.text().empty()) {
     const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
@@ -982,10 +1080,12 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       const int keyword_verbatim_relevance =
           GetKeywordVerbatimRelevance(&keyword_relevance_from_server);
       if (keyword_verbatim_relevance > 0) {
-        AddMatchToMap(keyword_input_.text(), keyword_input_.text(),
-                      keyword_verbatim_relevance, keyword_relevance_from_server,
+        SuggestResult keyword_verbatim(
+            keyword_input_.text(), true, keyword_verbatim_relevance,
+            keyword_relevance_from_server, keyword_input_.text());
+        AddMatchToMap(keyword_verbatim, keyword_input_.text(),
                       AutocompleteMatchType::SEARCH_OTHER_ENGINE,
-                      did_not_accept_keyword_suggestion, true, &map);
+                      did_not_accept_keyword_suggestion, &map);
       }
     }
   }
@@ -1208,10 +1308,9 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                                          is_keyword);
   for (SuggestResults::const_iterator i(scored_results.begin());
        i != scored_results.end(); ++i) {
-    AddMatchToMap(i->suggestion(), input_text, i->relevance(), false,
+    AddMatchToMap(*i, input_text,
                   AutocompleteMatchType::SEARCH_HISTORY,
-                  did_not_accept_suggestion,
-                  is_keyword, map);
+                  did_not_accept_suggestion, map);
   }
 }
 
@@ -1258,7 +1357,7 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
         i->time, is_keyword, !prevent_inline_autocomplete,
         prevent_search_history_inlining);
     scored_results.push_back(
-        SuggestResult(i->term, is_keyword, relevance, false));
+        SuggestResult(i->term, is_keyword, relevance, false, input_text));
   }
 
   // History returns results sorted for us.  However, we may have docked some
@@ -1282,11 +1381,10 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
 void SearchProvider::AddSuggestResultsToMap(const SuggestResults& results,
                                             MatchMap* map) {
   for (size_t i = 0; i < results.size(); ++i) {
-    const bool is_keyword = results[i].from_keyword_provider();
-    const string16& input = is_keyword ? keyword_input_.text() : input_.text();
-    AddMatchToMap(results[i].suggestion(), input, results[i].relevance(),
-                  results[i].relevance_from_server(),
-                  AutocompleteMatchType::SEARCH_SUGGEST, i, is_keyword, map);
+    const string16& input = results[i].from_keyword_provider() ?
+        keyword_input_.text() : input_.text();
+    AddMatchToMap(results[i], input,
+                  AutocompleteMatchType::SEARCH_SUGGEST, i, map);
   }
 }
 
@@ -1398,13 +1496,10 @@ int SearchProvider::CalculateRelevanceForHistory(
   return std::max(0, base_score - score_discount);
 }
 
-void SearchProvider::AddMatchToMap(const string16& query_string,
+void SearchProvider::AddMatchToMap(const SuggestResult& result,
                                    const string16& input_text,
-                                   int relevance,
-                                   bool relevance_from_server,
                                    AutocompleteMatch::Type type,
                                    int accepted_suggestion,
-                                   bool is_keyword,
                                    MatchMap* map) {
   // On non-mobile, ask the instant controller for the appropriate start margin.
   // On mobile the start margin is unused, so leave the value as default there.
@@ -1421,22 +1516,22 @@ void SearchProvider::AddMatchToMap(const string16& query_string,
   }
 #endif  // !defined(OS_ANDROID) && !defined(IOS)
 
-  const TemplateURL* template_url = is_keyword ?
+  const TemplateURL* template_url = result.from_keyword_provider() ?
       providers_.GetKeywordProviderURL() : providers_.GetDefaultProviderURL();
-  AutocompleteMatch match = CreateSearchSuggestion(this, relevance, type,
-      template_url, query_string, input_text, input_, is_keyword,
-      accepted_suggestion, omnibox_start_margin,
-      !is_keyword || providers_.default_provider().empty());
+  AutocompleteMatch match = CreateSearchSuggestion(this, result,
+      type, template_url, input_text, input_, accepted_suggestion,
+      omnibox_start_margin,
+     !result.from_keyword_provider() || providers_.default_provider().empty());
   if (!match.destination_url.is_valid())
     return;
   match.RecordAdditionalInfo(kRelevanceFromServerKey,
-                             relevance_from_server ? kTrue : kFalse);
+                             result.relevance_from_server() ? kTrue : kFalse);
 
   // Try to add |match| to |map|.  If a match for |query_string| is already in
   // |map|, replace it if |match| is more relevant.
   // NOTE: Keep this ToLower() call in sync with url_database.cc.
-  const std::pair<MatchMap::iterator, bool> i(
-      map->insert(std::make_pair(base::i18n::ToLower(query_string), match)));
+  const std::pair<MatchMap::iterator, bool> i(map->insert(std::make_pair(
+      base::i18n::ToLower(result.suggestion()), match)));
   // NOTE: We purposefully do a direct relevance comparison here instead of
   // using AutocompleteMatch::MoreRelevant(), so that we'll prefer "items added
   // first" rather than "items alphabetically first" when the scores are equal.
@@ -1457,18 +1552,10 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
                           AutocompleteMatchType::NAVSUGGEST);
   match.destination_url = navigation.url();
 
-  // First look for the user's input inside the fill_into_edit as it would be
-  // without trimming the scheme, so we can find matches at the beginning of the
-  // scheme.
-  const string16& untrimmed_fill_into_edit = navigation.formatted_url();
-  const URLPrefix* prefix =
-      URLPrefix::BestURLPrefix(untrimmed_fill_into_edit, input);
-  size_t match_start = (prefix == NULL) ?
-      untrimmed_fill_into_edit.find(input) : prefix->prefix.length();
-  size_t inline_autocomplete_offset = (prefix == NULL) ?
-      string16::npos : (match_start + input.length());
-  bool trim_http = !HasHTTPScheme(input) && (!prefix || (match_start != 0));
-
+  bool trim_http;
+  size_t match_start, inline_autocomplete_offset;
+  GetTrimHttpAndOffsets(navigation.formatted_url(), input, &trim_http,
+                        &match_start, &inline_autocomplete_offset);
   // Preserve the forced query '?' prefix in |match.fill_into_edit|.
   // Otherwise, user edits to a suggestion would show non-Search results.
   if (input_.type() == AutocompleteInput::FORCED_QUERY) {
@@ -1494,18 +1581,8 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
         match.fill_into_edit.substr(inline_autocomplete_offset);
   }
 
-  match.contents = net::FormatUrl(navigation.url(), languages,
-      format_types, net::UnescapeRule::SPACES, NULL, NULL, &match_start);
-  // If the first match in the untrimmed string was inside a scheme that we
-  // trimmed, look for a subsequent match.
-  if (match_start == string16::npos)
-    match_start = match.contents.find(input);
-  // Safe if |match_start| is npos; also safe if the input is longer than the
-  // remaining contents after |match_start|.
-  AutocompleteMatch::ClassifyLocationInString(match_start, input.length(),
-      match.contents.length(), ACMatchClassification::URL,
-      &match.contents_class);
-
+  match.contents = navigation.contents();
+  match.contents_class = navigation.contents_class();
   match.description = navigation.description();
   AutocompleteMatch::ClassifyMatchInString(input, match.description,
       ACMatchClassification::NONE, &match.description_class);