Replace the DownloadFileManager with direct ownership

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index a3e72ffa469e50f0efd18f92c63a50c2a99e0e16..c8a189baf76fc8a34518dc68d2d092098c9e8b47 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -19,7 +19,6 @@
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
-#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
@@ -121,6 +120,17 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
   }
 };
 
+// Detach the specified download file, then invoke the callback on the
+// UI thread.  Note that this will also delete the DownloadFile object,
+// as the function accepts ownership and does not transfer it on.
+static void ReleaseDownloadFile(scoped_ptr<DownloadFile> download_file,
+                                const base::Closure& ui_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Detach();
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, ui_callback);
+}
+
 }  // namespace
 
 namespace content {
@@ -300,6 +310,10 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Should always have been nuked before now, at worst in
+  // DownloadManager shutdown.
+  DCHECK(!download_file_.get());
+
   TransitionTo(REMOVING);
   STLDeleteContainerPairSecondPointers(
       external_data_map_.begin(), external_data_map_.end());
@@ -307,6 +321,12 @@ DownloadItemImpl::~DownloadItemImpl() {
   delegate_->Detach();
 }
 
+base::WeakPtr<content::DownloadDestinationController>
+DownloadItemImpl::DestinationControllerAsWeakPtr() {
+  // Return does private downcast.
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 void DownloadItemImpl::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -395,21 +415,6 @@ void DownloadItemImpl::DangerousDownloadValidated() {
   delegate_->MaybeCompleteDownload(this);
 }
 
-void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
-                                        const std::string& final_hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  hash_ = final_hash;
-  hash_state_ = "";
-
-  received_bytes_ = bytes_so_far;
-
-  // If we've received more data than we were expecting (bad server info?),
-  // revert to 'unknown size mode'.
-  if (received_bytes_ > total_bytes_)
-    total_bytes_ = 0;
-}
-
 // Updates from the download thread may have been posted while this download
 // was being cancelled in the UI thread, so we'll accept them unless we're
 // complete.
@@ -470,6 +475,20 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
     delegate_->DownloadStopped(this);
 }
 
+// We're starting the download.
+void DownloadItemImpl::Start(scoped_ptr<content::DownloadFile> download_file) {
+  DCHECK(!download_file_.get());
+  download_file_ = download_file.Pass();
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFile::Initialize,
+                 // Safe because we control download file lifetime.
+                 base::Unretained(download_file_.get()),
+                 base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
+                            weak_ptr_factory_.GetWeakPtr())));
+}
+
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // Somewhat counter-intuitively, it is possible for us to receive an
@@ -506,12 +525,16 @@ void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
 }
 
 void DownloadItemImpl::OnAllDataSaved(
-    int64 size, const std::string& final_hash) {
+    const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
-  ProgressComplete(size, final_hash);
+
+  // Store final hash and null out intermediate serialized hash state.
+  hash_ = final_hash;
+  hash_state_ = "";
+
   UpdateObservers();
 }
 
@@ -700,43 +723,47 @@ void DownloadItemImpl::TogglePause() {
   UpdateObservers();
 }
 
-void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
+void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Lost Cancel race.
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetName().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
-  DCHECK(file_manager);
 
   if (NeedsRename()) {
-    DownloadFileManager::RenameCompletionCallback callback =
+    content::DownloadFile::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   base::Unretained(file_manager));
+                   weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::RenameDownloadFile,
-                   file_manager, GetGlobalId(), GetTargetFilePath(),
-                   true, callback));
+        base::Bind(&DownloadFile::Rename,
+                   base::Unretained(download_file_.get()),
+                   GetTargetFilePath(), true, callback));
   } else {
     // Complete the download and release the DownloadFile.
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::CompleteDownload,
-                   file_manager, GetGlobalId(),
+        base::Bind(&ReleaseDownloadFile, base::Passed(download_file_.Pass()),
                    base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                               weak_ptr_factory_.GetWeakPtr())));
   }
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
-    DownloadFileManager* file_manager,
     content::DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Lost Cancel race.
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
@@ -755,14 +782,30 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   delegate_->DownloadRenamedToFinalName(this);
 
   // Complete the download and release the DownloadFile.
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CompleteDownload,
-                 file_manager, GetGlobalId(),
+      base::Bind(&ReleaseDownloadFile, base::Passed(download_file_.Pass()),
                  base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                             weak_ptr_factory_.GetWeakPtr())));
 }
 
+void DownloadItemImpl::OnDownloadFileInitialized(
+    content::DownloadInterruptReason result) {
+  if (result != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
+    Interrupt(result);
+    // 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;".
+  }
+
+  delegate_->DelegateStart(this);
+}
+
 void DownloadItemImpl::OnDownloadFileReleased() {
   if (delegate_->ShouldOpenDownload(this))
     Completed();
@@ -893,16 +936,31 @@ void DownloadItemImpl::OnContentCheckCompleted(
 }
 
 void DownloadItemImpl::OnIntermediatePathDetermined(
-    DownloadFileManager* file_manager,
     const FilePath& intermediate_path) {
-  DownloadFileManager::RenameCompletionCallback callback =
+  if (!IsInProgress()) {
+    // Lost interrupt/cancel race.  We need to continue the cascade
+    // anyway, so that we get this entry persisted and made visible
+    // to observers.  Actual error code doesn't matter as we've
+    // already stored the original reason for failure.
+    //
+    // TODO(rdsmith): Remove this code when we make downloads visible to
+    // observers earlier in the cascade so they can see immediate transitions
+    // to INTERRUPTED.
+    OnDownloadRenamedToIntermediateName(
+        content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED, FilePath());
+    return;
+  }
+
+  content::DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
+
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::RenameDownloadFile,
-                 file_manager, GetGlobalId(), intermediate_path,
-                 false, callback));
+      base::Bind(&DownloadFile::Rename,
+                 base::Unretained(download_file_.get()),
+                 intermediate_path, false, callback));
 }
 
 const FilePath& DownloadItemImpl::GetFullPath() const {
@@ -924,14 +982,62 @@ FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
       GetTargetFilePath() : GetFullPath();
 }
 
-void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
+void DownloadItemImpl::OffThreadCancel() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle_->CancelRequest();
 
+  DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CancelDownload,
-                 file_manager, download_id_));
+      // Will be deleted at end of task execution.
+      base::Bind(&DownloadFile::Cancel, base::Owned(download_file_.release())));
+}
+
+void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
+                                         int64 bytes_per_sec,
+                                         const std::string& hash_state) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!IsInProgress()) {
+    // Ignore if we're no longer in-progress.  This can happen if we race a
+    // Cancel on the UI thread with an update on the FILE thread.
+    //
+    // TODO(rdsmith): Arguably we should let this go through, as this means
+    // the download really did get further than we know before it was
+    // cancelled.  But the gain isn't very large, and the code is more
+    // fragile if it has to support in progress updates in a non-in-progress
+    // state.  This issue should be readdressed when we revamp performance
+    // reporting.
+    return;
+  }
+  bytes_per_sec_ = bytes_per_sec;
+  hash_state_ = hash_state;
+  received_bytes_ = bytes_so_far;
+
+  // If we've received more data than we were expecting (bad server info?),
+  // revert to 'unknown size mode'.
+  if (received_bytes_ > total_bytes_)
+    total_bytes_ = 0;
+
+  if (bound_net_log_.IsLoggingAllEvents()) {
+    bound_net_log_.AddEvent(
+        net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
+        net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
+  }
+
+  UpdateObservers();
+}
+
+void DownloadItemImpl::DestinationError(
+    content::DownloadInterruptReason reason) {
+  Interrupt(reason);
+}
+
+void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
+  if (!IsInProgress())
+    return;
+  OnAllDataSaved(final_hash);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::Init(bool active,
@@ -1042,7 +1148,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
         " etag = '%s'"
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
-        " target_path = \"%" PRFilePath "\"",
+        " target_path = \"%" PRFilePath "\""
+        " has download file = %s",
         GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
@@ -1054,7 +1161,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
         GetETag().c_str(),
         url_list.c_str(),
         GetFullPath().value().c_str(),
-        GetTargetFilePath().value().c_str());
+        GetTargetFilePath().value().c_str(),
+        download_file_.get() ? "true" : "false");
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }