Make DownloadHistory observe manager, items

chrome/browser/extensions/api/downloads/downloads_api_unittest.cc

Index: chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
index 702d89ebc291b542148129a5a750981d75347591..b1b303238e5f2e6c9a8ac5b89ac8b219feb607a4 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
+#include "chrome/browser/history/download_persistent_store_info.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -33,7 +34,6 @@
 #include "content/public/browser/browser_thread.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/notification_service.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
@@ -58,7 +58,6 @@ using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadItem;
 using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
 using content::URLRequestSlowDownloadJob;
 
 namespace events = extensions::event_names;
@@ -357,24 +356,19 @@ class DownloadExtensionTest : public ExtensionApiTest {
                               DownloadManager::DownloadVector* items) {
     DownloadIdComparator download_id_comparator;
     base::Time current = base::Time::Now();
-    std::vector<DownloadPersistentStoreInfo> entries;
-    entries.reserve(count);
+    items->clear();
+    GetOnRecordManager()->GetAllDownloads(items);
+    CHECK_EQ(0, static_cast<int>(items->size()));
     for (size_t i = 0; i < count; ++i) {
-      DownloadPersistentStoreInfo entry(
+      DownloadItem* item = GetOnRecordManager()->CreateDownloadItem(
           downloads_directory().Append(history_info[i].filename),
           GURL(), GURL(),    // URL, referrer
           current, current,  // start_time, end_time
           1, 1,              // received_bytes, total_bytes
           history_info[i].state,  // state
-          i + 1,                  // db_handle
           false);                 // opened
-      entries.push_back(entry);
+      items->push_back(item);
     }
-    GetOnRecordManager()->OnPersistentStoreQueryComplete(&entries);
-    GetOnRecordManager()->GetAllDownloads(items);
-    EXPECT_EQ(count, items->size());
-    if (count != items->size())
-      return false;
 
     // Order by ID so that they are in the order that we created them.
     std::sort(items->begin(), items->end(), download_id_comparator);
@@ -583,6 +577,10 @@ class MockIconExtractorImpl : public DownloadFileIconExtractor {
   IconURLCallback      callback_;
 };
 
+bool ItemNotInProgress(DownloadItem* item) {
+  return !item->IsInProgress();
+}
+
 // Cancels the underlying DownloadItem when the ScopedCancellingItem goes out of
 // scope. Like a scoped_ptr, but for DownloadItems.
 class ScopedCancellingItem {
@@ -590,6 +588,9 @@ class ScopedCancellingItem {
   explicit ScopedCancellingItem(DownloadItem* item) : item_(item) {}
   ~ScopedCancellingItem() {
     item_->Cancel(true);
+    content::DownloadUpdatedObserver observer(
+        item_, base::Bind(&ItemNotInProgress));
+    observer.WaitForEvent();
   }
   DownloadItem* operator*() { return item_; }
   DownloadItem* operator->() { return item_; }
@@ -612,6 +613,9 @@ class ScopedItemVectorCanceller {
          item != items_->end(); ++item) {
       if ((*item)->IsInProgress())
         (*item)->Cancel(true);
+      content::DownloadUpdatedObserver observer(
+          (*item), base::Bind(&ItemNotInProgress));
+      observer.WaitForEvent();
     }
   }
 
@@ -897,16 +901,13 @@ UIThreadExtensionFunction* MockedGetFileIconFunction(
   return function.release();
 }
 
-bool WaitForPersisted(DownloadItem* item) {
-  return item->IsPersisted();
-}
-
 // Test downloads.getFileIcon() on in-progress, finished, cancelled and deleted
 // download items.
 IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
     DownloadExtensionTest_FileIcon_Active) {
   DownloadItem* download_item = CreateSlowTestDownload();
   ASSERT_TRUE(download_item);
+  EXPECT_FALSE(download_item->GetTargetFilePath().empty());

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

I'm confused by this test.  It seems to rely on a quirk of the current
implementation that we're probably going to change at some point.  The target
file path is set during download filename determination, and
CreateSlowTestDownload() returns when it's got a Download to the IN_PROGRESS
state.  Is there any reason these tests should fail if we ever make the download
visible before filename determination?

[benjhayden, 2012/11/12 18:44:16]

Discussed offline.
The item is already available before filename determination.
DownloadsGetFileIconFunction relies on GetTargetFilePath.
DownloadsGetFileIconFunction would fail mysteriously if
DownloadTestObserverInProgress::IsDownloadInFinalState() did not check
GetTargetFilePath.
If this expect fails, then it will clarify why DownloadsGetFileIconFunction also
fails.
Changed to ASSERT.

   std::string args32(base::StringPrintf("[%d, {\"size\": 32}]",
                      download_item->GetId()));
   std::string result_string;
@@ -943,17 +944,14 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   // Now create another download.
   download_item = CreateSlowTestDownload();
-  args32 = base::StringPrintf("[%d, {\"size\": 32}]", download_item->GetId());
   ASSERT_TRUE(download_item);
-
-  // http://crbug.com/154995
-  content::DownloadUpdatedObserver persisted(
-      download_item, base::Bind(&WaitForPersisted));
-  persisted.WaitForEvent();
+  EXPECT_FALSE(download_item->GetTargetFilePath().empty());
+  args32 = base::StringPrintf("[%d, {\"size\": 32}]", download_item->GetId());
 
   // Cancel the download. As long as the download has a target path, we should
   // be able to query the file icon.
   download_item->Cancel(true);
+  EXPECT_FALSE(download_item->GetTargetFilePath().empty());

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

What are you testing here?

[benjhayden, 2012/11/12 18:44:16]

The same as above, but for a different DownloadItem.

[Randy Smith (Not in Mondays), 2012/11/12 19:24:30]

Should this also be an ASSERT?  (Up to you.)

[benjhayden, 2012/11/12 19:59:54]

Done.

   // Let cleanup complete on the FILE thread.
   content::RunAllPendingInMessageLoop(BrowserThread::FILE);
   // Check the path passed to the icon extractor post-cancellation.