Do at least some rudimentary sorting of bookmarked URLs in the omnibox dropdown (existing sort was effectively random). Patch by Pierre-Antoine LaFayette (see http://codereview.chromium.org/165455 ), r=sky,me, tweaked.

BUG=16230
TEST=In the omnibox dropdown, bookmarked URLs that have been typed more often should be ranked above those that have been typed less often.

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

2 files changed, 21 insertions, 15 deletions

diff --git a/chrome/browser/autocomplete/history_contents_provider.cc b/chrome/browser/autocomplete/history_contents_provider.cc
index c81ccd8..558911c 100644
--- a/chrome/browser/autocomplete/history_contents_provider.cc
+++ b/chrome/browser/autocomplete/history_contents_provider.cc
@@ -71,12 +71,10 @@ void HistoryContentsProvider::Start(const AutocompleteInput& input,
     return;
   }
 
-  // Change input type and reset relevance counters, so matches will be marked
-  // up properly.
+  // Change input type so matches will be marked up properly.
   input_type_ = input.type();
   trim_http_ = !url_util::FindAndCompareScheme(WideToUTF8(input.text()),
                                                chrome::kHttpScheme, NULL);
-  star_title_count_ = star_contents_count_ = title_count_ = contents_count_ = 0;
 
   // Decide what to do about any previous query/results.
   if (!minimal_changes) {
@@ -150,11 +148,20 @@ void HistoryContentsProvider::QueryComplete(HistoryService::Handle handle,
 }
 
 void HistoryContentsProvider::ConvertResults() {
+  // Reset the relevance counters so that result relevance won't vary on
+  // subsequent passes of ConvertResults.
+  star_title_count_ = star_contents_count_ = title_count_ = contents_count_ = 0;
+
   // Make the result references and score the results.
   std::vector<MatchReference> result_refs;
   result_refs.reserve(results_.size());
-  for (size_t i = 0; i < results_.size(); i++) {
-    MatchReference ref(&results_[i], CalculateRelevance(results_[i]));
+
+  // Results are sorted in decreasing order so we run the loop backwards so that
+  // the relevance increment favors the higher ranked results.
+  for (std::vector<history::URLResult*>::const_reverse_iterator i =
+       results_.rbegin(); i != results_.rend(); ++i) {
+    history::URLResult* result = *i;
+    MatchReference ref(result, CalculateRelevance(*result));
     result_refs.push_back(ref);
   }
 
@@ -275,8 +282,7 @@ void HistoryContentsProvider::QueryBookmarks(const AutocompleteInput& input) {
   if (!bookmark_model)
     return;
 
-  DCHECK(results_.size() == 0);  // When we get here the results should be
-                                 // empty.
+  DCHECK(results_.empty());
 
   TimeTicks start_time = TimeTicks::Now();
   std::vector<bookmark_utils::TitleMatch> matches;
diff --git a/chrome/browser/autocomplete/history_contents_provider_unittest.cc b/chrome/browser/autocomplete/history_contents_provider_unittest.cc
index 8930353..84dde8f 100644
--- a/chrome/browser/autocomplete/history_contents_provider_unittest.cc
+++ b/chrome/browser/autocomplete/history_contents_provider_unittest.cc
@@ -101,10 +101,10 @@ TEST_F(HistoryContentsProviderTest, Body) {
   // The results should be the first two pages, in decreasing order.
   const ACMatches& m = matches();
   ASSERT_EQ(2U, m.size());
-  EXPECT_EQ(test_entries[1].url, m[0].destination_url.spec());
-  EXPECT_STREQ(test_entries[1].title, m[0].description.c_str());
-  EXPECT_EQ(test_entries[0].url, m[1].destination_url.spec());
-  EXPECT_STREQ(test_entries[0].title, m[1].description.c_str());
+  EXPECT_EQ(test_entries[0].url, m[0].destination_url.spec());
+  EXPECT_STREQ(test_entries[0].title, m[0].description.c_str());
+  EXPECT_EQ(test_entries[1].url, m[1].destination_url.spec());
+  EXPECT_STREQ(test_entries[1].title, m[1].description.c_str());
 }
 
 TEST_F(HistoryContentsProviderTest, Title) {
@@ -114,10 +114,10 @@ TEST_F(HistoryContentsProviderTest, Title) {
   // The results should be the first two pages.
   const ACMatches& m = matches();
   ASSERT_EQ(2U, m.size());
-  EXPECT_EQ(test_entries[1].url, m[0].destination_url.spec());
-  EXPECT_STREQ(test_entries[1].title, m[0].description.c_str());
-  EXPECT_EQ(test_entries[0].url, m[1].destination_url.spec());
-  EXPECT_STREQ(test_entries[0].title, m[1].description.c_str());
+  EXPECT_EQ(test_entries[0].url, m[0].destination_url.spec());
+  EXPECT_STREQ(test_entries[0].title, m[0].description.c_str());
+  EXPECT_EQ(test_entries[1].url, m[1].destination_url.spec());
+  EXPECT_STREQ(test_entries[1].title, m[1].description.c_str());
 }
 
 // The "minimal changes" flag should mean that we don't re-query the DB.