Replace the DownloadFileManager with direct ownership of DownloadFileImpl

content/browser/download/download_browsertest.cc

Index: content/browser/download/download_browsertest.cc
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index 46030b1e08c42065384c72aa98fd76f078939263..1b057fb2bdbb8e318919811c12d6d9e9d6e04a1c 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -10,11 +10,9 @@
 #include "base/scoped_temp_dir.h"
 #include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_file_impl.h"
-#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/power_save_blocker.h"
-#include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_utils.h"
@@ -44,15 +42,17 @@ static DownloadManagerImpl* DownloadManagerForShell(Shell* shell) {
 class DownloadFileWithDelay : public DownloadFileImpl {
  public:
   DownloadFileWithDelay(
-      const DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream,
-      DownloadRequestHandleInterface* request_handle,
-      scoped_refptr<content::DownloadManager> download_manager,
-      bool calculate_hash,
-      scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
-      const net::BoundNetLog& bound_net_log,
-      // |owner| is required to outlive the DownloadFileWithDelay.
-      DownloadFileWithDelayFactory* owner);
+    const content::DownloadSaveInfo& save_info,
+    const FilePath& default_download_directory,
+    const GURL& url,
+    const GURL& referrer_url,
+    int64 received_bytes,
+    bool calculate_hash,
+    scoped_ptr<content::ByteStreamReader> stream,
+    const net::BoundNetLog& bound_net_log,
+    scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
+    base::WeakPtr<content::DownloadDestinationObserver> observer,
+    base::WeakPtr<DownloadFileWithDelayFactory> owner);
 
   virtual ~DownloadFileWithDelay();
 
@@ -79,26 +79,34 @@ class DownloadFileWithDelay : public DownloadFileImpl {
       DownloadFileWithDelayFactory* factory,
       const base::Closure& original_callback);
 
-  // May only be used on the UI thread.
-  DownloadFileWithDelayFactory* owner_;
+  // This variable may only be read on the FILE thread, and may only be
+  // indirected through (e.g. methods on DownloadFileWithDelayFactory called)
+  // on the UI thread.  This is because after construction,
+  // DownloadFileWithDelay lives on the file thread, but
+  // DownloadFileWithDelayFactory is purely a UI thread object.
+  base::WeakPtr<DownloadFileWithDelayFactory> owner_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadFileWithDelay);
 };
 
+// All routines on this class must be called on the UI thread.
 class DownloadFileWithDelayFactory : public DownloadFileFactory {
  public:
   DownloadFileWithDelayFactory();
   virtual ~DownloadFileWithDelayFactory();
 
   // DownloadFileFactory interface.
-  virtual content::DownloadFile* CreateFile(
-      DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream,
-      DownloadManager* download_manager,
+  virtual DownloadFile* CreateFile(
+      const content::DownloadSaveInfo& save_info,
+      const FilePath& default_download_directory,
+      GURL url,
+      GURL referrer_url,
+      int64 received_bytes,
       bool calculate_hash,
-      const net::BoundNetLog& bound_net_log) OVERRIDE;
+      scoped_ptr<content::ByteStreamReader> stream,
+      const net::BoundNetLog& bound_net_log,
+      base::WeakPtr<content::DownloadDestinationObserver> observer) OVERRIDE;
 
-  // Must all be called on the UI thread.
   void AddRenameCallback(base::Closure callback);
   void AddDetachCallback(base::Closure callback);
   void GetAllRenameCallbacks(std::vector<base::Closure>* results);
@@ -109,6 +117,7 @@ class DownloadFileWithDelayFactory : public DownloadFileFactory {
   void WaitForSomeCallback();
 
  private:
+  base::WeakPtrFactory<DownloadFileWithDelayFactory> weak_ptr_factory_;
   std::vector<base::Closure> rename_callbacks_;
   std::vector<base::Closure> detach_callbacks_;
   bool waiting_;
@@ -117,17 +126,21 @@ class DownloadFileWithDelayFactory : public DownloadFileFactory {
 };
 
 DownloadFileWithDelay::DownloadFileWithDelay(
-      const DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream,
-      DownloadRequestHandleInterface* request_handle,
-      scoped_refptr<content::DownloadManager> download_manager,
-      bool calculate_hash,
-      scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
-      const net::BoundNetLog& bound_net_log,
-      DownloadFileWithDelayFactory* owner)
-    : DownloadFileImpl(info, stream.Pass(), request_handle, download_manager,
-                       calculate_hash, power_save_blocker.Pass(),
-                       bound_net_log),
+    const content::DownloadSaveInfo& save_info,
+    const FilePath& default_download_directory,
+    const GURL& url,
+    const GURL& referrer_url,
+    int64 received_bytes,
+    bool calculate_hash,
+    scoped_ptr<content::ByteStreamReader> stream,
+    const net::BoundNetLog& bound_net_log,
+    scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
+    base::WeakPtr<content::DownloadDestinationObserver> observer,
+    base::WeakPtr<DownloadFileWithDelayFactory> owner)
+    : DownloadFileImpl(
+        save_info, default_download_directory, url, referrer_url,
+        received_bytes, calculate_hash, stream.Pass(), bound_net_log,
+        power_save_blocker.Pass(), observer),
       owner_(owner) {}
 
 DownloadFileWithDelay::~DownloadFileWithDelay() {}
@@ -139,14 +152,14 @@ void DownloadFileWithDelay::Rename(const FilePath& full_path,
   DownloadFileImpl::Rename(
       full_path, overwrite_existing_file,
       base::Bind(DownloadFileWithDelay::RenameCallbackWrapper,
-                 base::Unretained(owner_), callback));
+                 owner_, callback));
 }
 
 void DownloadFileWithDelay::Detach(base::Closure callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DownloadFileImpl::Detach(
       base::Bind(DownloadFileWithDelay::DetachCallbackWrapper,
-                 base::Unretained(owner_), callback));
+                 owner_, callback));
 }
 
 // static
@@ -168,24 +181,28 @@ void DownloadFileWithDelay::DetachCallbackWrapper(
 }
 
 DownloadFileWithDelayFactory::DownloadFileWithDelayFactory()
-    : waiting_(false) {}
+    : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      waiting_(false) {}
 DownloadFileWithDelayFactory::~DownloadFileWithDelayFactory() {}
 
 DownloadFile* DownloadFileWithDelayFactory::CreateFile(
-    DownloadCreateInfo* info,
-    scoped_ptr<content::ByteStreamReader> stream,
-    DownloadManager* download_manager,
+    const content::DownloadSaveInfo& save_info,
+    const FilePath& default_download_directory,
+    GURL url,
+    GURL referrer_url,
+    int64 received_bytes,
     bool calculate_hash,
-    const net::BoundNetLog& bound_net_log) {
-
+    scoped_ptr<content::ByteStreamReader> stream,
+    const net::BoundNetLog& bound_net_log,
+    base::WeakPtr<content::DownloadDestinationObserver> observer) {
+  scoped_ptr<content::PowerSaveBlocker> psb(
+      new content::PowerSaveBlocker(
+          content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
+          "Download in progress"));
   return new DownloadFileWithDelay(
-      info, stream.Pass(), new DownloadRequestHandle(info->request_handle),
-      download_manager, calculate_hash,
-      scoped_ptr<content::PowerSaveBlocker>(
-          new content::PowerSaveBlocker(
-              content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
-              "Download in progress")).Pass(),
-      bound_net_log, this);
+      save_info, default_download_directory, url, referrer_url,
+      received_bytes, calculate_hash, stream.Pass(), bound_net_log,
+      psb.Pass(), observer, weak_ptr_factory_.GetWeakPtr());
 }
 
 void DownloadFileWithDelayFactory::AddRenameCallback(base::Closure callback) {
@@ -308,11 +325,6 @@ class DownloadContentTest : public ContentBrowserTest {
     return true;
   }
 
-  DownloadFileManager* GetDownloadFileManager() {
-    ResourceDispatcherHostImpl* rdh(ResourceDispatcherHostImpl::Get());
-    return rdh->download_file_manager();
-  }
-
  private:
   static void EnsureNoPendingDownloadJobsOnIO(bool* result) {
     if (URLRequestSlowDownloadJob::NumberOutstandingRequests())
@@ -425,7 +437,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtFinalRename) {
   // Setup new factory.
   DownloadFileWithDelayFactory* file_factory =
       new DownloadFileWithDelayFactory();
-  GetDownloadFileManager()->SetFileFactoryForTesting(
+  DownloadManagerImpl* download_manager(DownloadManagerForShell(shell()));
+  download_manager->SetDownloadFileFactoryForTesting(
       scoped_ptr<content::DownloadFileFactory>(file_factory).Pass());
 
   // Create a download
@@ -451,7 +464,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtFinalRename) {
 
   // Cancel it.
   std::vector<DownloadItem*> items;
-  DownloadManagerForShell(shell())->GetAllDownloads(&items);
+  download_manager->GetAllDownloads(&items);
   ASSERT_EQ(1u, items.size());
   items[0]->Cancel(true);
   RunAllPendingInMessageLoop();
@@ -471,10 +484,10 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtFinalRename) {
 // release.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
   // Setup new factory.
-  // Setup new factory.
   DownloadFileWithDelayFactory* file_factory =
       new DownloadFileWithDelayFactory();
-  GetDownloadFileManager()->SetFileFactoryForTesting(
+  DownloadManagerImpl* download_manager(DownloadManagerForShell(shell()));
+  download_manager->SetDownloadFileFactoryForTesting(
       scoped_ptr<content::DownloadFileFactory>(file_factory).Pass());
 
   // Create a download
@@ -502,17 +515,15 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
   callbacks[0].Run();
   callbacks.clear();
 
-  // Confirm download isn't complete yet.
+  // Confirm download still IN_PROGRESS.
   std::vector<DownloadItem*> items;
-  DownloadManagerForShell(shell())->GetAllDownloads(&items);
+  download_manager->GetAllDownloads(&items);
   EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
 
-  // Cancel the download; confirm cancel fails anyway.
+  // Cancel the download; confirm cancel fails.
   ASSERT_EQ(1u, items.size());
   items[0]->Cancel(true);
   EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
-  RunAllPendingInMessageLoop();
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
 
   // Confirm detach callback and run it.
   file_factory->WaitForSomeCallback();
@@ -522,6 +533,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
   ASSERT_EQ(1u, callbacks.size());
   callbacks[0].Run();
   callbacks.clear();
+
+  // *Now* the download should be complete.
   EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState());
 }