Replace the DownloadFileManager with direct ownership

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 7570379eae814d624bfda65aab7759e8e965baef..717c76c8080601b95ece63665c893d10f6bace5a 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -20,7 +20,7 @@
 #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_file_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ -143,23 +143,13 @@ class MapValueIteratorAdapter {
   // Allow copy and assign.
 };
 
-void EnsureNoPendingDownloadsOnFile(scoped_refptr<DownloadFileManager> dfm,
-                                    bool* result) {
-  if (dfm->NumberOfActiveDownloads())
-    *result = false;
+void EnsureNoPendingDownloadJobsOnFile(bool* result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  *result = (content::DownloadFile::GetNumberOfDownloadFiles() == 0);
   BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE, MessageLoop::QuitClosure());
 }
 
-void EnsureNoPendingDownloadJobsOnIO(bool* result) {
-  scoped_refptr<DownloadFileManager> download_file_manager =
-      ResourceDispatcherHostImpl::Get()->download_file_manager();
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&EnsureNoPendingDownloadsOnFile,
-                 download_file_manager, result));
-}
-
 class DownloadItemFactoryImpl : public content::DownloadItemFactory {
  public:
     DownloadItemFactoryImpl() {}
@@ -204,8 +194,8 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   bool result = true;
   BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result));
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&EnsureNoPendingDownloadJobsOnFile, &result));
   MessageLoop::current()->Run();
   return result;
 }
@@ -213,19 +203,17 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
 }  // namespace content
 
 DownloadManagerImpl::DownloadManagerImpl(
-    DownloadFileManager* file_manager,
     scoped_ptr<content::DownloadItemFactory> factory,
     net::NetLog* net_log)
     : factory_(factory.Pass()),
       history_size_(0),
       shutdown_needed_(false),
       browser_context_(NULL),
-      file_manager_(file_manager),
       delegate_(NULL),
       net_log_(net_log) {
-  DCHECK(file_manager);
   if (!factory_.get())
     factory_.reset(new DownloadItemFactoryImpl());
+  file_factory_.reset(new content::DownloadFileFactory());
 }
 
 DownloadManagerImpl::~DownloadManagerImpl() {
@@ -244,6 +232,17 @@ DownloadId DownloadManagerImpl::GetNextId() {
   return id;
 }
 
+content::DownloadFileFactory* DownloadManagerImpl::GetDownloadFileFactory() {
+  return file_factory_.get();
+}
+
+void DownloadManagerImpl::DelegateStart(DownloadItemImpl* item) {
+  int32 download_id = item->GetId();
+
+  if (!delegate_ || delegate_->ShouldStartDownload(download_id))
+    RestartDownload(download_id);
+}
+
 bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) {
   if (!delegate_)
     return true;
@@ -277,11 +276,7 @@ void DownloadManagerImpl::Shutdown() {
   FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
   // TODO(benjhayden): Consider clearing observers_.
 
-  DCHECK(file_manager_);
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
-                 file_manager_, make_scoped_refptr(this)));
+  // The DownloadFiles will be canceled and deleted by their DownloadItems.
 
   AssertContainersConsistent();
 
@@ -328,7 +323,6 @@ void DownloadManagerImpl::Shutdown() {
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
 
-  file_manager_ = NULL;
   if (delegate_)
     delegate_->Shutdown();
 }
@@ -395,51 +389,24 @@ bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
   return true;
 }
 
-// We have received a message from DownloadFileManager about a new download.
 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, 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;".
-  }
+  net::BoundNetLog bound_net_log =
+      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
+  DownloadItemImpl* download =
+      CreateDownloadItem(info.get(), bound_net_log);

[Randy Smith (Not in Mondays), 2012/08/02 20:41:23]

Add comment about why create download item first (want complexity of two states
(have/not-have DF) in DI since DI is higher level and already needs those two
states).

[Randy Smith (Not in Mondays), 2012/08/03 17:32:44]

Done.

+  scoped_ptr<content::DownloadFile> download_file(
+      GetDownloadFileFactory()->CreateFile(
+          info->save_info, info->url(), info->referrer_url,
+          info->received_bytes, GenerateFileHash(),
+          stream.Pass(), bound_net_log,
+          download->DestinationObserverAsWeakPtr()));
+  download->Start(download_file.Pass());
 
-  if (!delegate_ || delegate_->ShouldStartDownload(download_id))
-    RestartDownload(download_id);
+  return download->GetGlobalId();
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
@@ -516,12 +483,10 @@ FilePath DownloadManagerImpl::LastDownloadPath() {
   return last_download_path_;
 }
 
-net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
-    DownloadCreateInfo* info) {
+DownloadItemImpl* DownloadManagerImpl::CreateDownloadItem(
+    DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  net::BoundNetLog bound_net_log =
-      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
   if (!info->download_id.IsValid())
     info->download_id = GetNextId();
   DownloadItemImpl* download = factory_->CreateActiveItem(
@@ -535,7 +500,7 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
   DCHECK(!ContainsKey(active_downloads_, download->GetId()));
   active_downloads_[download->GetId()] = download;
 
-  return bound_net_log;
+  return download;
 }
 
 DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
@@ -597,41 +562,7 @@ void DownloadManagerImpl::OnTargetPathAvailable(DownloadItemImpl* download) {
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() ==
          download->GetTargetFilePath().DirName());
-  download->OnIntermediatePathDetermined(file_manager_, intermediate_path);
-}
-
-void DownloadManagerImpl::UpdateDownload(int32 download_id,
-                                         int64 bytes_so_far,
-                                         int64 bytes_per_sec,
-                                         const std::string& hash_state) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = active_downloads_.find(download_id);
-  if (it != active_downloads_.end()) {
-    DownloadItemImpl* download = it->second;
-    if (download->IsInProgress()) {
-      download->UpdateProgress(bytes_so_far, bytes_per_sec, hash_state);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
-    }
-  }
-}
-
-void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
-                                              int64 size,
-                                              const std::string& hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
-           << " size = " << size;
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // If it's not in active_downloads_, that means it was cancelled; just
-  // ignore the notification.
-  if (active_downloads_.count(download_id) == 0)
-    return;
-
-  DownloadItemImpl* download = active_downloads_[download_id];
-  download->OnAllDataSaved(size, hash);
-  MaybeCompleteDownload(download);
+  download->OnIntermediatePathDetermined(intermediate_path);
 }
 
 void DownloadManagerImpl::AssertStateConsistent(
@@ -728,7 +659,7 @@ void DownloadManagerImpl::MaybeCompleteDownload(
 
   if (delegate_)
     delegate_->UpdateItemInPersistentStore(download);
-  download->OnDownloadCompleting(file_manager_);
+  download->OnDownloadCompleting();
 }
 
 void DownloadManagerImpl::MaybeCompleteDownloadById(int download_id) {
@@ -763,18 +694,7 @@ void DownloadManagerImpl::DownloadStopped(DownloadItemImpl* download) {
   // should already have been updated.
   AssertStateConsistent(download);
 
-  DCHECK(file_manager_);
-  download->OffThreadCancel(file_manager_);
-}
-
-void DownloadManagerImpl::OnDownloadInterrupted(
-    int32 download_id,
-    content::DownloadInterruptReason reason) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (!ContainsKey(active_downloads_, download_id))
-    return;
-  active_downloads_[download_id]->Interrupt(reason);
+  download->OffThreadCancel();
 }
 
 void DownloadManagerImpl::RemoveFromActiveList(DownloadItemImpl* download) {
@@ -794,6 +714,11 @@ bool DownloadManagerImpl::GenerateFileHash() {
   return delegate_ && delegate_->GenerateFileHash();
 }
 
+void DownloadManagerImpl::SetDownloadFileFactoryForTesting(
+    scoped_ptr<content::DownloadFileFactory> file_factory) {
+  file_factory_ = file_factory.Pass();
+}
+
 int DownloadManagerImpl::RemoveDownloadItems(
     const DownloadItemImplVector& pending_deletes) {
   if (pending_deletes.empty())
@@ -1160,8 +1085,7 @@ void DownloadManagerImpl::DownloadOpened(DownloadItemImpl* download) {
 void DownloadManagerImpl::DownloadRenamedToIntermediateName(
     DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // If the rename failed, we receive an OnDownloadInterrupted() call before we
-  // receive the DownloadRenamedToIntermediateName() call.
+  // If the rename failed, we processed an interrupt before we get here.
   if (delegate_)
     delegate_->AddItemToPersistentStore(download);
 }
@@ -1169,8 +1093,7 @@ void DownloadManagerImpl::DownloadRenamedToIntermediateName(
 void DownloadManagerImpl::DownloadRenamedToFinalName(
     DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // If the rename failed, we receive an OnDownloadInterrupted() call before we
-  // receive the DownloadRenamedToFinalName() call.
+  // If the rename failed, we processed an interrupt before we get here.
   if (delegate_) {
     delegate_->UpdatePathForItemInPersistentStore(
         download, download->GetFullPath());