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..f3177907132d62702a393154909df72437d1b8e9 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -133,12 +133,55 @@ 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) {
+  contents_.assign(suggestion_);

[Mark P, 2013/08/15 18:34:23]

This whole blob was moved from CreateSearchSuggestion, with
match.contents -> contents_
match.contents_class -> contents_class_

+  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_) {
+    size_t input_position = 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.
+      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::SuggestResult::~SuggestResult() {
@@ -165,13 +208,37 @@ 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());
+  // First look for the user's input inside the fill_into_edit as it would be

[Mark P, 2013/08/15 18:34:23]

This whole section is the necessary parts from NavigationToMatch() to compute
contents_ and contents_class_.

[msw, 2013/08/15 18:51:03]

Can we reduce more duplication between the two?

[Mark P, 2013/08/15 19:28:15]

I can't figure out how.  Here's the problem: inline_autocompletion has to be
computed live.  It's computed using inline_autocomplete_offset.  Many of the
lines that remain in NavigationToMatch are things that are used to compute
inline_autocomplete_offset; all these have to happen live on the current input,
none can be moved into NavigationResult for pre-computing.

The only idea I have is to make a helper function that returns format_types and
a tentative inline_autocomplete_offset (that later may get modified by
NavigationToMatch's FormatURL call).  That'll eliminate most of the common code,
but it sounds a little awkward.  What do you think?

[msw, 2013/08/15 19:48:30]

I was thinking of a helper function, but only if works well.

+  // without trimming the scheme, so we can find matches at the beginning of the
+  // scheme.
+  const string16& untrimmed_fill_into_edit = formatted_url_;
+  const URLPrefix* prefix =
+      URLPrefix::BestURLPrefix(untrimmed_fill_into_edit, input_text);
+  size_t match_start = (prefix == NULL) ?
+      untrimmed_fill_into_edit.find(input_text) : prefix->prefix.length();
+  bool trim_http = !HasHTTPScheme(input_text) && (!prefix || (match_start != 0));
+  const net::FormatUrlTypes format_types =
+      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::NavigationResult::~NavigationResult() {
@@ -259,81 +326,45 @@ SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
 // static
 AutocompleteMatch SearchProvider::CreateSearchSuggestion(
     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 +377,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;
@@ -906,6 +937,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.
@@ -915,6 +948,8 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
     // Apply valid suggested relevance scores; discard invalid lists.
     if (relevances != NULL && !relevances->GetInteger(index, &relevance))
       relevances = NULL;
+    string16 contents;
+    ACMatchClassifications contents_class;

[Mark P, 2013/08/15 18:34:23]

Forgot to delete those two lines, sorry.

     if (types && types->GetString(index, &type) && (type == "NAVIGATION")) {
       // Do not blindly trust the URL coming from the server to be valid.
       GURL url(URLFixerUpper::FixupURL(UTF16ToUTF8(result), std::string()));
@@ -922,12 +957,12 @@ 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 +999,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 +1018,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 +1246,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 +1295,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 +1319,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 +1434,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 +1454,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.
@@ -1494,18 +1527,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);