Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(444)

Unified Diff: content/browser/download/download_item_impl.cc

Issue 10799005: Replace the DownloadFileManager with direct ownership (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Fixed try job problems. Created 8 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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();
benjhayden 2012/07/25 15:19:16 Why don't you want to use SupportsWeakPtr::AsWeakP
Randy Smith (Not in Mondays) 2012/07/30 01:07:23 A couple of reasons. My usual reason (I don't wan
+}
+
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.
benjhayden 2012/07/25 15:19:16 Couldn't you use weak pointers to bounce a callbac
Randy Smith (Not in Mondays) 2012/07/30 01:07:23 I assume you mean off of the DownloadFile; I am us
+ 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());
}

Powered by Google App Engine
This is Rietveld 408576698