Revert 152213 - Replace the DownloadFileManager with direct ownership of DownloadFile.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
===================================================================
--- content/browser/download/download_item_impl.cc	(revision 152281)
+++ content/browser/download/download_item_impl.cc	(working copy)
@@ -19,6 +19,7 @@
 #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"
@@ -118,19 +119,6 @@
   }
 };
 
-// Wrapper around DownloadFile::Detach and DownloadFile::Cancel that
-// takes ownership of the DownloadFile and hence implicitly destroys it
-// at the end of the function.
-static void DownloadFileDetach(scoped_ptr<DownloadFile> download_file) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  download_file->Detach();
-}
-
-static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  download_file->Cancel();
-}
-
 }  // namespace
 
 namespace content {
@@ -304,22 +292,11 @@
 
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Should always have been nuked before now, at worst in
-  // DownloadManager shutdown.
-  DCHECK(!download_file_.get());
-
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
   delegate_->AssertStateConsistent(this);
   delegate_->Detach();
 }
 
-base::WeakPtr<content::DownloadDestinationObserver>
-DownloadItemImpl::DestinationObserverAsWeakPtr() {
-  // Return does private downcast.
-  return weak_ptr_factory_.GetWeakPtr();
-}
-
 void DownloadItemImpl::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -408,6 +385,21 @@
   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.
@@ -464,35 +456,10 @@
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED);
-
-  // Cancel and remove the download file.
-  DCHECK(download_file_.get());
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      // Will be deleted at end of task execution.
-      base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
-
-  // Cancel the originating URL request.
-  request_handle_->CancelRequest();
-
   if (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
@@ -510,17 +477,6 @@
 
   last_reason_ = reason;
   TransitionTo(INTERRUPTED);
-
-  // Cancel and remove the download file.
-  DCHECK(download_file_.get());
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      // Will be deleted at end of task execution.
-      base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
-
-  // Cancel the originating URL request.
-  request_handle_->CancelRequest();
-
   download_stats::RecordDownloadInterrupted(
       reason, received_bytes_, total_bytes_);
   delegate_->DownloadStopped(this);
@@ -540,17 +496,12 @@
 }
 
 void DownloadItemImpl::OnAllDataSaved(
-    const std::string& final_hash) {
+    int64 size, const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DCHECK_EQ(IN_PROGRESS, state_);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
-
-  // Store final hash and null out intermediate serialized hash state.
-  hash_ = final_hash;
-  hash_state_ = "";
-
+  ProgressComplete(size, final_hash);
   UpdateObservers();
 }
 
@@ -745,32 +696,29 @@
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (!IsInProgress())
-    return;
-
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetName().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
 
-  DCHECK(download_file_.get());
   if (NeedsRename()) {
-    content::DownloadFile::RenameCompletionCallback callback =
+    DownloadFileManager::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
                    weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFile::Rename,
-                   base::Unretained(download_file_.get()),
+        base::Bind(&DownloadFileManager::RenameDownloadFile,
+                   delegate_->GetDownloadFileManager(), GetGlobalId(),
                    GetTargetFilePath(), true, callback));
   } else {
     // Complete the download and release the DownloadFile.
-    BrowserThread::PostTaskAndReply(
+    BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
-        base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                   weak_ptr_factory_.GetWeakPtr()));
+        base::Bind(&DownloadFileManager::CompleteDownload,
+                   delegate_->GetDownloadFileManager(), GetGlobalId(),
+                   base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
+                              weak_ptr_factory_.GetWeakPtr())));
   }
 }
 
@@ -779,9 +727,6 @@
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (!IsInProgress())
-    return;
-
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
@@ -800,35 +745,14 @@
   delegate_->DownloadRenamedToFinalName(this);
 
   // Complete the download and release the DownloadFile.
-  // TODO(rdsmith): Unify this path with the !NeedsRename() path in
-  // OnDownloadCompleting above.  This can happen easily after history
-  // is made into an observer and the path accessors are cleaned up;
-  // that should allow OnDownloadCompleting to simply call
-  // OnDownloadRenamedToFinalName directly.
-  DCHECK(download_file_.get());
-  BrowserThread::PostTaskAndReply(
+  BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
-      base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&DownloadFileManager::CompleteDownload,
+                 delegate_->GetDownloadFileManager(), GetGlobalId(),
+                 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();
@@ -957,30 +881,18 @@
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
-  if (!IsInProgress()) {
-    // If we've been cancelled or interrupted while the target was being
-    // determined, continue the cascade with a null name.
-    // The error doesn't matter as the cause of download stoppaged
-    // will already have been recorded.
-    OnDownloadRenamedToIntermediateName(
-        content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED, FilePath());
-    return;
-  }
-
   // Rename to intermediate name.
   // TODO(asanka): Skip this rename if AllDataSaved() is true. This avoids a
   //               spurious rename when we can just rename to the final
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
-  DCHECK(download_file_.get());
-  DownloadFile::RenameCompletionCallback callback =
+  DownloadFileManager::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFile::Rename,
-                 // Safe because we control download file lifetime.
-                 base::Unretained(download_file_.get()),
+      base::Bind(&DownloadFileManager::RenameDownloadFile,
+                 delegate_->GetDownloadFileManager(), GetGlobalId(),
                  intermediate_path, false, callback));
 }
 
@@ -1011,55 +923,16 @@
       GetTargetFilePath() : GetFullPath();
 }
 
-void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
-                                         int64 bytes_per_sec,
-                                         const std::string& hash_state) {
+void DownloadItemImpl::OffThreadCancel() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  request_handle_->CancelRequest();
 
-  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();
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CancelDownload,
+                 delegate_->GetDownloadFileManager(), download_id_));
 }
 
-void DownloadItemImpl::DestinationError(
-    content::DownloadInterruptReason reason) {
-  // The DestinationError and Interrupt routines are being kept separate
-  // to allow for a future merging of the Cancel and Interrupt routines..
-  Interrupt(reason);
-}
-
-void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
-  if (!IsInProgress())
-    return;
-  OnAllDataSaved(final_hash);
-  delegate_->MaybeCompleteDownload(this);
-}
-
 void DownloadItemImpl::Init(bool active,
                             download_net_logs::DownloadType download_type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -1167,8 +1040,7 @@
         " etag = '%s'"
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
-        " target_path = \"%" PRFilePath "\""
-        " has download file = %s",
+        " target_path = \"%" PRFilePath "\"",
         GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
@@ -1179,8 +1051,7 @@
         GetETag().c_str(),
         url_list.c_str(),
         GetFullPath().value().c_str(),
-        GetTargetFilePath().value().c_str(),
-        download_file_.get() ? "true" : "false");
+        GetTargetFilePath().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }