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[] = {

[asanka, 2012/09/20 18:42:15]

A minor suggestion: Perhaps a switch based mapping function would be easier to
read?

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

Good idea.  I think it removes the pain around the COMPILE_ASSERT positioning
too (by removing the asserts).

+  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_)
     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) ==

[asanka, 2012/09/20 18:42:15]

Nit: Perhaps keep the COMPILE_ASSERTs next to the array definitions?

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

See above; now moot.

+                 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 kInternalStateMap[state_] == IN_PROGRESS;
 }
 
 bool DownloadItemImpl::IsInProgress() const {
-  return (state_ == IN_PROGRESS);
+  return kInternalStateMap[state_] == IN_PROGRESS;
 }
 
 bool DownloadItemImpl::IsCancelled() const {
-  return (state_ == CANCELLED) ||
-         (state_ == INTERRUPTED);
+  DownloadState external_state = kInternalStateMap[state_];
+  return  external_state == CANCELLED || external_state == INTERRUPTED;
 }
 
 bool DownloadItemImpl::IsInterrupted() const {
-  return (state_ == INTERRUPTED);
+  return kInternalStateMap[state_] == INTERRUPTED;
 }
 
 bool DownloadItemImpl::IsComplete() const {
-  return (state_ == COMPLETE);
+  return kInternalStateMap[state_] == COMPLETE;
 }
 
 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 (state_ != IN_PROGRESS_INTERNAL) {
+    // 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 stoppage
+    // will already have been recorded and will be retained, but we use
+    // whatever was recorded for consistency.
+    OnDownloadRenamedToIntermediateName(last_reason_, 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 (state_ != IN_PROGRESS_INTERNAL)
+    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 a cancel or interrupt hit, we'll cancel the DownloadFile, which
+  // will result in deleting the file on the file thread.  So we don't
+  // care about the name having been changed.
+  if (state_ != IN_PROGRESS_INTERNAL)
+    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_])
+    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));
 }
 
-
-
-
+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";
+  };
+}