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 cef57731792e96e8832ee0c0962b58a5d0f75b8b..7a66b4e5aea844a4f75247698b07d5c207110ad7 100644 |
--- a/content/browser/download/download_item_impl.cc |
+++ b/content/browser/download/download_item_impl.cc |
@@ -160,7 +160,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, |
bound_net_log_(bound_net_log), |
weak_ptr_factory_(this) { |
delegate_->Attach(); |
- DCHECK_NE(IN_PROGRESS_INTERNAL, state_); |
+ DCHECK(state_ == COMPLETE_INTERNAL || state_ == INTERRUPTED_INTERNAL || |
+ state_ == CANCELLED_INTERNAL); |
Init(false /* not actively downloading */, SRC_HISTORY_IMPORT); |
} |
@@ -193,7 +194,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, |
etag_(info.etag), |
last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE), |
start_tick_(base::TimeTicks::Now()), |
- state_(IN_PROGRESS_INTERNAL), |
+ state_(INITIAL_INTERNAL), |
danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), |
start_time_(info.start_time), |
delegate_(delegate), |
@@ -327,6 +328,7 @@ void DownloadItemImpl::StealDangerousDownload( |
DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK(IsDangerous()); |
+ |
if (download_file_) { |
BrowserThread::PostTaskAndReplyWithResult( |
BrowserThread::FILE, |
@@ -345,17 +347,46 @@ void DownloadItemImpl::Pause() { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
// Ignore irrelevant states. |
- if (state_ != IN_PROGRESS_INTERNAL || is_paused_) |
+ if (is_paused_) |
return; |
- request_handle_->PauseRequest(); |
- is_paused_ = true; |
- UpdateObservers(); |
+ switch (state_) { |
+ case INITIAL_INTERNAL: |
+ case COMPLETING_INTERNAL: |
+ case COMPLETE_INTERNAL: |
+ case INTERRUPTED_INTERNAL: |
+ case CANCELLED_INTERNAL: |
+ case RESUMING_INTERNAL: |
+ // No active request. |
+ return; |
+ |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ case IN_PROGRESS_INTERNAL: |
+ request_handle_->PauseRequest(); |
+ is_paused_ = true; |
+ UpdateObservers(); |
+ return; |
+ |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
+ } |
} |
void DownloadItemImpl::Resume() { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
switch (state_) { |
+ case INITIAL_INTERNAL: |
+ case COMPLETING_INTERNAL: |
+ case COMPLETE_INTERNAL: |
+ case CANCELLED_INTERNAL: |
+ // Nothing to resume. |
+ case RESUMING_INTERNAL: |
+ // Resumption in progress. |
+ return; |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
suggestion: DCHECK(!is_paused_)?
asanka
2016/02/12 05:04:26
Done.
|
+ |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
case IN_PROGRESS_INTERNAL: |
if (!is_paused_) |
return; |
@@ -364,12 +395,6 @@ void DownloadItemImpl::Resume() { |
UpdateObservers(); |
return; |
- case COMPLETING_INTERNAL: |
- case COMPLETE_INTERNAL: |
- case CANCELLED_INTERNAL: |
- case RESUMING_INTERNAL: |
- return; |
- |
case INTERRUPTED_INTERNAL: |
auto_resume_count_ = 0; // User input resets the counter. |
ResumeInterruptedDownload(); |
@@ -382,52 +407,9 @@ void DownloadItemImpl::Resume() { |
void DownloadItemImpl::Cancel(bool user_cancel) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
- |
DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true); |
- if (state_ != IN_PROGRESS_INTERNAL && |
- state_ != INTERRUPTED_INTERNAL && |
- state_ != RESUMING_INTERNAL) { |
- // Small downloads might be complete before this method has a chance to run. |
- return; |
- } |
- |
- if (IsDangerous()) { |
- RecordDangerousDownloadDiscard( |
- user_cancel ? DOWNLOAD_DISCARD_DUE_TO_USER_ACTION |
- : DOWNLOAD_DISCARD_DUE_TO_SHUTDOWN, |
- GetDangerType(), |
- GetTargetFilePath()); |
- } |
- |
- last_reason_ = user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED |
- : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN; |
- |
- RecordDownloadCount(CANCELLED_COUNT); |
- |
- // TODO(rdsmith/benjhayden): Remove condition as part of |
- // |SavePackage| integration. |
- // |download_file_| can be NULL if Interrupt() is called after the |
- // download file has been released. |
- if (!is_save_package_download_ && download_file_) |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
Interesting. Was the is_save_package_download_ ch
asanka
2016/02/12 05:04:26
It was cleanup. I don't know why it was there eith
|
- ReleaseDownloadFile(true); |
- |
- if (state_ == IN_PROGRESS_INTERNAL) { |
- // Cancel the originating URL request unless it's already been cancelled |
- // by interrupt. |
- request_handle_->CancelRequest(); |
- } |
- |
- // Remove the intermediate file if we are cancelling an interrupted download. |
- // Continuable interruptions leave the intermediate file around. |
- if ((state_ == INTERRUPTED_INTERNAL || state_ == RESUMING_INTERNAL) && |
- !current_path_.empty()) { |
- BrowserThread::PostTask( |
- BrowserThread::FILE, FROM_HERE, |
- base::Bind(base::IgnoreResult(&DeleteDownloadedFile), current_path_)); |
- current_path_.clear(); |
- } |
- |
- TransitionTo(CANCELLED_INTERNAL, UPDATE_OBSERVERS); |
+ Interrupt(user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED |
+ : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN); |
} |
void DownloadItemImpl::Remove() { |
@@ -435,7 +417,7 @@ void DownloadItemImpl::Remove() { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
delegate_->AssertStateConsistent(this); |
- Cancel(true); |
+ Interrupt(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED); |
delegate_->AssertStateConsistent(this); |
NotifyRemoved(); |
@@ -495,26 +477,43 @@ bool DownloadItemImpl::IsTemporary() const { |
} |
bool DownloadItemImpl::CanResume() const { |
- if ((GetState() == IN_PROGRESS) && IsPaused()) |
- return true; |
- |
- if (state_ != INTERRUPTED_INTERNAL) |
- return false; |
+ DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ switch (state_) { |
+ case INITIAL_INTERNAL: |
+ case COMPLETING_INTERNAL: |
+ case COMPLETE_INTERNAL: |
+ case CANCELLED_INTERNAL: |
+ case RESUMING_INTERNAL: |
+ return false; |
- // We currently only support HTTP(S) requests for download resumption. |
- if (!GetURL().SchemeIsHTTPOrHTTPS()) |
- return false; |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ case IN_PROGRESS_INTERNAL: |
+ return is_paused_; |
+ |
+ case INTERRUPTED_INTERNAL: { |
+ ResumeMode resume_mode = GetResumeMode(); |
+ // Only allow Resume() calls if the resumption mode requires a user |
+ // action. |
+ return IsDownloadResumptionEnabled() && |
+ (resume_mode == RESUME_MODE_USER_RESTART || |
+ resume_mode == RESUME_MODE_USER_CONTINUE); |
+ } |
- ResumeMode resume_mode = GetResumeMode(); |
- return IsDownloadResumptionEnabled() && |
- (resume_mode == RESUME_MODE_USER_RESTART || |
- resume_mode == RESUME_MODE_USER_CONTINUE); |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
+ } |
+ return false; |
} |
bool DownloadItemImpl::IsDone() const { |
switch (state_) { |
- case IN_PROGRESS_INTERNAL: |
+ case INITIAL_INTERNAL: |
case COMPLETING_INTERNAL: |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
Not in this CL (as not changed in this CL): I find
asanka
2016/02/12 05:04:26
Acknowledged.
|
+ case RESUMING_INTERNAL: |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ case IN_PROGRESS_INTERNAL: |
return false; |
case COMPLETE_INTERNAL: |
@@ -524,14 +523,10 @@ bool DownloadItemImpl::IsDone() const { |
case INTERRUPTED_INTERNAL: |
return !CanResume(); |
- case RESUMING_INTERNAL: |
- return false; |
- |
case MAX_DOWNLOAD_INTERNAL_STATE: |
- break; |
+ NOTREACHED(); |
} |
- NOTREACHED(); |
- return true; |
+ return false; |
} |
const GURL& DownloadItemImpl::GetURL() const { |
@@ -770,6 +765,14 @@ WebContents* DownloadItemImpl::GetWebContents() const { |
void DownloadItemImpl::OnContentCheckCompleted(DownloadDangerType danger_type) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK(AllDataSaved()); |
+ |
+ // Danger type is only allowed to be set on an active download after all data |
+ // has been saved. This excludes all other states. In particular, |
+ // OnContentCheckCompleted() isn't allowed on an INTERRUPTED download since |
+ // such an interruption would need to happen between OnAllDataSaved() and |
+ // OnContentCheckCompleted() during which no disk or network activity |
+ // should've taken place. |
+ DCHECK_EQ(state_, IN_PROGRESS_INTERNAL); |
DVLOG(20) << __FUNCTION__ << " danger_type=" << danger_type |
<< " download=" << DebugString(true); |
SetDangerType(danger_type); |
@@ -854,6 +857,14 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { |
DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ |
+ if (!IsDownloadResumptionEnabled()) |
+ return RESUME_MODE_INVALID; |
+ |
+ // Cnly support resumption for HTTP(S). |
Randy Smith (Not in Mondays)
2016/02/12 00:03:10
Interesting. This seems like a semantic change th
asanka
2016/02/12 05:04:26
I moved this check from CanResume() to here. Earli
|
+ if (!GetURL().SchemeIsHTTPOrHTTPS()) |
+ return RESUME_MODE_INVALID; |
+ |
// We can't continue without a handle on the intermediate file. |
// We also can't continue if we don't have some verifier to make sure |
// we're getting the same file. |
@@ -994,8 +1005,8 @@ void DownloadItemImpl::SetTotalBytes(int64_t total_bytes) { |
void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
- |
- DCHECK_EQ(IN_PROGRESS_INTERNAL, state_); |
+ DCHECK(state_ == TARGET_PENDING_INTERNAL || |
+ state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); |
Randy Smith (Not in Mondays)
2016/02/12 00:03:10
DestinationCompleted comes in via post from the fi
asanka
2016/02/12 05:04:26
This one is a bit subtle. Basically, if the DII tr
Randy Smith (Not in Mondays)
2016/02/12 17:37:02
Ah, thank you. However, as I read the code, what
asanka
2016/02/12 20:21:24
Yeah. We rely on download_file_ only being set to
|
DCHECK(!all_data_saved_); |
all_data_saved_ = true; |
DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true); |
@@ -1054,10 +1065,15 @@ void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far, |
} |
void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) { |
+ DCHECK(state_ == TARGET_PENDING_INTERNAL || |
+ state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); |
+ DVLOG(20) << __FUNCTION__ |
+ << "() reason:" << DownloadInterruptReasonToString(reason); |
+ |
// Postpone recognition of this error until after file name determination |
// has completed and the intermediate file has been renamed to simplify |
// resumption conditions. |
- if (current_path_.empty() || target_path_.empty()) |
+ if (state_ != IN_PROGRESS_INTERNAL) |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
Confirming I understand (if this is true, I'm ok w
asanka
2016/02/12 05:04:26
Correct.
|
destination_error_ = reason; |
else |
Interrupt(reason); |
@@ -1065,8 +1081,6 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) { |
void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) { |
DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true); |
- if (GetState() != IN_PROGRESS) |
- return; |
OnAllDataSaved(final_hash); |
MaybeCompleteDownload(); |
} |
@@ -1116,11 +1130,12 @@ void DownloadItemImpl::Start( |
DCHECK(!download_file_.get()); |
DCHECK(file.get()); |
DCHECK(req_handle.get()); |
+ DVLOG(20) << __FUNCTION__ << "() this=" << DebugString(true); |
download_file_ = std::move(file); |
request_handle_ = std::move(req_handle); |
- if (GetState() == CANCELLED) { |
+ if (state_ == CANCELLED_INTERNAL) { |
// The download was in the process of resuming when it was cancelled. Don't |
// proceed. |
ReleaseDownloadFile(true); |
@@ -1128,7 +1143,7 @@ void DownloadItemImpl::Start( |
return; |
} |
- TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS); |
+ TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS); |
BrowserThread::PostTask( |
BrowserThread::FILE, FROM_HERE, |
@@ -1142,7 +1157,13 @@ void DownloadItemImpl::Start( |
void DownloadItemImpl::OnDownloadFileInitialized( |
DownloadInterruptReason result) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DCHECK_EQ(state_, TARGET_PENDING_INTERNAL); |
+ DVLOG(20) << __FUNCTION__ |
+ << "() result:" << DownloadInterruptReasonToString(result); |
if (result != DOWNLOAD_INTERRUPT_REASON_NONE) { |
+ // Transition out to TARGET_RESOLVED_INTERNAL since this DownloadItem is |
+ // skipping the download target determination process. |
+ TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS); |
Interrupt(result); |
// TODO(rdsmith/asanka): Arguably we should show this in the UI, but |
// it's not at all clear what to show--we haven't done filename |
@@ -1168,6 +1189,7 @@ void DownloadItemImpl::OnDownloadTargetDetermined( |
DownloadDangerType danger_type, |
const base::FilePath& intermediate_path) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DCHECK_EQ(state_, TARGET_PENDING_INTERNAL); |
// If the |target_path| is empty, then we consider this download to be |
// canceled. |
@@ -1231,7 +1253,9 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName( |
DownloadInterruptReason reason, |
const base::FilePath& full_path) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DCHECK_EQ(state_, TARGET_PENDING_INTERNAL); |
DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true); |
+ TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS); |
if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) { |
// Process destination error. If both |reason| and |destination_error_| |
@@ -1249,7 +1273,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName( |
DCHECK(current_path_.empty()); |
} else { |
SetFullPath(full_path); |
- UpdateObservers(); |
+ TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS); |
MaybeCompleteDownload(); |
} |
} |
@@ -1272,12 +1296,8 @@ void DownloadItemImpl::MaybeCompleteDownload() { |
weak_ptr_factory_.GetWeakPtr()))) |
return; |
- // TODO(rdsmith): DCHECK that we only pass through this point |
- // once per download. The natural way to do this is by a state |
- // transition on the DownloadItem. |
- |
- // Confirm we're in the proper set of states to be here; |
- // have all data, have a history handle, (validated or safe). |
+ // Confirm we're in the proper set of states to be here; have all data, have a |
+ // history handle, (validated or safe). |
DCHECK_EQ(IN_PROGRESS_INTERNAL, state_); |
DCHECK(!IsDangerous()); |
DCHECK(all_data_saved_); |
@@ -1356,7 +1376,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( |
} |
// Complete the download and release the DownloadFile. |
- DCHECK(download_file_.get()); |
+ DCHECK(download_file_); |
ReleaseDownloadFile(false); |
// We're not completely done with the download item yet, but at this |
@@ -1430,6 +1450,9 @@ void DownloadItemImpl::OnResumeRequestStarted( |
void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason); |
+ DVLOG(20) << __FUNCTION__ |
+ << "() reason:" << DownloadInterruptReasonToString(reason) |
+ << " this=" << DebugString(true); |
// Somewhat counter-intuitively, it is possible for us to receive an |
// interrupt after we've already been interrupted. The generation of |
@@ -1439,56 +1462,90 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { |
// respectively), and since we choose not to keep state on the File thread, |
// this is the place where the races collide. It's also possible for |
// interrupts to race with cancels. |
+ switch (state_) { |
+ case CANCELLED_INTERNAL: |
+ // If the download is already cancelled, then there's no point in |
+ // transitioning out to interrupted. |
+ case COMPLETING_INTERNAL: |
+ case COMPLETE_INTERNAL: |
+ // Already complete. |
+ return; |
- // Whatever happens, the first one to hit the UI thread wins. |
- if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL) |
- return; |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ case IN_PROGRESS_INTERNAL: |
+ break; |
- last_reason_ = reason; |
+ case RESUMING_INTERNAL: |
+ case INTERRUPTED_INTERNAL: |
+ // The first non-cancel interrupt reason wins in cases where multiple |
+ // things go wrong. |
+ if (reason != DOWNLOAD_INTERRUPT_REASON_USER_CANCELED && |
+ reason != DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN) |
+ return; |
- ResumeMode resume_mode = GetResumeMode(); |
- |
- if (state_ == IN_PROGRESS_INTERNAL) { |
- // Cancel (delete file) if: |
- // 1) we're going to restart. |
- // 2) Resumption isn't possible (download was cancelled or blocked due to |
- // security restrictions). |
- // 3) Resumption isn't enabled. |
- // No point in leaving data around we aren't going to use. |
- ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART || |
- resume_mode == RESUME_MODE_USER_RESTART || |
- resume_mode == RESUME_MODE_INVALID || |
- !IsDownloadResumptionEnabled()); |
- |
- // Cancel the originating URL request. |
- request_handle_->CancelRequest(); |
- } else { |
- DCHECK(!download_file_.get()); |
+ if (!current_path_.empty()) { |
+ BrowserThread::PostTask( |
+ BrowserThread::FILE, FROM_HERE, |
+ base::Bind(base::IgnoreResult(&DeleteDownloadedFile), |
+ current_path_)); |
+ current_path_.clear(); |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
Am I right in thinking that if this path has been
asanka
2016/02/12 05:04:26
I moved the download_file_ handling to one place,
|
+ } |
+ break; |
+ |
+ case INITIAL_INTERNAL: |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
+ return; |
} |
- // Reset all data saved, as even if we did save all the data we're going |
- // to go through another round of downloading when we resume. |
- // There's a potential problem here in the abstract, as if we did download |
- // all the data and then run into a continuable error, on resumption we |
- // won't download any more data. However, a) there are currently no |
- // continuable errors that can occur after we download all the data, and |
- // b) if there were, that would probably simply result in a null range |
- // request, which would generate a DestinationCompleted() notification |
- // from the DownloadFile, which would behave properly with setting |
- // all_data_saved_ to false here. |
+ last_reason_ = reason; |
+ if (download_file_) { |
+ ResumeMode resume_mode = GetResumeMode(); |
+ ReleaseDownloadFile(resume_mode != RESUME_MODE_IMMEDIATE_CONTINUE && |
+ resume_mode != RESUME_MODE_USER_CONTINUE); |
+ } |
+ |
+ // Reset all data saved, as even if we did save all the data we're going to go |
+ // through another round of downloading when we resume. There's a potential |
+ // problem here in the abstract, as if we did download all the data and then |
+ // run into a continuable error, on resumption we won't download any more |
+ // data. However, a) there are currently no continuable errors that can occur |
+ // after we download all the data, and b) if there were, that would probably |
+ // simply result in a null range request, which would generate a |
+ // DestinationCompleted() notification from the DownloadFile, which would |
+ // behave properly with setting all_data_saved_ to false here. |
all_data_saved_ = false; |
- TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS); |
- RecordDownloadInterrupted(reason, received_bytes_, total_bytes_); |
- if (!GetWebContents()) |
- RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS); |
+ if (request_handle_) |
+ request_handle_->CancelRequest(); |
+ |
+ if (reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED || |
+ reason == DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN) { |
+ if (IsDangerous()) { |
+ RecordDangerousDownloadDiscard( |
+ reason == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED |
+ ? DOWNLOAD_DISCARD_DUE_TO_USER_ACTION |
+ : DOWNLOAD_DISCARD_DUE_TO_SHUTDOWN, |
+ GetDangerType(), GetTargetFilePath()); |
+ } |
+ |
+ RecordDownloadCount(CANCELLED_COUNT); |
+ TransitionTo(CANCELLED_INTERNAL, DONT_UPDATE_OBSERVERS); |
+ } else { |
+ RecordDownloadInterrupted(reason, received_bytes_, total_bytes_); |
+ if (!GetWebContents()) |
+ RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS); |
+ TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS); |
+ AutoResumeIfValid(); |
+ } |
- AutoResumeIfValid(); |
UpdateObservers(); |
} |
void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) { |
DCHECK_CURRENTLY_ON(BrowserThread::UI); |
+ DVLOG(20) << __FUNCTION__ << "() destroy_file:" << destroy_file; |
if (destroy_file) { |
BrowserThread::PostTask( |
@@ -1514,6 +1571,11 @@ void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) { |
bool DownloadItemImpl::IsDownloadReadyForCompletion( |
const base::Closure& state_change_notification) { |
+ // If the download hasn't progressed to the IN_PROGRESS state, then it's not |
+ // ready for completion. |
+ if (state_ != IN_PROGRESS_INTERNAL) |
+ return false; |
+ |
// If we don't have all the data, the download is not ready for |
// completion. |
if (!AllDataSaved()) |
@@ -1524,20 +1586,11 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion( |
if (IsDangerous()) |
return false; |
- // If the download isn't active (e.g. has been cancelled) it's not |
- // ready for completion. |
- if (state_ != IN_PROGRESS_INTERNAL) |
- return false; |
- |
- // If the target filename hasn't been determined, then it's not ready for |
- // completion. This is checked in ReadyForDownloadCompletionDone(). |
- if (GetTargetFilePath().empty()) |
- return false; |
- |
- // This is checked in NeedsRename(). Without this conditional, |
- // browser_tests:DownloadTest.DownloadMimeType fails the DCHECK. |
- if (target_path_.DirName() != current_path_.DirName()) |
- return false; |
+ // Invariants for the IN_PROGRESS state. DCHECKs here verify that the |
+ // invariants are still true. |
+ DCHECK(!target_path_.empty()); |
+ DCHECK(!current_path_.empty()); |
+ DCHECK(target_path_.DirName() == current_path_.DirName()); |
// Give the delegate a chance to hold up a stop sign. It'll call |
// use back through the passed callback if it does and that state changes. |
@@ -1557,49 +1610,89 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, |
DownloadInternalState old_state = state_; |
state_ = new_state; |
+ DCHECK(is_save_package_download_ |
+ ? IsValidSavePackageStateTransition(old_state, new_state) |
+ : IsValidStateTransition(old_state, new_state)) |
+ << "Invalid state transition from:" << DebugDownloadStateString(old_state) |
+ << " to:" << DebugDownloadStateString(new_state); |
+ |
switch (state_) { |
+ case INITIAL_INTERNAL: |
+ NOTREACHED(); |
+ break; |
+ |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ break; |
+ |
+ case IN_PROGRESS_INTERNAL: |
+ DCHECK(!current_path_.empty()) << "Current output path must be known."; |
+ DCHECK(!target_path_.empty()) << "Target path must be known."; |
+ DCHECK(current_path_.DirName() == target_path_.DirName()) |
+ << "Current output directory must match target directory."; |
+ DCHECK(download_file_) << "Output file must be owned by download item."; |
+ DCHECK(request_handle_) << "Download source must be active."; |
+ DCHECK(!is_paused_) << "At the time a download enters IN_PROGRESS state, " |
+ "it must not be paused."; |
+ break; |
+ |
case COMPLETING_INTERNAL: |
+ DCHECK(all_data_saved_) << "All data must be saved prior to completion."; |
+ DCHECK(!download_file_) |
+ << "Download file must be released prior to completion."; |
+ DCHECK(!target_path_.empty()) << "Target path must be known."; |
+ DCHECK(current_path_ == target_path_) |
+ << "Current output path must match target path."; |
+ |
bound_net_log_.AddEvent( |
net::NetLog::TYPE_DOWNLOAD_ITEM_COMPLETING, |
base::Bind(&ItemCompletingNetLogCallback, received_bytes_, &hash_)); |
break; |
+ |
case COMPLETE_INTERNAL: |
bound_net_log_.AddEvent( |
net::NetLog::TYPE_DOWNLOAD_ITEM_FINISHED, |
base::Bind(&ItemFinishedNetLogCallback, auto_opened_)); |
break; |
+ |
case INTERRUPTED_INTERNAL: |
bound_net_log_.AddEvent( |
net::NetLog::TYPE_DOWNLOAD_ITEM_INTERRUPTED, |
base::Bind(&ItemInterruptedNetLogCallback, last_reason_, |
received_bytes_, &hash_state_)); |
break; |
- case IN_PROGRESS_INTERNAL: |
- if (old_state == INTERRUPTED_INTERNAL) { |
- bound_net_log_.AddEvent( |
- net::NetLog::TYPE_DOWNLOAD_ITEM_RESUMED, |
- base::Bind(&ItemResumingNetLogCallback, |
- false, last_reason_, received_bytes_, &hash_state_)); |
- } |
+ |
+ case RESUMING_INTERNAL: |
+ bound_net_log_.AddEvent( |
+ net::NetLog::TYPE_DOWNLOAD_ITEM_RESUMED, |
+ base::Bind(&ItemResumingNetLogCallback, false, last_reason_, |
+ received_bytes_, &hash_state_)); |
break; |
+ |
case CANCELLED_INTERNAL: |
bound_net_log_.AddEvent( |
net::NetLog::TYPE_DOWNLOAD_ITEM_CANCELED, |
base::Bind(&ItemCanceledNetLogCallback, received_bytes_, |
&hash_state_)); |
break; |
- default: |
+ |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
break; |
} |
- DVLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true) |
- << " " << InternalToExternalState(old_state) |
- << " " << InternalToExternalState(state_); |
- |
- bool is_done = (state_ != IN_PROGRESS_INTERNAL && |
- state_ != COMPLETING_INTERNAL); |
- bool was_done = (old_state != IN_PROGRESS_INTERNAL && |
- old_state != COMPLETING_INTERNAL); |
+ DVLOG(20) << " " << __FUNCTION__ << "()" |
+ << " from:" << DebugDownloadStateString(old_state) |
+ << " to:" << DebugDownloadStateString(state_) |
+ << " this = " << DebugString(true); |
+ bool is_done = |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
nit, suggestion: I'm feeling tempted to suggest in
asanka
2016/02/12 05:04:26
Done.
|
+ (state_ != TARGET_PENDING_INTERNAL && |
+ state_ != TARGET_RESOLVED_INTERNAL && state_ != IN_PROGRESS_INTERNAL && |
+ state_ != COMPLETING_INTERNAL); |
+ bool was_done = |
+ (old_state != TARGET_PENDING_INTERNAL && |
+ old_state != TARGET_RESOLVED_INTERNAL && |
+ old_state != IN_PROGRESS_INTERNAL && old_state != COMPLETING_INTERNAL); |
// Termination |
if (is_done && !was_done) |
bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE); |
@@ -1704,17 +1797,23 @@ void DownloadItemImpl::ResumeInterruptedDownload() { |
base::Bind(&DownloadItemImpl::OnResumeRequestStarted, |
weak_ptr_factory_.GetWeakPtr())); |
+ TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS); |
delegate_->ResumeInterruptedDownload(std::move(download_params), GetId()); |
// Just in case we were interrupted while paused. |
is_paused_ = false; |
- |
- TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS); |
} |
// static |
DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState( |
DownloadInternalState internal_state) { |
switch (internal_state) { |
+ case INITIAL_INTERNAL: |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ // TODO(asanka): Introduce an externally visible state to distinguish |
+ // between the above states and IN_PROGRESS_INTERNAL. The latter (the |
+ // state where the download is active and has a known target) is the state |
+ // that most external users are interested in. |
case IN_PROGRESS_INTERNAL: |
return IN_PROGRESS; |
case COMPLETING_INTERNAL: |
@@ -1753,9 +1852,80 @@ DownloadItemImpl::ExternalToInternalState( |
return MAX_DOWNLOAD_INTERNAL_STATE; |
} |
+#if DCHECK_IS_ON() |
Randy Smith (Not in Mondays)
2016/02/12 00:03:11
nit, suggestion: I wince a bit at declaration a ro
asanka
2016/02/12 05:04:26
Opted to put the #if in the .h file.
asanka
2016/02/12 15:53:36
Well, that didn't. Apparently the symbol still nee
|
+// static |
+bool DownloadItemImpl::IsValidSavePackageStateTransition( |
+ DownloadInternalState from, |
+ DownloadInternalState to) { |
+ switch (from) { |
+ case INITIAL_INTERNAL: |
+ case TARGET_PENDING_INTERNAL: |
+ case TARGET_RESOLVED_INTERNAL: |
+ case COMPLETING_INTERNAL: |
+ case COMPLETE_INTERNAL: |
+ case INTERRUPTED_INTERNAL: |
+ case RESUMING_INTERNAL: |
+ case CANCELLED_INTERNAL: |
+ return false; |
+ |
+ case IN_PROGRESS_INTERNAL: |
+ return to == CANCELLED_INTERNAL || COMPLETE_INTERNAL; |
Randy Smith (Not in Mondays)
2016/02/12 00:03:10
I think you were typing a little too fast here :-}
asanka
2016/02/12 05:04:26
Hehe :) Done.
|
+ |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
+ } |
+ return false; |
+} |
+ |
+// static |
+bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from, |
+ DownloadInternalState to) { |
+ switch (from) { |
+ case INITIAL_INTERNAL: |
+ return to == TARGET_PENDING_INTERNAL || to == INTERRUPTED_INTERNAL; |
+ |
+ case TARGET_PENDING_INTERNAL: |
+ return to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL; |
+ |
+ case TARGET_RESOLVED_INTERNAL: |
+ return to == IN_PROGRESS_INTERNAL || to == INTERRUPTED_INTERNAL || |
+ to == CANCELLED_INTERNAL; |
+ |
+ case IN_PROGRESS_INTERNAL: |
+ return to == COMPLETING_INTERNAL || to == CANCELLED_INTERNAL || |
+ to == INTERRUPTED_INTERNAL; |
+ |
+ case COMPLETING_INTERNAL: |
+ return to == COMPLETE_INTERNAL; |
+ |
+ case COMPLETE_INTERNAL: |
+ return false; |
+ |
+ case INTERRUPTED_INTERNAL: |
+ return to == RESUMING_INTERNAL || to == CANCELLED_INTERNAL; |
+ |
+ case RESUMING_INTERNAL: |
+ return to == TARGET_PENDING_INTERNAL || to == CANCELLED_INTERNAL; |
+ |
+ case CANCELLED_INTERNAL: |
+ return false; |
+ |
+ case MAX_DOWNLOAD_INTERNAL_STATE: |
+ NOTREACHED(); |
+ } |
+ return false; |
+} |
+#endif // DCHECK_IS_ON() |
+ |
const char* DownloadItemImpl::DebugDownloadStateString( |
DownloadInternalState state) { |
switch (state) { |
+ case INITIAL_INTERNAL: |
+ return "INITIAL"; |
+ case TARGET_PENDING_INTERNAL: |
+ return "TARGET_PENDING"; |
+ case TARGET_RESOLVED_INTERNAL: |
+ return "TARGET_RESOLVED"; |
case IN_PROGRESS_INTERNAL: |
return "IN_PROGRESS"; |
case COMPLETING_INTERNAL: |