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

[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;
   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_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);
 }
 
 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();
 

[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();
 
   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::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) {
   // 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();
 
+  // 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;
 
   // 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 307 matching lines @@
         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.

[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));
     }
   }
 
   // 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;
 }