Replace the DownloadFileManager with direct ownership of DownloadFileImpl

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 78d39194720c8712968c5c0a4a7adbd40e0dfeeb..0780f54b20bb8a9e09e845c446abc53c55fdde45 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -4,7 +4,7 @@
 
 // File method ordering: Methods in this file are in the same order
 // as in download_item_impl.h, with the following exception: The public
-// interfaces DelayedDownloadOpened, OnDownloadTargetDetermined, and
+// interfaces Start, DelayedDownloadOpened, OnDownloadTargetDetermined, and
 // OnDownloadCompleting are placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state transitions
 // as the download progresses.  See "Download progression cascade" later in
@@ -37,7 +37,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"
@@ -104,6 +103,20 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
   }
 };
 
+// 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, base::Closure callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Detach(callback);
+}
+
+static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Cancel();
+}
+
 }  // namespace
 
 namespace content {
@@ -129,7 +142,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    DownloadId download_id,
                                    const DownloadPersistentStoreInfo& info,
                                    const net::BoundNetLog& bound_net_log)
-    : download_id_(download_id),
+    : is_save_package_download_(false),
+      download_id_(download_id),
       current_path_(info.path),
       target_path_(info.path),
       target_disposition_(TARGET_DISPOSITION_OVERWRITE),
@@ -176,7 +190,8 @@ DownloadItemImpl::DownloadItemImpl(
     const DownloadCreateInfo& info,
     scoped_ptr<DownloadRequestHandleInterface> request_handle,
     const net::BoundNetLog& bound_net_log)
-    : request_handle_(request_handle.Pass()),
+    : is_save_package_download_(false),
+      request_handle_(request_handle.Pass()),
       download_id_(info.download_id),
       target_disposition_(
           (info.prompt_user_for_save_location) ?
@@ -236,7 +251,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    DownloadId download_id,
                                    const std::string& mime_type,
                                    const net::BoundNetLog& bound_net_log)
-    : request_handle_(new NullDownloadRequestHandle()),
+    : is_save_package_download_(true),
+      request_handle_(new NullDownloadRequestHandle()),
       download_id_(download_id),
       current_path_(path),
       target_path_(path),
@@ -277,6 +293,11 @@ 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());
+
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
   delegate_->AssertStateConsistent(this);
   delegate_->Detach();
@@ -349,6 +370,21 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED_INTERNAL);
+
+  // Cancel and remove the download file.
+  // TODO(rdsmith/benjhayden): Remove condition as part of
+  // SavePackage integration.
+  if (!is_save_package_download_) {
+    CHECK(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);
 }
@@ -762,7 +798,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(),
@@ -773,7 +810,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());
   }
@@ -796,16 +834,6 @@ void DownloadItemImpl::OnDownloadedFileRemoved() {
   UpdateObservers();
 }
 
-void DownloadItemImpl::OffThreadCancel() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle_->CancelRequest();
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CancelDownload,
-                 delegate_->GetDownloadFileManager(), download_id_));
-}
-
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // Somewhat counter-intuitively, it is possible for us to receive an
@@ -823,11 +851,35 @@ void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
 
   last_reason_ = reason;
   TransitionTo(INTERRUPTED_INTERNAL);
+
+  // Cancel and remove the download file.
+  // TODO(rdsmith/benjhayden): Remove condition as part of
+  // SavePackage integration.
+  if (!is_save_package_download_) {
+    CHECK(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);
 }
 
+base::WeakPtr<content::DownloadDestinationObserver>
+DownloadItemImpl::DestinationObserverAsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+bool DownloadItemImpl::IsSavePackageDownload() const {
+  return is_save_package_download_;
+}
+
 void DownloadItemImpl::SetTotalBytes(int64 total_bytes) {
   total_bytes_ = total_bytes;
 }
@@ -870,13 +922,17 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
   UpdateObservers();
 }
 
-void DownloadItemImpl::OnAllDataSaved(
-    int64 size, const std::string& final_hash) {
+void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   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();
 }
 
@@ -901,6 +957,55 @@ void DownloadItemImpl::SetDbHandle(int64 handle) {
       net::NetLog::Int64Callback("db_handle", db_handle_));
 }
 
+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) {
+  // 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);
+}
+
 // **** Download progression cascade
 
 void DownloadItemImpl::Init(bool active,
@@ -944,7 +1049,38 @@ void DownloadItemImpl::Init(bool active,
   VLOG(20) << __FUNCTION__ << "() " << DebugString(true);
 }
 
-// Called by DownloadManagerImpl when the download target path has been
+// We're starting the download.
+void DownloadItemImpl::Start(scoped_ptr<content::DownloadFile> download_file) {
+  DCHECK(!download_file_.get());
+  DCHECK(download_file.get());

[benjhayden, 2012/09/24 17:43:27]

It took me a full 10 seconds to notice the underscore at the end of
download_file_ versus download_file in this line. Could you make the parameter
name a little more different? 'new_file', 'the_file', or just 'file'?

[Randy Smith (Not in Mondays), 2012/09/26 21:01:05]

Done.

+  download_file_ = download_file.Pass();
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFile::Initialize,
+                 // Safe because we control download file lifetime.

[benjhayden, 2012/09/24 17:43:27]

Just to check: even if an observer Cancels in OnDownloadCreated?
Unretained is still a little unnerving. A shim class could use weak pointers to
itself.

[Randy Smith (Not in Mondays), 2012/09/26 21:01:05]

Yep.  Our safety is guaranteed by two things:
* No-one besides the DownloadItem will ever initiate destruction of the
DownloadFile.
* If the DownloadItem initiates it, it'll be by a post to the file thread.  So
all previous file thread methods will have completed before that destruction
occurs. 
So I'm reluctant to put in boilerplate to obscure the central fact here: We own
the download file, and are responsible for tracking its lifetime and making sure
that it is used properly and only while it is alive.  No-one else.  Just us. 
Another way to put it is that no matter what boilerplate I put in, I think I'd
feel uncomfortable with the idea that anyone who didn't understand the lifetime
relationships and guarantees between DI and DF was modifying the DI->DF
communication (DF->DI is much more of a "fire-and-forget; if it's not there I
don't care", which is well summarized by the weak pointer semantics we have in
place).  

This may mean that I haven't explained or commented that lifetime relationship
sufficiently.  If you feel that way, please let me know that and let's work on
finding a better way to communicate it.  But I'm nervous about any coding here
that is designed to let the future maintainer get away with not understanding
the lifetime relationships in their gut--I think it'd fail.  Better to leave the
power tool accessible, and hopefully make people nervous when they pick it up
:-}.

+                 base::Unretained(download_file_.get()),
+                 base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
+                            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);
+}
+
+// Called by delegate_ when the download target path has been
 // determined.
 void DownloadItemImpl::OnDownloadTargetDetermined(
     const FilePath& target_path,
@@ -985,13 +1121,16 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   //               spurious rename when we can just rename to the final
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
-  DownloadFileManager::RenameCompletionCallback callback =
+  DCHECK(!is_save_package_download_);

[benjhayden, 2012/09/24 17:43:27]

Why shouldn't SavePackage use OnDownloadTargetDetermined to set the target path?

[Randy Smith (Not in Mondays), 2012/09/26 21:01:05]

I feel like I answered this question in commentary on your CL--if you don't
agree, could you re-raise it?

+  CHECK(download_file_.get());
+  DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::RenameDownloadFile,
-                 delegate_->GetDownloadFileManager(), GetGlobalId(),
+      base::Bind(&DownloadFile::Rename,
+                 // Safe because we control download file lifetime.
+                 base::Unretained(download_file_.get()),
                  intermediate_path, false, callback));
 }
 
@@ -1028,24 +1167,26 @@ void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(!GetTargetFilePath().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
 
+  // TODO(rdsmith/benjhayden): Remove as part of SavePackage integration.
+  if (is_save_package_download_) {
+    // Avoid doing anything on the file thread; there's nothing we control
+    // there.
+    OnDownloadFileReleased();
+    return;
+  }
+
+  CHECK(download_file_.get());
   if (NeedsRename()) {
-    DownloadFileManager::RenameCompletionCallback callback =
+    content::DownloadFile::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
                    weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileManager::RenameDownloadFile,
-                   delegate_->GetDownloadFileManager(), GetGlobalId(),
+        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,
-                   delegate_->GetDownloadFileManager(), GetGlobalId(),
-                   base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                              weak_ptr_factory_.GetWeakPtr())));
-    TransitionTo(COMPLETING_INTERNAL);
+    ReleaseDownloadFile();
   }
 }
 
@@ -1077,13 +1218,23 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   SetFullPath(full_path);
   delegate_->DownloadRenamedToFinalName(this);
 
+  ReleaseDownloadFile();
+}
+
+void DownloadItemImpl::ReleaseDownloadFile() {
   // Complete the download and release the DownloadFile.
+  DCHECK(!is_save_package_download_);
+  CHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CompleteDownload,
-                 delegate_->GetDownloadFileManager(), GetGlobalId(),
+      base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass()),
                  base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                             weak_ptr_factory_.GetWeakPtr())));
+
+  // We're not completely done with the download item yet, but at this
+  // point we're committed to complete the download.  Cancels (or Interrupts,
+  // though it's not clear how they could happen) after this point will be
+  // ignored.
   TransitionTo(COMPLETING_INTERNAL);
 }
 
@@ -1097,6 +1248,7 @@ void DownloadItemImpl::OnDownloadFileReleased() {
 void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
   auto_opened_ = auto_opened;
   Completed();
+  UpdateObservers();
 }
 
 void DownloadItemImpl::Completed() {
@@ -1133,21 +1285,6 @@ bool DownloadItemImpl::NeedsRename() const {
   return target_path_ != current_path_;
 }
 
-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;
-}
-
 void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   if (state_ == new_state)
     return;