Make DownloadHistory observe manager, items

chrome/browser/download/download_browsertest.cc

Index: chrome/browser/download/download_browsertest.cc
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 06fc2a41a9018738b0499794394e875aa72161d1..a94dad41d7cef666c30c0ff01ece63f661fe270d 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_history.h"
+#include "chrome/browser/download/download_persistent_store_info.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_shelf.h"
@@ -49,7 +50,6 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "content/public/browser/download_save_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/notification_source.h"
@@ -72,7 +72,6 @@ using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadItem;
 using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
 using content::DownloadUrlParameters;
 using content::WebContents;
 using extensions::Extension;
@@ -86,58 +85,6 @@ const FilePath kGoodCrxPath(FILE_PATH_LITERAL("extensions/good.crx"));
 const char kLargeThemeCrxId[] = "pjpgmfcmabopnnfonnhmdjglfpjjfkbf";
 const FilePath kLargeThemePath(FILE_PATH_LITERAL("extensions/theme2.crx"));
 
-// Get History Information.
-class DownloadsHistoryDataCollector {
- public:
-  DownloadsHistoryDataCollector(int64 download_db_handle,
-                                DownloadManager* manager)
-      : result_valid_(false),
-        download_db_handle_(download_db_handle) {
-    HistoryService* hs = HistoryServiceFactory::GetForProfile(
-        Profile::FromBrowserContext(manager->GetBrowserContext()),
-        Profile::EXPLICIT_ACCESS);
-    DCHECK(hs);
-    hs->QueryDownloads(
-        &callback_consumer_,
-        base::Bind(&DownloadsHistoryDataCollector::OnQueryDownloadsComplete,
-                   base::Unretained(this)));
-
-    // TODO(rdsmith): Move message loop out of constructor.
-    // Cannot complete immediately because the history backend runs on a
-    // separate thread, so we can assume that the RunMessageLoop below will
-    // be exited by the Quit in OnQueryDownloadsComplete.
-    content::RunMessageLoop();
-  }
-
-  bool GetDownloadsHistoryEntry(DownloadPersistentStoreInfo* result) {
-    DCHECK(result);
-    *result = result_;
-    return result_valid_;
-  }
-
- private:
-  void OnQueryDownloadsComplete(
-      std::vector<DownloadPersistentStoreInfo>* entries) {
-    result_valid_ = false;
-    for (std::vector<DownloadPersistentStoreInfo>::const_iterator it =
-             entries->begin();
-         it != entries->end(); ++it) {
-      if (it->db_handle == download_db_handle_) {
-        result_ = *it;
-        result_valid_ = true;
-      }
-    }
-    MessageLoopForUI::current()->Quit();
-  }
-
-  DownloadPersistentStoreInfo result_;
-  bool result_valid_;
-  int64 download_db_handle_;
-  CancelableRequestConsumer callback_consumer_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadsHistoryDataCollector);
-};
-
 // Mock that simulates a permissions dialog where the user denies
 // permission to install.  TODO(skerner): This could be shared with
 // extensions tests.  Find a common place for this class.
@@ -197,6 +144,13 @@ class TestRenderViewContextMenu : public RenderViewContextMenu {
   DISALLOW_COPY_AND_ASSIGN(TestRenderViewContextMenu);
 };
 
+void HistoryQueryCallback(
+    DownloadHistory::InfoVector* out,
+    DownloadHistory::InfoVector* in) {
+  in->swap(*out);
+  MessageLoop::current()->Quit();
+}
+
 }  // namespace
 
 // While an object of this class exists, it will mock out download
@@ -865,6 +819,54 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeType) {
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistory) {
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  HistoryService* hs = HistoryServiceFactory::GetForProfile(
+      browser()->profile(), Profile::EXPLICIT_ACCESS);
+  CancelableRequestConsumer history_consumer;
+  DownloadHistory::InfoVector infos;
+  hs->QueryDownloads(&history_consumer, base::Bind(
+      &HistoryQueryCallback, &infos));
+  MessageLoop::current()->Run();
+  EXPECT_EQ(0, static_cast<int>(infos.size()));
+
+  // Download the file and wait.  We do not expect the Select File dialog.
+  DownloadAndWait(browser(), url);
+  CheckDownload(browser(), file, file);
+
+  DownloadManager* manager = DownloadManagerForBrowser(browser());
+  DownloadManager::DownloadVector downloads;
+  manager->SearchDownloads(string16(), &downloads);
+  EXPECT_EQ(1, static_cast<int>(downloads.size()));
+
+  hs->QueryDownloads(&history_consumer, base::Bind(
+      &HistoryQueryCallback, &infos));
+  MessageLoop::current()->Run();
+  EXPECT_EQ(1, static_cast<int>(infos.size()));
+  EXPECT_EQ(downloads[0]->GetFullPath().value(), infos[0].path.value());
+  EXPECT_EQ(url.spec(), infos[0].url.spec());
+  EXPECT_EQ("", infos[0].referrer_url.spec());
+  EXPECT_EQ(downloads[0]->GetStartTime().ToTimeT(),
+            infos[0].start_time.ToTimeT());
+  EXPECT_EQ(downloads[0]->GetEndTime().ToTimeT(), infos[0].end_time.ToTimeT());
+  EXPECT_EQ(downloads[0]->GetReceivedBytes(), infos[0].received_bytes);
+  EXPECT_EQ(downloads[0]->GetTotalBytes(), infos[0].total_bytes);
+  EXPECT_EQ(DownloadItem::COMPLETE, infos[0].state);
+  EXPECT_EQ(1, infos[0].db_handle);
+  EXPECT_EQ(false, infos[0].opened);
+
+  downloads[0]->MockDownloadOpenForTesting();
+  downloads[0]->OpenDownload();
+  EXPECT_TRUE(downloads[0]->GetOpened());
+  hs->QueryDownloads(&history_consumer, base::Bind(
+      &HistoryQueryCallback, &infos));
+  MessageLoop::current()->Run();
+  EXPECT_EQ(1, static_cast<int>(infos.size()));
+  EXPECT_EQ(1, infos[0].db_handle);
+  EXPECT_EQ(true, infos[0].opened);
+}
+
 #if defined(OS_WIN)
 // Download a file and confirm that the zone identifier (on windows)
 // is set to internet.
@@ -1529,42 +1531,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) {
   EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
 }
 
-// Confirm a download makes it into the history properly.
-IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) {
-  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
-  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
-  FilePath origin_file(OriginFile(file));
-  int64 origin_size;
-  file_util::GetFileSize(origin_file, &origin_size);
-
-  // Download the file and wait.  We do not expect the Select File dialog.
-  DownloadAndWait(browser(), url);
-
-  // Get details of what downloads have just happened.
-  std::vector<DownloadItem*> downloads;
-  GetDownloads(browser(), &downloads);
-  ASSERT_EQ(1u, downloads.size());
-  int64 db_handle = downloads[0]->GetDbHandle();
-
-  // Check state.
-  EXPECT_EQ(1, browser()->tab_count());
-  CheckDownload(browser(), file, file);
-  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
-
-  // Check history results.
-  DownloadsHistoryDataCollector history_collector(
-      db_handle,
-      DownloadManagerForBrowser(browser()));
-  DownloadPersistentStoreInfo info;
-  EXPECT_TRUE(history_collector.GetDownloadsHistoryEntry(&info)) << db_handle;
-  EXPECT_EQ(file, info.path.BaseName());
-  EXPECT_EQ(url, info.url);
-  // Ignore start_time.
-  EXPECT_EQ(origin_size, info.received_bytes);
-  EXPECT_EQ(origin_size, info.total_bytes);
-  EXPECT_EQ(DownloadItem::COMPLETE, info.state);
-}
-
 // Test for crbug.com/14505. This tests that chrome:// urls are still functional
 // after download of a file while viewing another chrome://.
 IN_PROC_BROWSER_TEST_F(DownloadTest, ChromeURLAfterDownload) {
@@ -1816,56 +1782,46 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxLargeTheme) {
   ASSERT_TRUE(extension_service->GetExtensionById(kLargeThemeCrxId, false));
 }
 
-// Sort download items by db_handle.
+// Sort download items by id.
 static bool DownloadItemSorter(DownloadItem* d1, DownloadItem* d2) {
-  return d1->GetDbHandle() < d2->GetDbHandle();
+  return d1->GetId() < d2->GetId();
 }
 
 // Confirm that searching through the history works properly
 IN_PROC_BROWSER_TEST_F(DownloadTest, SearchDownloads) {
-  // Downloads to populate history with.
-  base::Time current(base::Time::Now());
-  DownloadPersistentStoreInfo population_entries[] = {
-    DownloadPersistentStoreInfo(
-        FilePath(FILE_PATH_LITERAL("/path/to/file")),
-        GURL("http://www.google.com/fantasy_download"),
-        GURL(""),
-        current - base::TimeDelta::FromMinutes(5),
-        current,
-        128,
-        128,
-        DownloadItem::COMPLETE,
-        1,
-        false),
-    DownloadPersistentStoreInfo(
-        FilePath(FILE_PATH_LITERAL("/path/to/another_file")),
-        GURL("http://www.google.com/reality_download"),
-        GURL(""),
-        current - base::TimeDelta::FromMinutes(10),
-        current,
-        256,
-        256,
-        DownloadItem::COMPLETE,
-        2,
-        false),
-    DownloadPersistentStoreInfo(
-        FilePath(FILE_PATH_LITERAL("/different_path/to/another_file")),
-        GURL("http://www.izzle.com/not_really_a_download"),
-        GURL(""),
-        current - base::TimeDelta::FromMinutes(15),
-        current,
-        512,
-        512,
-        DownloadItem::COMPLETE,
-        3,
-        true)
-  };
-  std::vector<DownloadPersistentStoreInfo> entries(
-      population_entries, population_entries + arraysize(population_entries));
-
   // Populate the manager.
   DownloadManager* manager = DownloadManagerForBrowser(browser());
-  manager->OnPersistentStoreQueryComplete(&entries);
+  base::Time current(base::Time::Now());
+  manager->CreateDownloadItem(
+      FilePath(FILE_PATH_LITERAL("/path/to/file")),
+      GURL("http://www.google.com/fantasy_download"),
+      GURL(""),
+      current - base::TimeDelta::FromMinutes(5),
+      current,
+      128,
+      128,
+      DownloadItem::COMPLETE,
+      false);
+  manager->CreateDownloadItem(
+      FilePath(FILE_PATH_LITERAL("/path/to/another_file")),
+      GURL("http://www.google.com/reality_download"),
+      GURL(""),
+      current - base::TimeDelta::FromMinutes(10),
+      current,
+      256,
+      256,
+      DownloadItem::COMPLETE,
+      false);
+  manager->CreateDownloadItem(
+      FilePath(FILE_PATH_LITERAL("/different_path/to/another_file")),
+      GURL("http://www.izzle.com/not_really_a_download"),
+      GURL(""),
+      current - base::TimeDelta::FromMinutes(15),
+      current,
+      512,
+      512,
+      DownloadItem::COMPLETE,
+      true);
 
   // Do some searches and check the results.
   std::vector<DownloadItem*> search_results;
@@ -1889,7 +1845,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SearchDownloads) {
     EXPECT_EQ(128, d1->GetReceivedBytes());
     EXPECT_EQ(128, d1->GetTotalBytes());
     EXPECT_EQ(DownloadItem::COMPLETE, d1->GetState());
-    EXPECT_EQ(1, d1->GetDbHandle());
     EXPECT_FALSE(d1->GetOpened());
 
     EXPECT_EQ(FilePath(FILE_PATH_LITERAL("/path/to/another_file")),
@@ -1902,7 +1857,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SearchDownloads) {
     EXPECT_EQ(256, d2->GetReceivedBytes());
     EXPECT_EQ(256, d2->GetTotalBytes());
     EXPECT_EQ(DownloadItem::COMPLETE, d2->GetState());
-    EXPECT_EQ(2, d2->GetDbHandle());
     EXPECT_FALSE(d2->GetOpened());
 
     EXPECT_EQ(FilePath(FILE_PATH_LITERAL("/different_path/to/another_file")),
@@ -1915,7 +1869,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SearchDownloads) {
     EXPECT_EQ(512, d3->GetReceivedBytes());
     EXPECT_EQ(512, d3->GetTotalBytes());
     EXPECT_EQ(DownloadItem::COMPLETE, d3->GetState());
-    EXPECT_EQ(3, d3->GetDbHandle());
     EXPECT_TRUE(d3->GetOpened());
   }
   search_results.clear();
@@ -1925,24 +1878,18 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SearchDownloads) {
   ASSERT_EQ(2u, search_results.size());
   std::sort(search_results.begin(), search_results.end(),
             DownloadItemSorter);
-  EXPECT_EQ(1, search_results[0]->GetDbHandle());
-  EXPECT_EQ(2, search_results[1]->GetDbHandle());
   search_results.clear();
 
   manager->SearchDownloads(UTF8ToUTF16("real"), &search_results);
   ASSERT_EQ(2u, search_results.size());
   std::sort(search_results.begin(), search_results.end(),
             DownloadItemSorter);
-  EXPECT_EQ(2, search_results[0]->GetDbHandle());
-  EXPECT_EQ(3, search_results[1]->GetDbHandle());
   search_results.clear();
 
   manager->SearchDownloads(UTF8ToUTF16("another_file"), &search_results);
   ASSERT_EQ(2u, search_results.size());
   std::sort(search_results.begin(), search_results.end(),
             DownloadItemSorter);
-  EXPECT_EQ(2, search_results[0]->GetDbHandle());
-  EXPECT_EQ(3, search_results[1]->GetDbHandle());
   search_results.clear();
 }