Omnibox: Reduce Bolding Flicker in SearchProvider

chrome/browser/autocomplete/search_provider.h

Index: chrome/browser/autocomplete/search_provider.h
diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h
index 281684d22f54d0565b215ae9bb2f9a81f303be50..5388088463892e3076d0b11a14dbff1b4726366f 100644
--- a/chrome/browser/autocomplete/search_provider.h
+++ b/chrome/browser/autocomplete/search_provider.h
@@ -60,36 +60,6 @@ class SearchProvider : public AutocompleteProvider,
 
   SearchProvider(AutocompleteProviderListener* listener, Profile* profile);
 
-  // Returns an AutocompleteMatch with the given |autocomplete_provider|,
-  // |relevance|, and |type|, which represents a search via |template_url| for
-  // |query_string|.  If |template_url| is NULL, returns a match with an invalid
-  // destination URL.
-  //
-  // |input_text| is the original user input, which may differ from
-  // |query_string|; e.g. the user typed "foo" and got a search suggestion of
-  // "food", which we're now marking up.  This is used to highlight portions of
-  // the match contents to distinguish locally-typed text from suggested text.
-  //
-  // |input| and |is_keyword| are necessary for various other details, like
-  // whether we should allow inline autocompletion and what the transition type
-  // should be.  |accepted_suggestion| and |omnibox_start_margin| are used along
-  // with |input_text| to generate Assisted Query Stats.
-  // |append_extra_query_params| should be set if |template_url| is the default
-  // search engine, so the destination URL will contain any
-  // command-line-specified query params.
-  static AutocompleteMatch CreateSearchSuggestion(
-      AutocompleteProvider* autocomplete_provider,
-      int relevance,
-      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);
-
   // AutocompleteProvider:
   virtual void AddProviderInfo(ProvidersInfo* provider_info) const OVERRIDE;
   virtual void ResetSession() OVERRIDE;
@@ -181,6 +151,11 @@ class SearchProvider : public AutocompleteProvider,
       relevance_from_server_ = relevance_from_server;
     }
 
+    const string16& contents() const { return contents_; }
+    const ACMatchClassifications& contents_class() const {
+      return contents_class_;
+    }
+
     // Returns if this result is inlineable against the current input |input|.
     // Non-inlineable results are stale.
     virtual bool IsInlineable(const string16& input) const = 0;
@@ -199,6 +174,10 @@ class SearchProvider : public AutocompleteProvider,
     // The relevance score.
     int relevance_;
 
+    // The main text displayed in the address bar dropdown.

[msw, 2013/08/26 22:57:15]

nit: ..."and its style info." or similar that fits on one line.

+    string16 contents_;
+    ACMatchClassifications contents_class_;
+
    private:
     // Whether this result's relevance score was fully or partly calculated
     // based on server information, and thus is assumed to be more accurate.
@@ -210,14 +189,27 @@ class SearchProvider : public AutocompleteProvider,
 
   class SuggestResult : public Result {
    public:
+    // Most parameters are self-explanatory.  |input_text| is used to

[msw, 2013/08/26 22:57:15]

I think this whole comment is unnecessary here; ditto for NavigationResult.
Put the bit about avoiding flicker in SearchProvider::RecalculateBolding.

+    // determine highlighting (i.e., populate |contents_| and
+    // |contents_class_|).  We determine highlighting at the time
+    // the SuggestResult is created in order to reduce flicker as
+    // additional characters are typed or deleted.
     SuggestResult(const string16& suggestion,
                   bool from_keyword_provider,
                   int relevance,
-                  bool relevance_from_server);
+                  bool relevance_from_server,
+                  const string16& input_text);
     virtual ~SuggestResult();
 
     const string16& suggestion() const { return suggestion_; }
 
+    // Fills in |contents_| and |contents_class_| to reflect how |contents_|
+    // should be displayed and bolded against the current |input_text|.  If
+    // |allow_bolding_all| is false and the new |contents_class_| would have
+    // all of |contents_| bolded, do nothing.
+    void CalculateContents(bool allow_bolding_all,

[msw, 2013/08/26 22:57:15]

It'd be nice to put a single pure virtual decl in the base Result struct, even
though the two structs would OVERRIDE to provide distinct impls and
SuggestResult would ignore the |languages| arg. I suggest a consolidated decl to
make it easier to handle SuggestResult and NavigationResult instances
similarly/together in the future.

Even better would be more cohesion of some of the match generation code,
specifically adding a |Result::fill_into_edit_| member that's set within this
same function alongside |contents_| and |contents_class_|.

+                           const string16& input_text);
+
     // Result:
     virtual bool IsInlineable(const string16& input) const OVERRIDE;
     virtual int CalculateRelevance(
@@ -233,18 +225,34 @@ class SearchProvider : public AutocompleteProvider,
    public:
     // |provider| is necessary to use StringForURLDisplay() in order to
     // compute |formatted_url_|.
+    // |input_text| and |languages| are used to determine
+    // highlighting (i.e., populate |contents_| and |contents_class_|).
+    // We determine highlighting at the time the NavigationResult is
+    // created in order to reduce flicker as additional characters are
+    // typed or deleted.
     NavigationResult(const AutocompleteProvider& provider,
                      const GURL& url,
                      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);
     virtual ~NavigationResult();
 
     const GURL& url() const { return url_; }
     const string16& description() const { return description_; }
     const string16& formatted_url() const { return formatted_url_; }
 
+    // Fills in |contents_| and |contents_class_| to reflect how |contents_|
+    // should be displayed and bolded against the current |input_text| and
+    // user's |language|.  If |allow_bolding_nothing| is false and
+    // overwriting |contents_| and |contents_class_| would result in an
+    // entirely unbolded |contents_|, do nothing.
+    void CalculateContents(bool allow_bolding_nothing,
+                           const string16& input_text,
+                           const std::string languages);
+
     // Result:
     virtual bool IsInlineable(const string16& input) const OVERRIDE;
     virtual int CalculateRelevance(
@@ -304,6 +312,35 @@ class SearchProvider : public AutocompleteProvider,
 
   virtual ~SearchProvider();
 
+  // Returns an AutocompleteMatch with the given |autocomplete_provider|,
+  // and |type| based on the search query suggestion represented in |result|

[msw, 2013/08/26 22:57:15]

nit: add trailing period.

+  // The AutocompleteMatch represents a search via |template_url| for this
+  // suggestion.  If |template_url| is NULL, returns a match with an invalid
+  // destination URL.
+  //
+  // |input_text| is the original user input, which may differ from
+  // |result.suggestion()|; e.g. the user typed "foo" and got a search
+  // suggestion of "food", which we're now marking up.  This is used to
+  // decide whether the match returned is inlineable.
+  //
+  // |input| and |is_keyword| are necessary for various other details, like
+  // whether we should allow inline autocompletion and what the transition type
+  // should be.  |accepted_suggestion| and |omnibox_start_margin| are used along
+  // with |input_text| to generate Assisted Query Stats.
+  // |append_extra_query_params| should be set if |template_url| is the default
+  // search engine, so the destination URL will contain any
+  // command-line-specified query params.
+  static AutocompleteMatch CreateSearchSuggestion(

[msw, 2013/08/26 22:57:15]

This would be simpler as a non-static function more similar to
NavigationToMatch; it could drop |autocomplete_provider| (using |this|
internally), drop |input| and |input_text| for |input_|[.text()], and use
result.from_keyword_provider() instead of taking |template_url|.

I would also highly recommend removing the last three parameters (int
accepted_suggestion, int omnibox_start_margin, and bool
append_extra_query_params) and simply setting that data within
match.search_terms_args and match.destination_url from
SearchProvider::AddMatchToMap.

I know it's a bit tangential, but SearchProvider is such a mess it's hardly
maintainable. I can hardly imagine approving this CL's additional complexity for
a fairly low-impact behavioral change without offsetting the new complexity a
bit (either now or with some prerequisite work).

+      AutocompleteProvider* autocomplete_provider,
+      const SuggestResult& result,
+      AutocompleteMatch::Type type,
+      const TemplateURL* template_url,
+      const string16& input_text,

[msw, 2013/08/26 22:57:15]

Remove this and just use |input|'s AutocompleteInput::text().

+      const AutocompleteInput& input,
+      int accepted_suggestion,
+      int omnibox_start_margin,
+      bool append_extra_query_params);
+
   // Removes non-inlineable results until either the top result can inline
   // autocomplete the current input or verbatim outscores the top result.
   static void RemoveStaleResults(const string16& input,
@@ -311,6 +348,13 @@ class SearchProvider : public AutocompleteProvider,
                                  SuggestResults* suggest_results,
                                  NavigationResults* navigation_results);
 
+  // For every result in |results| (suggest or navigation), call
+  // CalculateContents(), which may replace its contents and bolding to
+  // better display against the current input.
+  static void RecalculateBolding(const string16& input_text,
+                                 const std::string languages,
+                                 Results* 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,
@@ -445,13 +489,10 @@ class SearchProvider : public AutocompleteProvider,
   // Creates an AutocompleteMatch for "Search <engine> for |query_string|" with
   // the supplied relevance.  Adds this match to |map|; if such a match already
   // exists, whichever one has lower relevance is eliminated.
-  void AddMatchToMap(const string16& query_string,
+  void 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);
 
   // Returns an AutocompleteMatch for a navigational suggestion.