Omnibox: Reduce Bolding Flicker in SearchProvider

chrome/browser/autocomplete/search_provider.h

 // Copyright (c) 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.
 //
 // This file contains the Search autocomplete provider.  This provider is
 // responsible for all autocomplete entries that start with "Search <engine>
 // for ...", including searching for the current input string, search
 // history, and search suggestions.  An instance of it gets created and
 // managed by the autocomplete controller.
 
@@ skipping 42 matching lines @@
                        public net::URLFetcherDelegate {
  public:
   // ID used in creating URLFetcher for default provider's suggest results.
   static const int kDefaultProviderURLFetcherID;
 
   // ID used in creating URLFetcher for keyword provider's suggest results.
   static const int kKeywordProviderURLFetcherID;
 
   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;
 
   bool field_trial_triggered_in_session() const {
     return field_trial_triggered_in_session_;
   }
 
  private:
   // TODO(hfung): Remove ZeroSuggestProvider as a friend class after
@@ skipping 71 matching lines @@
     bool from_keyword_provider() const { return from_keyword_provider_; }
 
     int relevance() const { return relevance_; }
     void set_relevance(int relevance) { relevance_ = relevance; }
 
     bool relevance_from_server() const { return relevance_from_server_; }
     void set_relevance_from_server(bool relevance_from_server) {
       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;
 
     // Returns the default relevance value for this result (which may
     // be left over from a previous omnibox input) given the current
     // input and whether the current input caused a keyword provider
     // to be active.
     virtual int CalculateRelevance(const AutocompleteInput& input,
                                    bool keyword_provider_requested) const = 0;
 
    protected:
     // True if the result came from the keyword provider.
     bool from_keyword_provider_;
 
     // 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.
     // This is ultimately used in
     // SearchProvider::ConvertResultsToAutocompleteMatches(), see comments
     // there.
     bool relevance_from_server_;
   };
 
   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(
         const AutocompleteInput& input,
         bool keyword_provider_requested) const OVERRIDE;
 
    private:
     // The search suggestion string.
     string16 suggestion_;
   };
 
   class NavigationResult : public Result {
    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(
         const AutocompleteInput& input,
         bool keyword_provider_requested) const OVERRIDE;
 
    private:
     // The suggested url for navigation.
     GURL url_;
 
@@ skipping 39 matching lines @@
     // indicate that there is no suggested score; a value of 0
     // suppresses the verbatim result.
     int verbatim_relevance;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Results);
   };
 
   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,
                                  int verbatim_relevance,
                                  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,
                                                   bool prefer_keyword);
 
   // AutocompleteProvider:
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes) OVERRIDE;
   virtual void Stop(bool clear_cached_results) OVERRIDE;
 
@@ skipping 114 matching lines @@
   // scores.  When using the aggressive method, scores may exceed 1300
   // unless |prevent_search_history_inlining| is set.
   int CalculateRelevanceForHistory(const base::Time& time,
                                    bool is_keyword,
                                    bool use_aggressive_method,
                                    bool prevent_search_history_inlining) const;
 
   // 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.
   AutocompleteMatch NavigationToMatch(const NavigationResult& navigation);
 
   // Resets the scores of all |keyword_navigation_results_| matches to
   // be below that of the top keyword query match (the verbatim match
   // as expressed by |keyword_verbatim_relevance_| or keyword query
   // suggestions stored in |keyword_suggest_results_|).  If there
   // are no keyword suggestions and keyword verbatim is suppressed,
@@ skipping 61 matching lines @@
   bool field_trial_triggered_in_session_;
 
   // If true, search history query suggestions will score low enough that
   // they will not be inlined.
   bool prevent_search_history_inlining_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchProvider);
 };
 
 #endif  // CHROME_BROWSER_AUTOCOMPLETE_SEARCH_PROVIDER_H_