History: Properly merge timestamps when removing duplicates

This fixes a bug where timestamps were not being merged properly, with only the
first timestamp being put in.

Also adds a unit test that checks for the correct behavior + minor clean-up.

BUG=243373
TEST=unit_tests --gtest_filter=HistoryUITest.*
R=jhawkins@chromium.org

Review URL: https://codereview.chromium.org/15755009

Patch from Sergiu Iordache <sergiu@chromium.org>.

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

3 files changed, 38 insertions, 14 deletions

diff --git a/chrome/browser/ui/webui/history_ui.cc b/chrome/browser/ui/webui/history_ui.cc
index f0eca99..c707851 100644
--- a/chrome/browser/ui/webui/history_ui.cc
+++ b/chrome/browser/ui/webui/history_ui.cc
@@ -668,11 +668,15 @@ void BrowsingHistoryHandler::HandleRemoveBookmark(const ListValue* args) {
 }
 
 // static
-void BrowsingHistoryHandler::RemoveDuplicateResults(
+void BrowsingHistoryHandler::MergeDuplicateResults(
     std::vector<BrowsingHistoryHandler::HistoryEntry>* results) {
   std::vector<BrowsingHistoryHandler::HistoryEntry> new_results;
+  // Pre-reserve the size of the new vector. Since we're working with pointers
+  // later on not doing this could lead to the vector being resized and to
+  // pointers to invalid locations.
+  new_results.reserve(results->size());
   // Maps a URL to the most recent entry on a particular day.
-  std::map<GURL,BrowsingHistoryHandler::HistoryEntry>
+  std::map<GURL,BrowsingHistoryHandler::HistoryEntry*>
       current_day_entries;
 
   // Keeps track of the day that |current_day_urls| is holding the URLs for,
@@ -692,13 +696,12 @@ void BrowsingHistoryHandler::RemoveDuplicateResults(
 
     // Keep this visit if it's the first visit to this URL on the current day.
     if (current_day_entries.count(it->url) == 0) {
-      current_day_entries.insert(
-          std::pair<GURL,BrowsingHistoryHandler::HistoryEntry>(it->url, *it));
       new_results.push_back(*it);
+      current_day_entries[it->url] = &new_results.back();
     } else {
       // Keep track of the timestamps of all visits to the URL on the same day.
       BrowsingHistoryHandler::HistoryEntry* entry =
-          &current_day_entries[it->url];
+          current_day_entries[it->url];
       entry->all_timestamps.insert(
           it->all_timestamps.begin(), it->all_timestamps.end());
 
@@ -739,7 +742,7 @@ void BrowsingHistoryHandler::ReturnResultsToFrontEnd() {
     query_results_.insert(query_results_.end(),
                           web_history_query_results_.begin(),
                           web_history_query_results_.end());
-    RemoveDuplicateResults(&query_results_);
+    MergeDuplicateResults(&query_results_);
 
     if (local_result_count) {
       // In the best case, we expect that all local results are duplicated on
diff --git a/chrome/browser/ui/webui/history_ui.h b/chrome/browser/ui/webui/history_ui.h
index cdbbc3f..f51bb6b 100644
--- a/chrome/browser/ui/webui/history_ui.h
+++ b/chrome/browser/ui/webui/history_ui.h
@@ -106,9 +106,10 @@ class BrowsingHistoryHandler : public content::WebUIMessageHandler,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // Removes duplicate entries from the query results, only retaining the most
-  // recent visit to a URL on a particular day.
-  static void RemoveDuplicateResults(
+  // Merges duplicate entries from the query results, only retaining the most
+  // recent visit to a URL on a particular day. That visit contains the
+  // timestamps of the other visits.
+  static void MergeDuplicateResults(
       std::vector<BrowsingHistoryHandler::HistoryEntry>* results);
 
  private:
diff --git a/chrome/browser/ui/webui/history_ui_unittest.cc b/chrome/browser/ui/webui/history_ui_unittest.cc
index 242ab39..b820ec0 100644
--- a/chrome/browser/ui/webui/history_ui_unittest.cc
+++ b/chrome/browser/ui/webui/history_ui_unittest.cc
@@ -29,6 +29,7 @@ void AddQueryResults(
     entry.time = baseline_time +
                  base::TimeDelta::FromHours(test_results[i].hour_offset);
     entry.url = GURL(test_results[i].url);
+    entry.all_timestamps.insert(entry.time.ToInternalValue());
     results->push_back(entry);
   }
 }
@@ -46,9 +47,9 @@ bool ResultEquals(
 
 }  // namespace
 
-// Tests that the RemoveDuplicateResults method correctly removes duplicate
+// Tests that the MergeDuplicateResults method correctly removes duplicate
 // visits to the same URL on the same day.
-TEST(HistoryUITest, RemoveDuplicateResults) {
+TEST(HistoryUITest, MergeDuplicateResults) {
   {
     // Basic test that duplicates on the same day are removed.
     TestResult test_data[] = {
@@ -59,7 +60,7 @@ TEST(HistoryUITest, RemoveDuplicateResults) {
     };
     std::vector<BrowsingHistoryHandler::HistoryEntry> results;
     AddQueryResults(test_data, arraysize(test_data), &results);
-    BrowsingHistoryHandler::RemoveDuplicateResults(&results);
+    BrowsingHistoryHandler::MergeDuplicateResults(&results);
 
     ASSERT_EQ(2U, results.size());
     EXPECT_TRUE(ResultEquals(results[0], test_data[3]));
@@ -75,7 +76,7 @@ TEST(HistoryUITest, RemoveDuplicateResults) {
     };
     std::vector<BrowsingHistoryHandler::HistoryEntry> results;
     AddQueryResults(test_data, arraysize(test_data), &results);
-    BrowsingHistoryHandler::RemoveDuplicateResults(&results);
+    BrowsingHistoryHandler::MergeDuplicateResults(&results);
 
     ASSERT_EQ(2U, results.size());
     EXPECT_TRUE(ResultEquals(results[0], test_data[2]));
@@ -99,7 +100,7 @@ TEST(HistoryUITest, RemoveDuplicateResults) {
     };
     std::vector<BrowsingHistoryHandler::HistoryEntry> results;
     AddQueryResults(test_data, arraysize(test_data), &results);
-    BrowsingHistoryHandler::RemoveDuplicateResults(&results);
+    BrowsingHistoryHandler::MergeDuplicateResults(&results);
 
     ASSERT_EQ(4U, results.size());
     EXPECT_TRUE(ResultEquals(results[0], test_data[7]));
@@ -107,4 +108,23 @@ TEST(HistoryUITest, RemoveDuplicateResults) {
     EXPECT_TRUE(ResultEquals(results[2], test_data[3]));
     EXPECT_TRUE(ResultEquals(results[3], test_data[2]));
   }
+
+  {
+    // Test that timestamps for duplicates are properly saved.
+    TestResult test_data[] = {
+      { "http://google.com", 0 },
+      { "http://google.de", 1 },
+      { "http://google.com", 2 },
+      { "http://google.com", 3 }  // Most recent.
+    };
+    std::vector<BrowsingHistoryHandler::HistoryEntry> results;
+    AddQueryResults(test_data, arraysize(test_data), &results);
+    BrowsingHistoryHandler::MergeDuplicateResults(&results);
+
+    ASSERT_EQ(2U, results.size());
+    EXPECT_TRUE(ResultEquals(results[0], test_data[3]));
+    EXPECT_TRUE(ResultEquals(results[1], test_data[1]));
+    EXPECT_EQ(3u, results[0].all_timestamps.size());
+    EXPECT_EQ(1u, results[1].all_timestamps.size());
+  }
 }