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

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // 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
 // 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
@@ skipping 62 matching lines @@
     case DownloadItem::DANGEROUS:
       return "DANGEROUS";
     case DownloadItem::DANGEROUS_BUT_VALIDATED:
       return "DANGEROUS_BUT_VALIDATED";
     default:
       NOTREACHED() << "Unknown safety state " << state;
       return "unknown";
   };
 }
 
-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
 // any operation that implies an off-thread component, since they don't
 // have any).
 class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
  public:
   NullDownloadRequestHandle() {}
 
   // DownloadRequestHandleInterface calls
@@ skipping 19 matching lines @@
 // 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
 // more negative.
 // static
 const int DownloadItem::kUninitializedHandle = 0;
 
 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
 // more negative.
 
 // Constructor for reading from the history service.
 DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    DownloadId download_id,
                                    const DownloadPersistentStoreInfo& info,
                                    const net::BoundNetLog& bound_net_log)
     : download_id_(download_id),
       current_path_(info.path),
       target_path_(info.path),
       target_disposition_(TARGET_DISPOSITION_OVERWRITE),
       url_chain_(1, info.url),
       referrer_url_(info.referrer_url),
       transition_type_(content::PAGE_TRANSITION_LINK),
       has_user_gesture_(false),
       total_bytes_(info.total_bytes),
       received_bytes_(info.received_bytes),
       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),
       db_handle_(info.db_handle),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
       is_persisted_(true),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(info.opened),
       open_enabled_(true),
       delegate_delayed_complete_(false),
       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);
 }
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(
     DownloadItemImplDelegate* delegate,
     const DownloadCreateInfo& info,
     scoped_ptr<DownloadRequestHandleInterface> request_handle,
@@ skipping 12 matching lines @@
       content_disposition_(info.content_disposition),
       mime_type_(info.mime_type),
       original_mime_type_(info.original_mime_type),
       referrer_charset_(info.referrer_charset),
       remote_address_(info.remote_address),
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       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),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
       is_persisted_(false),
@@ skipping 34 matching lines @@
       referrer_url_(GURL()),
       transition_type_(content::PAGE_TRANSITION_LINK),
       has_user_gesture_(false),
       mime_type_(mime_type),
       original_mime_type_(mime_type),
       total_bytes_(0),
       received_bytes_(0),
       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),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
       is_persisted_(false),
@@ skipping 50 matching lines @@
                  GetDangerType(), GetSafetyState()));
 
   UpdateObservers();
 
   delegate_->MaybeCompleteDownload(this);
 }
 
 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
     request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   last_reason_ = user_cancel ?
       content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED :
       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;
   }
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  TransitionTo(CANCELLED);
+  TransitionTo(CANCELLED_INTERNAL);
   if (user_cancel)
     delegate_->DownloadStopped(this);
 }
 
 void DownloadItemImpl::Delete(DeleteReason reason) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
       UMA_HISTOGRAM_ENUMERATION(
@@ skipping 25 matching lines @@
   delegate_->AssertStateConsistent(this);
 
   NotifyRemoved();
   delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
 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())
       open_when_complete_ = !open_when_complete_;
     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
   // don't generally have the proper interface for that to the external
   // program that opens the file.  So instead we spawn a check to update
   // the UI if the file has been deleted in parallel with the open.
   delegate_->CheckForFileRemoval(this);
   download_stats::RecordOpen(GetEndTime(), !GetOpened());
   opened_ = true;
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
@@ skipping 19 matching lines @@
 
 DownloadId DownloadItemImpl::GetGlobalId() const {
   return download_id_;
 }
 
 int64 DownloadItemImpl::GetDbHandle() const {
   return db_handle_;
 }
 
 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 {
   return last_reason_;
 }
 
 bool DownloadItemImpl::IsPaused() const {
   return is_paused_;
 }
 
 bool DownloadItemImpl::IsTemporary() const {
   return is_temporary_;
 }
 
 bool DownloadItemImpl::IsPersisted() const {
   return is_persisted_;
 }
 
 // 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 {
   return url_chain_.empty() ?
              GURL::EmptyGURL() : url_chain_.back();
 }
 
 const std::vector<GURL>& DownloadItemImpl::GetUrlChain() const {
   return url_chain_;
 }
@@ skipping 152 matching lines @@
 
 base::Time DownloadItemImpl::GetStartTime() const {
   return start_time_;
 }
 
 base::Time DownloadItemImpl::GetEndTime() const {
   return end_time_;
 }
 
 bool DownloadItemImpl::CanShowInFolder() {
-  return !IsCancelled() && !file_externally_removed_;
+  return state_ != CANCELLED_INTERNAL && !file_externally_removed_;
 }
 
 bool DownloadItemImpl::CanOpenDownload() {
   return !file_externally_removed_;
 }
 
 bool DownloadItemImpl::ShouldOpenFileBasedOnExtension() {
   return delegate_->ShouldOpenFileBasedOnExtension(GetUserVerifiedFilePath());
 }
 
@@ skipping 59 matching lines @@
 
 void DownloadItemImpl::SetDisplayName(const FilePath& name) {
   display_name_ = name;
 }
 
 std::string DownloadItemImpl::DebugString(bool verbose) const {
   std::string description =
       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>");
   if (!url_chain_.empty()) {
     std::vector<GURL>::const_iterator iter = url_chain_.begin();
     std::vector<GURL>::const_iterator last = url_chain_.end();
     url_list = (*iter).spec();
     ++iter;
     for ( ; verbose && (iter != last); ++iter) {
       url_list += " ->\n\t";
@@ skipping 63 matching lines @@
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
   // interrupts from the file thread Renames and the generation of
   // interrupts from disk writes go through two different mechanisms (driven
   // by rename requests from UI thread and by write requests from IO thread,
   // 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.
 
   // 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);
 }
 
 void DownloadItemImpl::SetTotalBytes(int64 total_bytes) {
   total_bytes_ = total_bytes;
 }
 
 // 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.
 void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
                                       int64 bytes_per_sec,
                                       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.
     //
     // 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;
@@ skipping 24 matching lines @@
   all_data_saved_ = true;
   ProgressComplete(size, final_hash);
   UpdateObservers();
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
 }
 
 void DownloadItemImpl::SetIsPersisted() {
   is_persisted_ = true;
   UpdateObservers();
 }
 
 void DownloadItemImpl::SetDbHandle(int64 handle) {
   db_handle_ = handle;
 
@@ skipping 64 matching lines @@
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
   // TODO(asanka): SetDangerType() doesn't need to send a notification here.
 
   // We want the intermediate and target paths to refer to the same directory so
   // that they are both on the same device and subject to same
   // 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
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
   DownloadFileManager::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ skipping 19 matching lines @@
 void DownloadItemImpl::MaybeCompleteDownload() {
   // TODO(rdsmith): Move logic for this function here.
   delegate_->MaybeCompleteDownload(this);
 }
 
 // Called by DownloadManagerImpl::MaybeCompleteDownload() when it has
 // determined that the download is ready for completion.
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (!IsInProgress())
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetFilePath().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
 
   if (NeedsRename()) {
     DownloadFileManager::RenameCompletionCallback callback =
         base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
                    weak_ptr_factory_.GetWeakPtr());
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::RenameDownloadFile,
                    delegate_->GetDownloadFileManager(), GetGlobalId(),
                    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);
   }
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
     content::DownloadInterruptReason reason,
     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()
            << " " << DebugString(false);
   DCHECK(NeedsRename());
 
   if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     return;
   }
 
   // full_path is now the current and target file path.
   DCHECK(!full_path.empty());
   target_path_ = full_path;
   SetFullPath(full_path);
   delegate_->DownloadRenamedToFinalName(this);
 
   // 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);
 }
 
 void DownloadItemImpl::OnDownloadFileReleased() {
   if (delegate_->ShouldOpenDownload(this))
     Completed();
   else
     delegate_delayed_complete_ = true;
 }
 
 void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
   auto_opened_ = auto_opened;
   Completed();
 }
 
 void DownloadItemImpl::Completed() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
   delegate_->DownloadCompleted(this);
   download_stats::RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
     // If it was already handled by the delegate, do nothing.
   } else if (GetOpenWhenComplete() ||
              ShouldOpenFileBasedOnExtension() ||
              IsTemporary()) {
     // If the download is temporary, like in drag-and-drop, do not open it but
     // we still need to set it auto-opened so that it can be removed from the
@@ skipping 21 matching lines @@
   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(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,
                      received_bytes_, &hash_state_));
       break;
     default:
       break;
   }
 
   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);
 }
 
 void DownloadItemImpl::SetDangerType(content::DownloadDangerType danger_type) {
   danger_type_ = danger_type;
   // Notify observers if the safety state has changed as a result of the new
   // danger type.
   SafetyState updated_value = IsDangerous() ?
       DownloadItem::DANGEROUS : DownloadItem::SAFE;
@@ skipping 13 matching lines @@
            << " " << DebugString(true);
   DCHECK(!new_path.empty());
   current_path_ = new_path;
 
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_RENAMED,
       base::Bind(&download_net_logs::ItemRenamedCallback,
                  &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.