Simplifiy download initiation.

Now, instead of a three-way dance between DownloadResourceHandler, 
DownloadFileManager, and DownloadManager, DownloadResourceHandler tells
DownloadManager to start the download, and it calls out (with return callback)
to DownloadFileManager to create the DownloadFile.

BUG=132832
TEST=Refactor; existing tests should continue to work.
R=benjhayden@chromium.org


Review URL: https://chromiumcodereview.appspot.com/10544161

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

12 files changed, 178 insertions, 178 deletions

diff --git a/content/browser/download/download_create_info.h b/content/browser/download/download_create_info.h
index 3fb6305..0dd93df 100644
--- a/content/browser/download/download_create_info.h
+++ b/content/browser/download/download_create_info.h
@@ -13,6 +13,7 @@
 #include "base/file_path.h"
 #include "base/time.h"
 #include "content/browser/download/download_file.h"
+#include "content/browser/download/download_request_handle.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_save_info.h"
@@ -104,6 +105,9 @@ struct CONTENT_EXPORT DownloadCreateInfo {
   // UrlRequest::GetSocketAddress().
   std::string remote_address;
 
+  // The handle to the URLRequest sourcing this download.
+  DownloadRequestHandle request_handle;
+
   // The request's |BoundNetLog|, for "source_dependency" linking with the
   // download item's.
   const net::BoundNetLog request_bound_net_log;
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index f00a2ac..f7f190a 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -31,7 +31,7 @@ DownloadFileImpl::DownloadFileImpl(
     const DownloadCreateInfo* info,
     scoped_ptr<content::ByteStreamReader> stream,
     DownloadRequestHandleInterface* request_handle,
-    DownloadManager* download_manager,
+    scoped_refptr<DownloadManager> download_manager,
     bool calculate_hash,
     scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
     const net::BoundNetLog& bound_net_log)
diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h
index de88298..c00babd 100644
--- a/content/browser/download/download_file_impl.h
+++ b/content/browser/download/download_file_impl.h
@@ -33,7 +33,7 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
   DownloadFileImpl(const DownloadCreateInfo* info,
                    scoped_ptr<content::ByteStreamReader> stream,
                    DownloadRequestHandleInterface* request_handle,
-                   content::DownloadManager* download_manager,
+                   scoped_refptr<content::DownloadManager> download_manager,
                    bool calculate_hash,
                    scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
                    const net::BoundNetLog& bound_net_log);
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 21c8597..3c65250 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -41,7 +41,6 @@ class DownloadFileFactoryImpl
   virtual content::DownloadFile* CreateFile(
       DownloadCreateInfo* info,
       scoped_ptr<content::ByteStreamReader> stream,
-      const DownloadRequestHandle& request_handle,
       DownloadManager* download_manager,
       bool calculate_hash,
       const net::BoundNetLog& bound_net_log) OVERRIDE;
@@ -50,12 +49,11 @@ class DownloadFileFactoryImpl
 DownloadFile* DownloadFileFactoryImpl::CreateFile(
     DownloadCreateInfo* info,
     scoped_ptr<content::ByteStreamReader> stream,
-    const DownloadRequestHandle& request_handle,
     DownloadManager* download_manager,
     bool calculate_hash,
     const net::BoundNetLog& bound_net_log) {
   return new DownloadFileImpl(
-      info, stream.Pass(), new DownloadRequestHandle(request_handle),
+      info, stream.Pass(), new DownloadRequestHandle(info->request_handle),
       download_manager, calculate_hash,
       scoped_ptr<content::PowerSaveBlocker>(
           new content::PowerSaveBlocker(
@@ -91,41 +89,26 @@ void DownloadFileManager::OnShutdown() {
 void DownloadFileManager::CreateDownloadFile(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<content::ByteStreamReader> stream,
-    const DownloadRequestHandle& request_handle,
-    DownloadManager* download_manager, bool get_hash,
-    const net::BoundNetLog& bound_net_log) {
+    scoped_refptr<DownloadManager> download_manager, bool get_hash,
+    const net::BoundNetLog& bound_net_log,
+    const CreateDownloadFileCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(info.get());
   VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  // Create the download file.
   scoped_ptr<DownloadFile> download_file(download_file_factory_->CreateFile(
-      info.get(), stream.Pass(), request_handle, download_manager,
-      get_hash, bound_net_log));
-
-  net::Error init_result = download_file->Initialize();
-  if (net::OK != init_result) {
-    // Error:  Handle via download manager/item.
-    BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(
-            &DownloadManager::OnDownloadInterrupted,
-            download_manager,
-            info->download_id.local(),
-            0,
-            "",
-            content::ConvertNetErrorToInterruptReason(
-                init_result, content::DOWNLOAD_INTERRUPT_FROM_DISK)));
-  } else {
+      info.get(), stream.Pass(), download_manager, get_hash, bound_net_log));
+
+  content::DownloadInterruptReason interrupt_reason(
+      content::ConvertNetErrorToInterruptReason(
+          download_file->Initialize(), content::DOWNLOAD_INTERRUPT_FROM_DISK));
+  if (interrupt_reason == content::DOWNLOAD_INTERRUPT_REASON_NONE) {
     DCHECK(GetDownloadFile(info->download_id) == NULL);
     downloads_[info->download_id] = download_file.release();
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DownloadManager::StartDownload, download_manager,
-                 info->download_id.local()));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(callback, interrupt_reason));
 }
 
 DownloadFile* DownloadFileManager::GetDownloadFile(
@@ -153,32 +136,6 @@ void DownloadFileManager::UpdateInProgressDownloads() {
   }
 }
 
-DownloadId DownloadFileManager::StartDownload(
-    scoped_ptr<DownloadCreateInfo> info,
-    scoped_ptr<content::ByteStreamReader> stream,
-    const DownloadRequestHandle& request_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(info.get());
-
-  DownloadManager* manager = request_handle.GetDownloadManager();
-  DCHECK(manager);  // Checked in |DownloadResourceHandler::StartOnUIThread()|.
-
-  // |bound_net_log| will be used for logging the both the download item's and
-  // the download file's events.
-  net::BoundNetLog bound_net_log =
-      manager->CreateDownloadItem(info.get(), request_handle);
-  DownloadId download_id = info->download_id;
-  bool hash_needed = manager->GenerateFileHash();
-
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CreateDownloadFile, this,
-                 base::Passed(info.Pass()), base::Passed(stream.Pass()),
-                 request_handle,
-                 make_scoped_refptr(manager),
-                 hash_needed, bound_net_log));
-  return download_id;
-}
-
 // This method will be sent via a user action, or shutdown on the UI thread, and
 // run on the download thread. Since this message has been sent from the UI
 // thread, the download may have already completed and won't exist in our map.
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
index a769946..7a961e5 100644
--- a/content/browser/download/download_file_manager.h
+++ b/content/browser/download/download_file_manager.h
@@ -77,6 +77,11 @@ class BoundNetLog;
 class CONTENT_EXPORT DownloadFileManager
     : public base::RefCountedThreadSafe<DownloadFileManager> {
  public:
+  // Callback used with CreateDownloadFile().  |reason| will be
+  // DOWNLOAD_INTERRUPT_REASON_NONE on a successful creation.
+  typedef base::Callback<void(content::DownloadInterruptReason reason)>
+      CreateDownloadFileCallback;
+
   // Callback used with RenameInProgressDownloadFile() and
   // RenameCompletingDownloadFile().
   typedef base::Callback<void(const FilePath&)> RenameCompletionCallback;
@@ -88,7 +93,6 @@ class CONTENT_EXPORT DownloadFileManager
     virtual content::DownloadFile* CreateFile(
         DownloadCreateInfo* info,
         scoped_ptr<content::ByteStreamReader> stream,
-        const DownloadRequestHandle& request_handle,
         content::DownloadManager* download_manager,
         bool calculate_hash,
         const net::BoundNetLog& bound_net_log) = 0;
@@ -99,15 +103,17 @@ class CONTENT_EXPORT DownloadFileManager
   // |DownloadFileFactory| to be used.
   explicit DownloadFileManager(DownloadFileFactory* factory);
 
-  // Called on shutdown on the UI thread.
-  virtual void Shutdown();
-
-  // Called on UI thread to make DownloadFileManager start the download. Creates
-  // and returns the DownloadId of the download.
-  virtual content::DownloadId StartDownload(
+  // Create a download file and record it in the download file manager.
+  virtual void CreateDownloadFile(
       scoped_ptr<DownloadCreateInfo> info,
       scoped_ptr<content::ByteStreamReader> stream,
-      const DownloadRequestHandle& request_handle);
+      scoped_refptr<content::DownloadManager> download_manager,
+      bool hash_needed,
+      const net::BoundNetLog& bound_net_log,
+      const CreateDownloadFileCallback& callback);
+
+  // Called on shutdown on the UI thread.
+  virtual void Shutdown();
 
   // Handlers for notifications sent from the UI thread and run on the
   // FILE thread.  These are both terminal actions with respect to the
@@ -175,15 +181,6 @@ class CONTENT_EXPORT DownloadFileManager
   // Clean up helper that runs on the download thread.
   void OnShutdown();
 
-  // Creates DownloadFile on FILE thread and continues starting the download
-  // process.
-  void CreateDownloadFile(scoped_ptr<DownloadCreateInfo> info,
-                          scoped_ptr<content::ByteStreamReader> stream,
-                          const DownloadRequestHandle& request_handle,
-                          content::DownloadManager* download_manager,
-                          bool hash_needed,
-                          const net::BoundNetLog& bound_net_log);
-
   // Called only on the download thread.
   content::DownloadFile* GetDownloadFile(content::DownloadId global_id);
 
diff --git a/content/browser/download/download_file_manager_unittest.cc b/content/browser/download/download_file_manager_unittest.cc
index 0845bc6..c25a722 100644
--- a/content/browser/download/download_file_manager_unittest.cc
+++ b/content/browser/download/download_file_manager_unittest.cc
@@ -54,7 +54,6 @@ class MockDownloadFileFactory :
   virtual content::DownloadFile* CreateFile(
       DownloadCreateInfo* info,
       scoped_ptr<content::ByteStreamReader> stream,
-      const DownloadRequestHandle& request_handle,
       content::DownloadManager* download_manager,
       bool calculate_hash,
       const net::BoundNetLog& bound_net_log) OVERRIDE;
@@ -68,7 +67,6 @@ class MockDownloadFileFactory :
 content::DownloadFile* MockDownloadFileFactory::CreateFile(
     DownloadCreateInfo* info,
     scoped_ptr<content::ByteStreamReader> stream,
-    const DownloadRequestHandle& request_handle,
     content::DownloadManager* download_manager,
     bool calculate_hash,
     const net::BoundNetLog& bound_net_log) {
@@ -138,8 +136,9 @@ class DownloadFileManagerTest : public testing::Test {
   // calling Release() on |download_manager_| won't ever result in its
   // destructor being called and we get a leak.
   DownloadFileManagerTest()
-    : ui_thread_(BrowserThread::UI, &loop_),
-      file_thread_(BrowserThread::FILE, &loop_) {
+      : last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
+        ui_thread_(BrowserThread::UI, &loop_),
+        file_thread_(BrowserThread::FILE, &loop_) {
   }
 
   ~DownloadFileManagerTest() {
@@ -173,39 +172,27 @@ class DownloadFileManagerTest : public testing::Test {
     Mock::VerifyAndClearExpectations(download_manager_);
   }
 
-  // Start a download.
+  void OnDownloadFileCreated(content::DownloadInterruptReason reason) {
+    last_reason_ = reason;
+  }
+
+  // Create a download item on the DFM.
   // |info| is the information needed to create a new download file.
   // |id| is the download ID of the new download file.
-  void StartDownload(scoped_ptr<DownloadCreateInfo> info,
-                     const DownloadId& id) {
-    // Expected call sequence:
-    //  StartDownload
-    //    DownloadManager::CreateDownloadItem
-    //    DownloadManagerDelegate::GenerateFileHash
-    //    Process one message in the message loop
-    //      CreateDownloadFile
-    //        new MockDownloadFile, add to downloads_[id]
-    //        DownloadRequestHandle::ResumeRequest
-    //        StartUpdateTimer
-    //        DownloadCreateInfo is destroyed
-    //    Process one message in the message loop
-    //        DownloadManager::StartDownload
-    info->download_id = id;
-
-    // Set expectations and return values.
-    EXPECT_CALL(*download_manager_, CreateDownloadItem(info.get(), _))
-        .Times(1)
-        .WillOnce(Return(net::BoundNetLog()));
-    EXPECT_CALL(*download_manager_, GenerateFileHash())
-        .Times(AtLeast(1))
-        .WillRepeatedly(Return(false));
-    EXPECT_CALL(*download_manager_, StartDownload(id.local()));
-
-    download_file_manager_->StartDownload(
-        info.Pass(), scoped_ptr<content::ByteStreamReader>(), *request_handle_);
+  void CreateDownloadFile(scoped_ptr<DownloadCreateInfo> info) {
+    // Mostly null out args; they'll be passed to MockDownloadFileFactory
+    // to be ignored anyway.
+    download_file_manager_->CreateDownloadFile(
+        info.Pass(), scoped_ptr<content::ByteStreamReader>(),
+        download_manager_, true, net::BoundNetLog(),
+        base::Bind(&DownloadFileManagerTest::OnDownloadFileCreated,
+                   // The test jig will outlive all download files.
+                   base::Unretained(this)));
+
+    // Anything that isn't DOWNLOAD_INTERRUPT_REASON_NONE.
+    last_reason_ = content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
     ProcessAllPendingMessages();
-
-    ClearExpectations(id);
+    EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, last_reason_);
   }
 
   // Renames the download file.
@@ -301,6 +288,9 @@ class DownloadFileManagerTest : public testing::Test {
   MockDownloadFileFactory* download_file_factory_;
   scoped_refptr<DownloadFileManager> download_file_manager_;
 
+  // Error from creating download file.
+  content::DownloadInterruptReason last_reason_;
+
   // Per-download statistics.
   std::map<DownloadId, int64> byte_count_;
   std::map<DownloadId, int> error_count_;
@@ -328,33 +318,24 @@ const int32 DownloadFileManagerTest::kDummyDownloadId2 = 77;
 const int DownloadFileManagerTest::kDummyChildId = 3;
 const int DownloadFileManagerTest::kDummyRequestId = 67;
 
-TEST_F(DownloadFileManagerTest, CancelAtStart) {
+TEST_F(DownloadFileManagerTest, Cancel) {
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
 
-  StartDownload(info.Pass(), dummy_id);
-
-  CleanUp(dummy_id);
-}
-
-TEST_F(DownloadFileManagerTest, CancelBeforeFinished) {
-  // Same as StartDownload, at first.
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
-
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   CleanUp(dummy_id);
 }
 
 TEST_F(DownloadFileManagerTest, RenameInProgress) {
-  // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
   RenameFile(dummy_id, foo, foo, net::OK, IN_PROGRESS, OVERWRITE);
@@ -363,13 +344,13 @@ TEST_F(DownloadFileManagerTest, RenameInProgress) {
 }
 
 TEST_F(DownloadFileManagerTest, RenameInProgressWithUniquification) {
-  // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
   FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)")));
@@ -380,13 +361,13 @@ TEST_F(DownloadFileManagerTest, RenameInProgressWithUniquification) {
 }
 
 TEST_F(DownloadFileManagerTest, RenameInProgressWithError) {
-  // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
   RenameFile(dummy_id, foo, foo, net::ERR_FILE_PATH_TOO_LONG,
@@ -396,13 +377,13 @@ TEST_F(DownloadFileManagerTest, RenameInProgressWithError) {
 }
 
 TEST_F(DownloadFileManagerTest, RenameWithUniquification) {
-  // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
   FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)")));
@@ -416,13 +397,13 @@ TEST_F(DownloadFileManagerTest, RenameWithUniquification) {
 }
 
 TEST_F(DownloadFileManagerTest, RenameTwice) {
-  // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
+  CreateDownloadFile(info.Pass());
 
   FilePath crfoo(download_dir.path().Append(
       FILE_PATH_LITERAL("foo.txt.crdownload")));
@@ -437,15 +418,16 @@ TEST_F(DownloadFileManagerTest, RenameTwice) {
 TEST_F(DownloadFileManagerTest, TwoDownloads) {
   // Same as StartDownload, at first.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  scoped_ptr<DownloadCreateInfo> info2(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+  info->download_id = dummy_id;
+  scoped_ptr<DownloadCreateInfo> info2(new DownloadCreateInfo);
   DownloadId dummy_id2(download_manager_.get(), kDummyDownloadId2);
+  info2->download_id = dummy_id2;
   ScopedTempDir download_dir;
   ASSERT_TRUE(download_dir.CreateUniqueTempDir());
 
-  StartDownload(info.Pass(), dummy_id);
-
-  StartDownload(info2.Pass(), dummy_id2);
+  CreateDownloadFile(info.Pass());
+  CreateDownloadFile(info2.Pass());
 
   FilePath crbar(download_dir.path().Append(
       FILE_PATH_LITERAL("bar.txt.crdownload")));
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index 5c52f4c..42c1761 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -62,12 +62,11 @@ class MockDownloadFileFactory
   content::DownloadFile* CreateFile(
       DownloadCreateInfo* info,
       scoped_ptr<content::ByteStreamReader> stream_reader,
-      const DownloadRequestHandle& request_handle,
       DownloadManager* mgr,
       bool calculate_hash,
       const net::BoundNetLog& bound_net_log) {
     return MockCreateFile(
-        info, stream_reader.get(), request_handle, mgr, calculate_hash,
+        info, stream_reader.get(), info->request_handle, mgr, calculate_hash,
         bound_net_log);
   }
 
@@ -84,16 +83,6 @@ class MockDownloadFileManager : public DownloadFileManager {
  public:
   MockDownloadFileManager();
   MOCK_METHOD0(Shutdown, void());
-  MOCK_METHOD3(MockStartDownload,
-               void(DownloadCreateInfo*, content::ByteStreamReader*,
-                    const DownloadRequestHandle&));
-  virtual DownloadId StartDownload(scoped_ptr<DownloadCreateInfo> info,
-                            scoped_ptr<content::ByteStreamReader> stream,
-                            const DownloadRequestHandle& request_handle) {
-    MockStartDownload(info.release(), stream.release(), request_handle);
-    return DownloadId();
-  }
-
   MOCK_METHOD1(CancelDownload, void(DownloadId));
   MOCK_METHOD1(CompleteDownload, void(DownloadId));
   MOCK_METHOD1(OnDownloadManagerShutdown, void(DownloadManager*));
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 5e8c80f..a3d556e 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -17,6 +17,7 @@
 #include "base/synchronization/lock.h"
 #include "base/sys_string_conversions.h"
 #include "build/build_config.h"
+#include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item_impl.h"
@@ -367,9 +368,48 @@ bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
 }
 
 // We have received a message from DownloadFileManager about a new download.
-void DownloadManagerImpl::StartDownload(int32 download_id) {
+content::DownloadId DownloadManagerImpl::StartDownload(
+    scoped_ptr<DownloadCreateInfo> info,
+    scoped_ptr<content::ByteStreamReader> stream) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // |bound_net_log| will be used for logging both the download item's and
+  // the download file's events.
+  net::BoundNetLog bound_net_log = CreateDownloadItem(info.get());
+
+  // If info->download_id was unknown on entry to this function, it was
+  // assigned in CreateDownloadItem.
+  DownloadId download_id = info->download_id;
+
+  DownloadFileManager::CreateDownloadFileCallback callback(
+      base::Bind(&DownloadManagerImpl::OnDownloadFileCreated,
+                 this, download_id.local()));
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CreateDownloadFile,
+                 file_manager_, base::Passed(info.Pass()),
+                 base::Passed(stream.Pass()),
+                 make_scoped_refptr(this),
+                 GenerateFileHash(), bound_net_log,
+                 callback));
+
+  return download_id;
+}
+
+void DownloadManagerImpl::OnDownloadFileCreated(
+    int32 download_id, content::DownloadInterruptReason reason) {
+  if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
+    OnDownloadInterrupted(download_id, 0, "", reason);
+    // TODO(rdsmith): It makes no sense to continue along the
+    // regular download path after we've gotten an error.  But it's
+    // the way the code has historically worked, and this allows us
+    // to get the download persisted and observers of the download manager
+    // notified, so tests work.  When we execute all side effects of cancel
+    // (including queue removal) immedately rather than waiting for
+    // persistence we should replace this comment with a "return;".
+  }
+
   if (!delegate_ || delegate_->ShouldStartDownload(download_id))
     RestartDownload(download_id);
 }
@@ -450,7 +490,7 @@ FilePath DownloadManagerImpl::LastDownloadPath() {
 }
 
 net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
-    DownloadCreateInfo* info, const DownloadRequestHandle& request_handle) {
+    DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   net::BoundNetLog bound_net_log =
@@ -458,8 +498,9 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
   if (!info->download_id.IsValid())
     info->download_id = GetNextId();
   DownloadItem* download = factory_->CreateActiveItem(
-      this, *info, scoped_ptr<DownloadRequestHandleInterface>(
-          new DownloadRequestHandle(request_handle)).Pass(),
+      this, *info,
+      scoped_ptr<DownloadRequestHandleInterface>(
+          new DownloadRequestHandle(info->request_handle)).Pass(),
       browser_context_->IsOffTheRecord(), bound_net_log);
   int32 download_id = info->download_id.local();
 
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 0f6670a..924fa8b 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -47,7 +47,9 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void SearchDownloads(const string16& query,
                                DownloadVector* result) OVERRIDE;
   virtual bool Init(content::BrowserContext* browser_context) OVERRIDE;
-  virtual void StartDownload(int32 id) OVERRIDE;
+  virtual content::DownloadId StartDownload(
+      scoped_ptr<DownloadCreateInfo> info,
+      scoped_ptr<content::ByteStreamReader> stream) OVERRIDE;
   virtual void UpdateDownload(int32 download_id,
                               int64 bytes_so_far,
                               int64 bytes_per_sec,
@@ -76,8 +78,7 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual content::BrowserContext* GetBrowserContext() const OVERRIDE;
   virtual FilePath LastDownloadPath() OVERRIDE;
   virtual net::BoundNetLog CreateDownloadItem(
-      DownloadCreateInfo* info,
-      const DownloadRequestHandle& request_handle) OVERRIDE;
+      DownloadCreateInfo* info) OVERRIDE;
   virtual content::DownloadItem* CreateSavePackageDownloadItem(
       const FilePath& main_file_path,
       const GURL& page_url,
@@ -179,6 +180,12 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Remove from internal maps.
   int RemoveDownloadItems(const DownloadVector& pending_deletes);
 
+  // Called in response to our request to the DownloadFileManager to
+  // create a DownloadFile.  A |reason| of
+  // content::DOWNLOAD_INTERRUPT_REASON_NONE indicates success.
+  void OnDownloadFileCreated(
+      int32 download_id, content::DownloadInterruptReason reason);
+
   // Called when a download entry is committed to the persistent store.
   void OnDownloadItemAddedToPersistentStore(int32 download_id, int64 db_handle);
 
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index 761d301..58dfd8b 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -89,19 +89,30 @@ class MockDownloadFileManager : public DownloadFileManager {
  public:
   MockDownloadFileManager();
 
-  MOCK_METHOD0(Shutdown, void());
-  MOCK_METHOD3(MockStartDownload,
-               content::DownloadId(DownloadCreateInfo*,
-                                   content::ByteStreamReader* stream,
-                                   const DownloadRequestHandle&));
-
-  virtual content::DownloadId StartDownload(
+  void CreateDownloadFile(
       scoped_ptr<DownloadCreateInfo> info,
       scoped_ptr<content::ByteStreamReader> stream,
-      const DownloadRequestHandle& request_handle) OVERRIDE {
-    return MockStartDownload(info.get(), stream.get(), request_handle);
+      scoped_refptr<content::DownloadManager> download_manager,
+      bool hash_needed,
+      const net::BoundNetLog& bound_net_log,
+      const CreateDownloadFileCallback& callback) OVERRIDE {
+    // Note that scoped_refptr<> on download manager is also stripped
+    // to make mock comparisons easier.  Comparing the scoped_refptr<>
+    // works, but holds a reference to the DownloadManager until
+    // MockDownloadFileManager destruction, which messes up destruction
+    // testing.
+    MockCreateDownloadFile(info.get(), stream.get(), download_manager.get(),
+                           hash_needed, bound_net_log, callback);
   }
 
+  MOCK_METHOD6(MockCreateDownloadFile, void(
+      DownloadCreateInfo* info,
+      content::ByteStreamReader* stream,
+      content::DownloadManager* download_manager,
+      bool hash_needed,
+      const net::BoundNetLog& bound_net_log,
+      const CreateDownloadFileCallback& callback));
+  MOCK_METHOD0(Shutdown, void());
   MOCK_METHOD1(CancelDownload, void(content::DownloadId));
   MOCK_METHOD1(CompleteDownload, void(content::DownloadId));
   MOCK_METHOD1(OnDownloadManagerShutdown, void(content::DownloadManager*));
@@ -340,7 +351,6 @@ class DownloadManagerTest : public testing::Test {
   // Returns download id.
   content::MockDownloadItem& AddItemToManager() {
     DownloadCreateInfo info;
-    DownloadRequestHandle handle;
 
     static const char* kDownloadIdDomain = "Test download id domain";
 
@@ -349,7 +359,8 @@ class DownloadManagerTest : public testing::Test {
     int id = next_download_id_;
     ++next_download_id_;
     info.download_id = content::DownloadId(kDownloadIdDomain, id);
-    download_manager_->CreateDownloadItem(&info, handle);
+    info.request_handle = DownloadRequestHandle();
+    download_manager_->CreateDownloadItem(&info);
 
     DCHECK(mock_download_item_factory_->GetItem(id));
     content::MockDownloadItem& item(*mock_download_item_factory_->GetItem(id));
@@ -430,6 +441,26 @@ class DownloadManagerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
 };
 
+// Confirm the appropriate invocations occur when you start a download.
+TEST_F(DownloadManagerTest, StartDownload) {
+  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+  scoped_ptr<content::ByteStreamReader> stream;
+  int32 local_id(5);                    // Random value
+
+  EXPECT_FALSE(GetActiveDownloadItem(local_id));
+
+  EXPECT_CALL(GetMockDownloadManagerDelegate(), GetNextId())
+      .WillOnce(Return(content::DownloadId(this, local_id)));
+  EXPECT_CALL(GetMockDownloadManagerDelegate(), GenerateFileHash())
+      .WillOnce(Return(true));
+  EXPECT_CALL(GetMockDownloadFileManager(), MockCreateDownloadFile(
+      info.get(), static_cast<content::ByteStreamReader*>(NULL),
+      download_manager_.get(), true, _, _));
+
+  download_manager_->StartDownload(info.Pass(), stream.Pass());
+  EXPECT_TRUE(GetActiveDownloadItem(local_id));
+}
+
 // Does the DownloadManager prompt when requested?
 TEST_F(DownloadManagerTest, RestartDownload) {
   // Put a mock we have a handle to on the download manager.
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index dc36e93..d83fb28 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -58,12 +58,10 @@ void CallStartedCBOnUIThread(
 static void StartOnUIThread(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<content::ByteStreamReader> stream,
-    scoped_refptr<DownloadFileManager> download_file_manager,
-    const DownloadRequestHandle& handle,
     const DownloadResourceHandler::OnStartedCallback& started_cb) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DownloadManager* download_manager = handle.GetDownloadManager();
+  DownloadManager* download_manager = info->request_handle.GetDownloadManager();
   if (!download_manager) {
     // NULL in unittests or if the page closed right after starting the
     // download.
@@ -73,7 +71,7 @@ static void StartOnUIThread(
   }
 
   DownloadId download_id =
-      download_file_manager->StartDownload(info.Pass(), stream.Pass(), handle);
+      download_manager->StartDownload(info.Pass(), stream.Pass());
 
   if (!started_cb.is_null())
     started_cb.Run(download_id, net::OK);
@@ -86,14 +84,12 @@ DownloadResourceHandler::DownloadResourceHandler(
     int render_view_id,
     int request_id,
     const GURL& url,
-    scoped_refptr<DownloadFileManager> download_file_manager,
     net::URLRequest* request,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     const content::DownloadSaveInfo& save_info)
     : global_id_(render_process_host_id, request_id),
       render_view_id_(render_view_id),
       content_length_(0),
-      download_file_manager_(download_file_manager),
       request_(request),
       started_cb_(started_cb),
       save_info_(save_info),
@@ -173,8 +169,9 @@ bool DownloadResourceHandler::OnResponseStarted(
   info->remote_address = request_->GetSocketAddress().host();
   download_stats::RecordDownloadMimeType(info->mime_type);
 
-  DownloadRequestHandle request_handle(AsWeakPtr(), global_id_.child_id,
-                                       render_view_id_, global_id_.request_id);
+  info->request_handle =
+      DownloadRequestHandle(AsWeakPtr(), global_id_.child_id,
+                            render_view_id_, global_id_.request_id);
 
   // Get the last modified time and etag.
   const net::HttpResponseHeaders* headers = request_->response_headers();
@@ -210,8 +207,6 @@ bool DownloadResourceHandler::OnResponseStarted(
       base::Bind(&StartOnUIThread,
                  base::Passed(info.Pass()),
                  base::Passed(stream_reader.Pass()),
-                 download_file_manager_,
-                 request_handle,
                  // Pass to StartOnUIThread so that variable
                  // access is always on IO thread but function
                  // is called on UI thread.
diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h
index 7f99316..0f6efb3 100644
--- a/content/browser/download/download_resource_handler.h
+++ b/content/browser/download/download_resource_handler.h
@@ -19,7 +19,6 @@
 #include "content/public/browser/global_request_id.h"
 #include "net/base/net_errors.h"
 
-class DownloadFileManager;
 class DownloadRequestHandle;
 struct DownloadCreateInfo;
 
@@ -45,7 +44,6 @@ class DownloadResourceHandler
       int render_view_id,
       int request_id,
       const GURL& url,
-      scoped_refptr<DownloadFileManager> download_file_manager,
       net::URLRequest* request,
       const OnStartedCallback& started_cb,
       const content::DownloadSaveInfo& save_info);
@@ -109,7 +107,6 @@ class DownloadResourceHandler
   int render_view_id_;
   std::string content_disposition_;
   int64 content_length_;
-  scoped_refptr<DownloadFileManager> download_file_manager_;
   net::URLRequest* request_;
   // This is read only on the IO thread, but may only
   // be called on the UI thread.