Fixes up the autocomplete providers to correctly update starred

state. I broke this in order to land my last mongo
patch. HistoryContentsProvider is still lacking in one area.

BUG=1256202
TEST=none


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

7 files changed, 41 insertions, 85 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 4153032..881ac2c 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -37,7 +37,9 @@
 #include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/external_protocol_handler.h"
+#include "chrome/browser/bookmark_bar_model.h"
 #include "chrome/browser/history_tab_ui.h"
+#include "chrome/browser/profile.h"
 #include "chrome/browser/url_fixer_upper.h"
 #include "chrome/common/gfx/url_elider.h"
 #include "chrome/common/l10n_util.h"
@@ -411,6 +413,20 @@ std::wstring AutocompleteProvider::StringForURLDisplay(
       std::wstring());
 }
 
+void AutocompleteProvider::UpdateStarredStateOfMatches() {
+  if (matches_.empty())
+    return;
+
+  if (!profile_)
+    return;
+  BookmarkBarModel* bookmark_bar_model = profile_->GetBookmarkBarModel();
+  if (!bookmark_bar_model || !bookmark_bar_model->IsLoaded())
+    return;
+
+  for (ACMatches::iterator i = matches_.begin(); i != matches_.end(); ++i)
+    i->starred = bookmark_bar_model->IsBookmarked(GURL(i->destination_url));
+}
+
 // AutocompleteResult ---------------------------------------------------------
 
 // static
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index cdcc0d1..75b37d5 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -507,6 +507,10 @@ class AutocompleteProvider
   static size_t max_matches() { return max_matches_; }
 
  protected:
+  // Updates the starred state of each of the matches in matches_ from the
+  // profile's bookmark bar model.
+  void UpdateStarredStateOfMatches();
+
   // The profile associated with the AutocompleteProvider.  Reference is not
   // owned by us.
   Profile* profile_;
diff --git a/chrome/browser/autocomplete/history_contents_provider.cc b/chrome/browser/autocomplete/history_contents_provider.cc
index 564f832..b46a34e 100644
--- a/chrome/browser/autocomplete/history_contents_provider.cc
+++ b/chrome/browser/autocomplete/history_contents_provider.cc
@@ -109,7 +109,9 @@ void HistoryContentsProvider::Start(const AutocompleteInput& input,
     return;
   }
 
-  // TODO(sky): this needs to query BookmarkBarModel for starred entries.
+  // TODO(sky): re-enable providing suggestions from the user supplied title of
+  // bookmarks.
+
   if (!synchronous_only) {
     HistoryService* history = history_service_ ? history_service_ :
         profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index d0ee441..ee989f7 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -48,8 +48,6 @@
 #include "googleurl/src/url_util.h"
 #include "net/base/net_util.h"
 
-// TODO(sky): this needs to check and update starred state.
-
 HistoryURLProviderParams::HistoryURLProviderParams(
     const AutocompleteInput& input,
     bool trim_http,
@@ -236,6 +234,7 @@ void HistoryURLProvider::QueryComplete(
 
   done_ = true;
   matches_.swap(params->matches);
+  UpdateStarredStateOfMatches();
   listener_->OnProviderUpdate(true);
 }
 
@@ -670,6 +669,7 @@ void HistoryURLProvider::RunAutocompletePasses(const AutocompleteInput& input,
       // the not-yet-fixed-up What You Typed match, which is exactly what
       // matches_ currently contains, just swap them.
       matches_.swap(params->matches);
+      UpdateStarredStateOfMatches();
     }
   }
 
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 6f66694..3dc0030 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -221,8 +221,6 @@ void SearchProvider::StopSuggest() {
   fetcher_.reset();  // Stop any in-progress URL fetch.
   suggest_results_.clear();
   have_suggest_results_ = false;
-  star_request_consumer_.CancelAllRequests();
-  star_requests_pending_ = false;
 }
 
 void SearchProvider::OnGotMostRecentKeywordSearchTerms(
@@ -235,32 +233,6 @@ void SearchProvider::OnGotMostRecentKeywordSearchTerms(
   listener_->OnProviderUpdate(!history_results_.empty());
 }
 
-void SearchProvider::OnQueryURLComplete(HistoryService::Handle handle,
-                                        bool success,
-                                        const history::URLRow* url_row,
-                                        history::VisitVector* unused) {
-  bool is_starred =
-      (success && profile_ &&
-       profile_->GetBookmarkBarModel()->IsBookmarked(url_row->url()));
-  star_requests_pending_ = false;
-  // We can't just use star_request_consumer_.HasPendingRequests() here;
-  // see comment in ConvertResultsToAutocompleteMatches().
-  for (NavigationResults::iterator i(navigation_results_.begin());
-       i != navigation_results_.end(); ++i) {
-    if (i->star_request_handle == handle) {
-      i->star_request_handle = 0;
-      i->starred = is_starred;
-    } else if (i->star_request_handle) {
-      star_requests_pending_ = true;
-    }
-  }
-  if (!star_requests_pending_) {
-    // No more requests. Notify the observer.
-    ConvertResultsToAutocompleteMatches();
-    listener_->OnProviderUpdate(true);
-  }
-}
-
 bool SearchProvider::ParseSuggestResults(Value* root_val) {
   if (!root_val->IsType(Value::TYPE_LIST))
     return false;
@@ -333,19 +305,6 @@ bool SearchProvider::ParseSuggestResults(Value* root_val) {
     }
   }
 
-  // Request the star state for all URLs from the history service.
-  HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return true;
-
-  for (NavigationResults::iterator i(navigation_results_.begin());
-       i != navigation_results_.end(); ++i) {
-    i->star_request_handle = hs->QueryURL(GURL(i->url), false,
-        &star_request_consumer_,
-        NewCallback(this, &SearchProvider::OnQueryURLComplete));
-  }
-  star_requests_pending_ = !navigation_results_.empty();
-
   return true;
 }
 
@@ -381,8 +340,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
     // suggestions. If we can get more useful information about the score,
     // consider adding more results.
     matches_.push_back(NavigationToMatch(navigation_results_[0],
-                                         CalculateRelevanceForNavigation(0),
-                                         navigation_results_[0].starred));
+                                         CalculateRelevanceForNavigation(0)));
   }
 
   const size_t max_total_matches = max_matches() + 1;  // 1 for "what you typed"
@@ -392,16 +350,17 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   if (matches_.size() > max_total_matches)
     matches_.resize(max_total_matches);
 
+  UpdateStarredStateOfMatches();
+
   // We're done when both asynchronous subcomponents have finished.
   // We can't use CancelableRequestConsumer.HasPendingRequests() for
-  // history and star requests here.  A pending request is not cleared
-  // until after the completion callback has returned, but we've
-  // reached here from inside that callback.  HasPendingRequests()
-  // would therefore return true, and if this is the last thing left
-  // to calculate for this query, we'll never mark the query "done".
+  // history requests here.  A pending request is not cleared until after the
+  // completion callback has returned, but we've reached here from inside that
+  // callback.  HasPendingRequests() would therefore return true, and if this is
+  // the last thing left to calculate for this query, we'll never mark the query
+  // "done".
   done_ = !history_request_pending_ &&
-          !suggest_results_pending_ &&
-          !star_requests_pending_;
+          !suggest_results_pending_;
 }
 
 int SearchProvider::CalculateRelevanceForWhatYouTyped() const {
@@ -574,8 +533,7 @@ void SearchProvider::AddMatchToMap(const std::wstring& query_string,
 
 AutocompleteMatch SearchProvider::NavigationToMatch(
     const NavigationResult& navigation,
-    int relevance,
-    bool starred) {
+    int relevance) {
   AutocompleteMatch match(this, relevance, false);
   match.destination_url = navigation.url;
   match.contents = StringForURLDisplay(GURL(navigation.url), true);
@@ -592,7 +550,6 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
                                            ACMatchClassification::NONE,
                                            &match.description_class);
 
-  match.starred = starred;
   // 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.
diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h
index ab4b2ba..c130e58 100644
--- a/chrome/browser/autocomplete/search_provider.h
+++ b/chrome/browser/autocomplete/search_provider.h
@@ -58,9 +58,6 @@ class Value;
 // text.  It also starts a task to query the Suggest servers.  When that data
 // comes back, the provider creates and returns matches for the best
 // suggestions.
-//
-// TODO(pkasting): http://b/893701 This should eventually remember the user's
-// search history and use that to create/rank suggestions as well.
 class SearchProvider : public AutocompleteProvider,
                        public URLFetcher::Delegate,
                        public Task {
@@ -71,7 +68,6 @@ class SearchProvider : public AutocompleteProvider,
 #pragma warning(suppress: 4355)  // Okay to pass "this" here.
         timer_(new Timer(kQueryDelayMs, this, false)),
         fetcher_(NULL),
-        star_requests_pending_(false),
         history_request_pending_(false),
         have_history_results_(false),
         suggest_results_pending_(false),
@@ -99,9 +95,7 @@ class SearchProvider : public AutocompleteProvider,
   struct NavigationResult {
     NavigationResult(const std::wstring& url, const std::wstring& site_name)
         : url(url),
-          site_name(site_name),
-          star_request_handle(0),
-          starred(false) {
+          site_name(site_name) {
     }
 
     // The URL.
@@ -109,13 +103,6 @@ class SearchProvider : public AutocompleteProvider,
 
     // Name for the site.
     std::wstring site_name;
-
-    // If non-zero, there is a pending request to the history service to
-    // obtain the starred state.
-    HistoryService::Handle star_request_handle;
-
-    // Whether the URL has been starred.
-    bool starred;
   };
 
   typedef std::vector<std::wstring> SuggestResults;
@@ -140,14 +127,6 @@ class SearchProvider : public AutocompleteProvider,
       CancelableRequestProvider::Handle handle,
       HistoryResults* results);
 
-  // Notification from the history service that the star state for the URL
-  // is available. If this is the last url's star state that is being requested
-  // the listener is notified.
-  void OnQueryURLComplete(HistoryService::Handle handle,
-                          bool success,
-                          const history::URLRow* url_row,
-                          history::VisitVector* unused);
-
   // Parses the results from the Suggest server and stores up to kMaxMatches of
   // them in server_results_.  Returns whether parsing succeeded.
   bool ParseSuggestResults(Value* root_val);
@@ -177,8 +156,7 @@ class SearchProvider : public AutocompleteProvider,
                      MatchMap* map);
   // Returns an AutocompleteMatch for a navigational suggestion.
   AutocompleteMatch NavigationToMatch(const NavigationResult& query_string,
-                                      int relevance,
-                                      bool starred);
+                                      int relevance);
 
   // Trims "http:" and up to two subsequent slashes from |url|.  Returns the
   // number of characters that were trimmed.
@@ -201,12 +179,8 @@ class SearchProvider : public AutocompleteProvider,
                                   // TODO(pkasting): http://b/1162970  We
                                   // shouldn't need this.
 
-  // An object we can use to cancel history and star requests.
+  // An object we can use to cancel history requests.
   CancelableRequestConsumer history_request_consumer_;
-  CancelableRequestConsumerT<int, 0> star_request_consumer_;
-
-  // Whether we are waiting for star requests to finish.
-  bool star_requests_pending_;
 
   // Searches in the user's history that begin with the input text.
   HistoryResults history_results_;
diff --git a/chrome/browser/bookmark_bar_model.h b/chrome/browser/bookmark_bar_model.h
index a481004..1d35582 100644
--- a/chrome/browser/bookmark_bar_model.h
+++ b/chrome/browser/bookmark_bar_model.h
@@ -101,6 +101,9 @@ class BookmarkBarNode : public ChromeViews::TreeNode<BookmarkBarNode> {
   // Is this a URL?
   bool is_url() const { return type_ == history::StarredEntry::URL; }
 
+  // TODO(sky): Consider adding last visit time here, it'll greatly simplify
+  // HistoryContentsProvider.
+
  private:
   // Resets the properties of the node from the supplied entry.
   void Reset(const history::StarredEntry& entry);