Shift "commit point" for when a download will no longer accept cancels.

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 ea39f32a214641d17265bc73a4104ba0f7c1ecf6..adc4bbc55c112cb1e89a0d5d3ddd7dab6cf068a0 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -80,22 +80,6 @@ const char* DebugSafetyStateString(DownloadItem::SafetyState state) {
   };
 }
 
-const char* DebugDownloadStateString(DownloadItem::DownloadState state) {
-  switch (state) {
-    case DownloadItem::IN_PROGRESS:
-      return "IN_PROGRESS";
-    case DownloadItem::COMPLETE:
-      return "COMPLETE";
-    case DownloadItem::CANCELLED:
-      return "CANCELLED";
-    case DownloadItem::INTERRUPTED:
-      return "INTERRUPTED";
-    default:
-      NOTREACHED() << "Unknown download state " << state;
-      return "unknown";
-  };
-}
-
 // Classes to null out request handle calls (for SavePage DownloadItems, which
 // may have, e.g., Cancel() called on them without it doing anything)
 // and to DCHECK on them (for history DownloadItems, which should never have
@@ -135,6 +119,23 @@ const char DownloadItem::kEmptyFileHash[] = "";
 
 }
 
+const content::DownloadItem::DownloadState
+DownloadItemImpl::kInternalStateMap[] = {
+  content::DownloadItem::IN_PROGRESS,
+  content::DownloadItem::IN_PROGRESS,
+  content::DownloadItem::COMPLETE,
+  content::DownloadItem::CANCELLED,
+  content::DownloadItem::INTERRUPTED,
+};
+
+const DownloadItemImpl::DownloadInternalState
+DownloadItemImpl::kExternalStateMap[] = {
+  DownloadItemImpl::IN_PROGRESS_INTERNAL,
+  DownloadItemImpl::COMPLETE_INTERNAL,
+  DownloadItemImpl::CANCELLED_INTERNAL,
+  DownloadItemImpl::INTERRUPTED_INTERNAL,
+};
+
 // Our download table ID starts at 1, so we use 0 to represent a download that
 // has started, but has not yet had its data persisted in the table. We use fake
 // database handles in incognito mode starting at -1 and progressively getting
@@ -158,7 +159,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks()),
-      state_(info.state),
+      state_(kExternalStateMap[info.state]),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       end_time_(info.end_time),
@@ -178,9 +179,9 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
-  if (IsInProgress())
-    state_ = CANCELLED;
-  if (IsComplete())
+  if (state_ == IN_PROGRESS_INTERNAL)
+    state_ = CANCELLED_INTERNAL;
+  if (state_ == COMPLETE_INTERNAL)
     all_data_saved_ = true;
   Init(false /* not actively downloading */,
        download_net_logs::SRC_HISTORY_IMPORT);
@@ -213,7 +214,7 @@ DownloadItemImpl::DownloadItemImpl(
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
+      state_(kExternalStateMap[IN_PROGRESS]),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       db_handle_(DownloadItem::kUninitializedHandle),
@@ -268,7 +269,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
+      state_(kExternalStateMap[IN_PROGRESS]),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(base::Time::Now()),
       db_handle_(DownloadItem::kUninitializedHandle),
@@ -339,7 +340,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
 void DownloadItemImpl::TogglePause() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DCHECK(IsInProgress());
+  DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == COMPLETING_INTERNAL);
   if (is_paused_)
     request_handle_->ResumeRequest();
   else
@@ -356,7 +357,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
       content::DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
 
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (!IsPartialDownload()) {
+  if (state_ != IN_PROGRESS_INTERNAL) {
     // Small downloads might be complete before this method has
     // a chance to run.
     return;
@@ -364,7 +365,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  TransitionTo(CANCELLED);
+  TransitionTo(CANCELLED_INTERNAL);
   if (user_cancel)
     delegate_->DownloadStopped(this);
 }
@@ -410,7 +411,7 @@ void DownloadItemImpl::Remove() {
 void DownloadItemImpl::OpenDownload() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (IsPartialDownload()) {
+  if (state_ == IN_PROGRESS_INTERNAL) {
     // We don't honor the open_when_complete_ flag for temporary
     // downloads. Don't set it because it shows up in the UI.
     if (!IsTemporary())
@@ -418,7 +419,7 @@ void DownloadItemImpl::OpenDownload() {
     return;
   }
 
-  if (!IsComplete() || file_externally_removed_)
+  if (!state_ == COMPLETE_INTERNAL || file_externally_removed_)

[benjhayden, 2012/09/19 15:44:35]

s/!state_ ==/state_ !=/

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Whooops.  Done.

     return;
 
   // Ideally, we want to detect errors in opening and report them, but we
@@ -458,7 +459,14 @@ int64 DownloadItemImpl::GetDbHandle() const {
 }
 
 DownloadItem::DownloadState DownloadItemImpl::GetState() const {
-  return state_;
+  // Make sure the mapping arrays satisfy constraints.
+  COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kInternalStateMap) ==

[benjhayden, 2012/09/19 15:44:35]

Could you put these up by the maps themselves?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

I'd like to, but they need to be in the DownloadItemImpl namespace to get access
to the list of internal states (it's private).  I didn't want to create a
function purely to do this, and this function seemed like the most appropriate
one to stick them into.   If you'd like me to put them in the first constructor,
I'm willing, but they'll still be a ways separated from the maps because of all
of the member variables we initialize.

+                 MAX_DOWNLOAD_INTERNAL_STATE,
+                 internal_state_map_conflicts_with_states);
+  COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kExternalStateMap) == MAX_DOWNLOAD_STATE,
+                 external_state_map_conflicts_with_states);
+
+  return kInternalStateMap[state_];
 }
 
 content::DownloadInterruptReason DownloadItemImpl::GetLastReason() const {
@@ -480,24 +488,23 @@ bool DownloadItemImpl::IsPersisted() const {
 // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to
 // |IsPartialDownload()|, when resuming interrupted downloads is implemented.
 bool DownloadItemImpl::IsPartialDownload() const {
-  return (state_ == IN_PROGRESS);
+  return (state_ == IN_PROGRESS_INTERNAL);

[benjhayden, 2012/09/19 15:44:35]

Have you inspected all callers of IsPartialDownload() to make sure that they
don't actually mean IsInProgress()?
And vice versa?
Could you remove IsPartialDownload() in this CL?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Argggh.  You triggerred my "not conceptually part of this CL, so I'd rather put
it off" subroutine, then I realized that this arguably *is* part of this CL.  I
think I'm going to say: There's a bug in this code in that it's not
functionality preserving, and I'm going to fix that (by making it check
COMPLETING as well), but that makes the definition false to the name of the
routine.  OTOH, the definition *is* false to the name of the routine anyway,
since we can finish downloading the data well before we switch out of
IN_PROGRESS (depending on user response).  On the third hand, the intended use
for IsPartialDownload() was to eventually return true for interrupted downloads
that could be resumed.  On the fourth hand, I'm inclined to get rid of *all* of
these probes and have consumers just check the state variable for what they want
to know anyway, but I haven't solidly come to that decision yet.

So: I'd rather not tug on this string (other than checking COMPLETING) at this
point.  That ok by you?

 }
 
 bool DownloadItemImpl::IsInProgress() const {
-  return (state_ == IN_PROGRESS);
+  return (state_ == IN_PROGRESS_INTERNAL || state_ == COMPLETING_INTERNAL);
 }
 
 bool DownloadItemImpl::IsCancelled() const {
-  return (state_ == CANCELLED) ||
-         (state_ == INTERRUPTED);
+  return (state_ == CANCELLED_INTERNAL || state_ == INTERRUPTED_INTERNAL);
 }
 
 bool DownloadItemImpl::IsInterrupted() const {
-  return (state_ == INTERRUPTED);
+  return (state_ == INTERRUPTED_INTERNAL);
 }
 
 bool DownloadItemImpl::IsComplete() const {
-  return (state_ == COMPLETE);
+  return (state_ == COMPLETE_INTERNAL);
 }
 
 const GURL& DownloadItemImpl::GetURL() const {
@@ -670,7 +677,7 @@ base::Time DownloadItemImpl::GetEndTime() const {
 }
 
 bool DownloadItemImpl::CanShowInFolder() {
-  return !IsCancelled() && !file_externally_removed_;
+  return state_ != CANCELLED_INTERNAL && !file_externally_removed_;
 }
 
 bool DownloadItemImpl::CanOpenDownload() {
@@ -750,7 +757,7 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
       base::StringPrintf("{ id = %d"
                          " state = %s",
                          download_id_.local(),
-                         DebugDownloadStateString(GetState()));
+                         DebugDownloadStateString(state_));
 
   // Construct a string of the URL chain.
   std::string url_list("<none>");
@@ -834,11 +841,11 @@ void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // interrupts to race with cancels.
 
   // Whatever happens, the first one to hit the UI thread wins.
-  if (!IsInProgress())
+  if (state_ != IN_PROGRESS_INTERNAL)
     return;
 
   last_reason_ = reason;
-  TransitionTo(INTERRUPTED);
+  TransitionTo(INTERRUPTED_INTERNAL);
   download_stats::RecordDownloadInterrupted(
       reason, received_bytes_, total_bytes_);
   delegate_->DownloadStopped(this);
@@ -856,7 +863,7 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
                                       const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (!IsInProgress()) {
+  if (state_ != IN_PROGRESS_INTERNAL) {
     // 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.
     //
@@ -901,7 +908,7 @@ void DownloadItemImpl::MarkAsComplete() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
 }
 
 void DownloadItemImpl::SetIsPersisted() {
@@ -986,6 +993,16 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
+  if (!IsInProgress()) {

[benjhayden, 2012/09/19 15:44:35]

Why respell some IsInProgress()/IsComplete()/etc. calls using state_ directly,
but not all calls?
I'd prefer using state_ directly whenever possible for clarity.

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Agreed.  I had thought I had replaced them all (I consider this respecting the
interface boundary--if DownloadItemImpl can't understand what it's own states
mean, we have a problem).  Thanks for calling this out.

+    // 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

[benjhayden, 2012/09/19 15:44:35]

s/stoppaged/stoppage/

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Done.

+    // will already have been recorded.

[benjhayden, 2012/09/19 15:44:35]

Could you convert "the cause of download stoppage" to a DownloadInterruptReason
with more fidelity?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

I'm slightly confused by your question.  My comment was intended to mean
"Whatever I pass to ODRTIN will be dropped on the floor, so it doesn't matter
what I pass".  But no reason not to pass the actual reason, since it's in
last_reason_.  Done.

+    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
@@ -1025,6 +1042,9 @@ void DownloadItemImpl::MaybeCompleteDownload() {
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
@@ -1048,6 +1068,7 @@ void DownloadItemImpl::OnDownloadCompleting() {
                    delegate_->GetDownloadFileManager(), GetGlobalId(),
                    base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                               weak_ptr_factory_.GetWeakPtr())));
+    TransitionTo(COMPLETING_INTERNAL);
   }
 }
 
@@ -1056,6 +1077,9 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (!IsInProgress())

[benjhayden, 2012/09/19 15:44:35]

So, if I cancel a download *while* it's being renamed, current_path_ won't be
updated. Chrome will lose the file, and think it was externally removed.
Am I reading this right? Is that what you meant to do?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Huh.  Good point; at a minimum we should have a comment here.  But I don't think
it matters.  If we're canceled at any time before we hit the commit point, we're
going to send a Cancel message over to the file thread, which will delete the
file.  So I don't think we care that we're not updating the file name.

Comment added.

+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
@@ -1080,6 +1104,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
                  delegate_->GetDownloadFileManager(), GetGlobalId(),
                  base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                             weak_ptr_factory_.GetWeakPtr())));
+  TransitionTo(COMPLETING_INTERNAL);
 }
 
 void DownloadItemImpl::OnDownloadFileReleased() {
@@ -1101,7 +1126,7 @@ void DownloadItemImpl::Completed() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
   delegate_->DownloadCompleted(this);
   download_stats::RecordDownloadCompleted(start_tick_, received_bytes_);
 
@@ -1143,27 +1168,33 @@ void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
     total_bytes_ = 0;
 }
 
-void DownloadItemImpl::TransitionTo(DownloadState new_state) {
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   if (state_ == new_state)
     return;
 
-  DownloadState old_state = state_;
+  DownloadInternalState old_state = state_;
   state_ = new_state;
 
   switch (state_) {
-    case COMPLETE:
+    case COMPLETING_INTERNAL:
+      bound_net_log_.AddEvent(
+          net::NetLog::TYPE_DOWNLOAD_ITEM_COMPLETING,
+          base::Bind(&download_net_logs::ItemCompletingCallback,
+                     received_bytes_, &hash_));
+      break;
+    case COMPLETE_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_FINISHED,
           base::Bind(&download_net_logs::ItemFinishedCallback,
-                     received_bytes_, &hash_));
+                     auto_opened_));
       break;
-    case INTERRUPTED:
+    case INTERRUPTED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_INTERRUPTED,
           base::Bind(&download_net_logs::ItemInterruptedCallback,
                      last_reason_, received_bytes_, &hash_state_));
       break;
-    case CANCELLED:
+    case CANCELLED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_CANCELED,
           base::Bind(&download_net_logs::ItemCanceledCallback,
@@ -1175,10 +1206,14 @@ void DownloadItemImpl::TransitionTo(DownloadState new_state) {
 
   VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
 
-  UpdateObservers();
+  // Only update observers on user visible state changes.
+  if (kInternalStateMap[old_state] != kInternalStateMap[state_])

[benjhayden, 2012/09/19 15:44:35]

Should you use the state maps in IsInProgress()/IsComplete()/etc. as well as
here so that they stay in sync?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

Could you put a few more words behind your argument?  I feel like they're doing
somewhat different things.  Or are you saying that they're both expressing the
state that should be exposed to the user, and if I made the checks in Is* use
the external states rather than the internals states, that would mean I don't
"leak" state I don't mean the users to see?  I agree with that argument
(although it strengthens my interest in getting rid of the Is* probes).  

Done.

+    UpdateObservers();
 
-  bool is_done = (state_ != IN_PROGRESS);
-  bool was_done = (old_state != IN_PROGRESS);
+  bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
+                  state_ != COMPLETING_INTERNAL);
+  bool was_done = (old_state != IN_PROGRESS_INTERNAL &&
+                   old_state != COMPLETING_INTERNAL);
   if (is_done && !was_done)
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
 }
@@ -1212,6 +1247,26 @@ void DownloadItemImpl::SetFullPath(const FilePath& new_path) {
                  &current_path_, &new_path));
 }
 
+// static

[benjhayden, 2012/09/19 15:44:35]

What's the difference between a private static method and a function in the
anonymous namespace?
If a static method is private, does it make a noise?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

I originally followed your suggestion and took it out, and the compiler was kind
enough to remind me why it's a static private: So that it has visibility into
the private typedef of DownloadInternalState.  I suppose I could just make it a
friend, but I'd rather make it a static private.
Only when you try to take it out of its class :-}.

+const char* DownloadItemImpl::DebugDownloadStateString(
+    DownloadInternalState state) {
+  switch (state) {
+    case IN_PROGRESS_INTERNAL:
+      return "IN_PROGRESS";
+    case COMPLETING_INTERNAL:
+      return "COMPLETING";
+    case COMPLETE_INTERNAL:
+      return "COMPLETE";
+    case CANCELLED_INTERNAL:
+      return "CANCELLED";
+    case INTERRUPTED_INTERNAL:
+      return "INTERRUPTED";
+    default:
+      NOTREACHED() << "Unknown download state " << state;
+      return "unknown";
+  };
+}
+

[benjhayden, 2012/09/19 15:44:35]

Mind deleting all these blank lines?

[Randy Smith (Not in Mondays), 2012/09/19 20:46:39]

I'll have you know, I have a deep, sentimental attachment to these blank lines!!
 We've been through a lot together!

(It can't be alcohol; I've haven't drunk today.  Lack of alcohol?)

Done.