Makes the SearchProvider set the description from the KeywordProvider.

BUG=89263
TEST=none
R=pkasting@chromium.org

Review URL: http://codereview.chromium.org/7328002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92452 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 55 insertions, 63 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index fff149f..6eddcad 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -47,6 +47,14 @@
 
 using base::TimeDelta;
 
+static bool AreTemplateURLsEqual(const TemplateURL* a,
+                                 const TemplateURL* b) {
+  // We can't use equality of the pointers because SearchProvider copies the
+  // TemplateURLs. Instead we compare based on ID.
+  // a may be NULL, but never b, so we don't handle the case of a==b==NULL.
+  return a && b && (a->id() == b->id());
+}
+
 // AutocompleteInput ----------------------------------------------------------
 
 AutocompleteInput::AutocompleteInput()
@@ -507,9 +515,6 @@ void AutocompleteProvider::Stop() {
 void AutocompleteProvider::DeleteMatch(const AutocompleteMatch& match) {
 }
 
-void AutocompleteProvider::PostProcessResult(AutocompleteResult* result) {
-}
-
 AutocompleteProvider::~AutocompleteProvider() {
   Stop();
 }
@@ -802,7 +807,8 @@ AutocompleteController::AutocompleteController(
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kDisableHistoryURLProvider))
     providers_.push_back(new HistoryURLProvider(this, profile));
-  providers_.push_back(new KeywordProvider(this, profile));
+  keyword_provider_ = new KeywordProvider(this, profile);
+  providers_.push_back(keyword_provider_);
   providers_.push_back(new HistoryContentsProvider(this, profile, hqp_enabled));
   providers_.push_back(new BuiltinProvider(this, profile));
   providers_.push_back(new ExtensionAppProvider(this, profile));
@@ -953,13 +959,7 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
     result_.CopyOldMatches(input_, last_result);
   }
 
-  size_t start_size = result_.size();
-  for (ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
-       ++i)
-    (*i)->PostProcessResult(&result_);
-  // Providers should not alter the number of matches, otherwise it's very
-  // likely the matches are no longer sorted.
-  DCHECK_EQ(start_size, result_.size());
+  UpdateKeywordDescriptions(&result_);
 
   bool notify_default_match = is_synchronous_pass;
   if (!is_synchronous_pass) {
@@ -980,6 +980,33 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
   NotifyChanged(notify_default_match);
 }
 
+void AutocompleteController::UpdateKeywordDescriptions(
+    AutocompleteResult* result) {
+  const TemplateURL* last_template_url = NULL;
+  for (AutocompleteResult::iterator i = result->begin(); i != result->end();
+       ++i) {
+    if (((i->provider == keyword_provider_) && i->template_url) ||
+        ((i->provider == search_provider_) &&
+         (i->type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED ||
+          i->type == AutocompleteMatch::SEARCH_HISTORY ||
+          i->type == AutocompleteMatch::SEARCH_SUGGEST))) {
+      i->description.clear();
+      i->description_class.clear();
+      DCHECK(i->template_url);
+      if (!AreTemplateURLsEqual(last_template_url, i->template_url)) {
+        i->description = l10n_util::GetStringFUTF16(
+            IDS_AUTOCOMPLETE_SEARCH_DESCRIPTION,
+            i->template_url->AdjustedShortNameForLocaleDirection());
+        i->description_class.push_back(
+            ACMatchClassification(0, ACMatchClassification::DIM));
+      }
+      last_template_url = i->template_url;
+    } else {
+      last_template_url = NULL;
+    }
+  }
+}
+
 void AutocompleteController::NotifyChanged(bool notify_default_match) {
   if (delegate_)
     delegate_->OnResultChanged(notify_default_match);
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index 1cbc3fc..c6d90eb 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -173,6 +173,7 @@ struct AutocompleteMatch;
 class AutocompleteProvider;
 class AutocompleteResult;
 class HistoryContentsProvider;
+class KeywordProvider;
 class Profile;
 class SearchProvider;
 class TemplateURL;
@@ -403,13 +404,6 @@ class AutocompleteProvider
   // NOTE: Remember to call OnProviderUpdate() if matches_ is updated.
   virtual void DeleteMatch(const AutocompleteMatch& match);
 
-  // Invoked after combining the results from all providers (including sorting,
-  // culling and all that) but before the AutocompleteController's delegate is
-  // notified. This is exposed to allow providers to alter the descriptions of
-  // matches based on position in the final result set. Providers should not use
-  // this to add new matches.
-  virtual void PostProcessResult(AutocompleteResult* result);
-
 #ifdef UNIT_TEST
   void set_listener(ACProviderListener* listener) { listener_ = listener; }
 #endif
@@ -604,6 +598,7 @@ class AutocompleteController : public ACProviderListener {
   explicit AutocompleteController(const ACProviders& providers)
       : delegate_(NULL),
         providers_(providers),
+        keyword_provider_(NULL),
         search_provider_(NULL),
         done_(true),
         in_start_(false) {
@@ -668,6 +663,11 @@ class AutocompleteController : public ACProviderListener {
   // the popup to ensure it's not showing an out-of-date query.
   void ExpireCopiedEntries();
 
+#ifdef UNIT_TEST
+  void set_search_provider(SearchProvider* provider) {
+    search_provider_ = provider;
+  }
+#endif
   SearchProvider* search_provider() const { return search_provider_; }
 
   // Getters
@@ -684,6 +684,10 @@ class AutocompleteController : public ACProviderListener {
   // Start() is calling this to get the synchronous result.
   void UpdateResult(bool is_synchronous_pass);
 
+  // For each group of contiguous matches from the same TemplateURL, show the
+  // provider name as a description on the first match in the group.
+  void UpdateKeywordDescriptions(AutocompleteResult* result);
+
   // Calls AutocompleteControllerDelegate::OnResultChanged() and if done sends
   // AUTOCOMPLETE_CONTROLLER_RESULT_READY.
   void NotifyChanged(bool notify_default_match);
@@ -699,6 +703,8 @@ class AutocompleteController : public ACProviderListener {
   // A list of all providers.
   ACProviders providers_;
 
+  KeywordProvider* keyword_provider_;
+
   SearchProvider* search_provider_;
 
   // Input passed to Start.
diff --git a/chrome/browser/autocomplete/keyword_provider.cc b/chrome/browser/autocomplete/keyword_provider.cc
index 00580ef..6a8cc42 100644
--- a/chrome/browser/autocomplete/keyword_provider.cc
+++ b/chrome/browser/autocomplete/keyword_provider.cc
@@ -436,20 +436,6 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch(
     result.template_url = element;
   result.transition = PageTransition::KEYWORD;
 
-  // Create popup entry description based on the keyword name.
-  if (!element->IsExtensionKeyword()) {
-    result.description.assign(l10n_util::GetStringFUTF16(
-        IDS_AUTOCOMPLETE_KEYWORD_DESCRIPTION, keyword));
-    string16 keyword_desc(
-        l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_KEYWORD_DESCRIPTION));
-    AutocompleteMatch::ClassifyLocationInString(
-        keyword_desc.find(ASCIIToUTF16("%s")),
-        prefix_length,
-        result.description.length(),
-        ACMatchClassification::DIM,
-        &result.description_class);
-  }
-
   return result;
 }
 
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 88b8908..915ce9f 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -14,8 +14,8 @@
 #include "base/string16.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
-#include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/instant/instant_controller.h"
 #include "chrome/browser/net/url_fixer_upper.h"
@@ -215,33 +215,6 @@ void SearchProvider::Stop() {
   default_provider_suggest_text_.clear();
 }
 
-void SearchProvider::PostProcessResult(AutocompleteResult* result) {
-  // For each group of contiguous matches from the same TemplateURL, show the
-  // provider name as a description on the first match in the group.
-  const TemplateURL* last_template_url = NULL;
-  for (AutocompleteResult::iterator i = result->begin(); i != result->end();
-       ++i) {
-    if (i->provider == this &&
-        (i->type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED ||
-         i->type == AutocompleteMatch::SEARCH_HISTORY ||
-         i->type == AutocompleteMatch::SEARCH_SUGGEST)) {
-      i->description.clear();
-      i->description_class.clear();
-      DCHECK(i->template_url);  // We should always set a template_url
-      if (last_template_url != i->template_url) {
-        i->description = l10n_util::GetStringFUTF16(
-            IDS_AUTOCOMPLETE_SEARCH_DESCRIPTION,
-            i->template_url->AdjustedShortNameForLocaleDirection());
-        i->description_class.push_back(
-            ACMatchClassification(0, ACMatchClassification::DIM));
-      }
-      last_template_url = i->template_url;
-    } else {
-      last_template_url = NULL;
-    }
-  }
-}
-
 void SearchProvider::OnURLFetchComplete(const URLFetcher* source,
                                         const GURL& url,
                                         const net::URLRequestStatus& status,
diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h
index 287a40d..208fcf7 100644
--- a/chrome/browser/autocomplete/search_provider.h
+++ b/chrome/browser/autocomplete/search_provider.h
@@ -69,7 +69,6 @@ class SearchProvider : public AutocompleteProvider,
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes) OVERRIDE;
   virtual void Stop() OVERRIDE;
-  virtual void PostProcessResult(AutocompleteResult* result) OVERRIDE;
 
   // URLFetcher::Delegate
   virtual void OnURLFetchComplete(const URLFetcher* source,
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc
index dfeaaa4..ace8bbe 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -540,8 +540,8 @@ TEST_F(SearchProviderTest, AutocompletePreviousSearchOnSpace) {
   EXPECT_EQ(4u, term_match.inline_autocomplete_offset);
 }
 
-// Verifies the SearchProvider sets descriptions for results correctly.
-TEST_F(SearchProviderTest, PostProcessResults) {
+// Verifies AutocompleteControllers sets descriptions for results correctly.
+TEST_F(SearchProviderTest, UpdateKeywordDescriptions) {
   // Add an entry that corresponds to a keyword search with 'term2'.
   string16 term(ASCIIToUTF16("term2"));
   HistoryService* history =
@@ -559,6 +559,7 @@ TEST_F(SearchProviderTest, PostProcessResults) {
   SearchProvider* provider = provider_.release();
   providers.push_back(provider);
   AutocompleteController controller(providers);
+  controller.set_search_provider(provider);
   provider->set_listener(&controller);
   controller.Start(ASCIIToUTF16("k t"), string16(), false, false, true,
                    AutocompleteInput::ALL_MATCHES);