[Downloads] Move UpdateObservers out of TransitionTo in DownloadItemImpl

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 9b6b51637677c663957783b67abac46f46355138..bf190bd412477b3e264133a5f42c4d22977a0c6d 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -404,7 +404,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
     current_path_.clear();
   }
 
-  TransitionTo(CANCELLED_INTERNAL);
+  TransitionTo(CANCELLED_INTERNAL, UPDATE_OBSERVERS);
 }
 
 void DownloadItemImpl::Remove() {
@@ -920,8 +920,9 @@ void DownloadItemImpl::MarkAsComplete() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL);
+  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
 }
+
 void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
                                          int64 bytes_per_sec,
                                          const std::string& hash_state) {
@@ -1034,9 +1035,7 @@ void DownloadItemImpl::Start(
     return;
   }
 
-  TransitionTo(IN_PROGRESS_INTERNAL);
-
-  last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+  TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -1098,7 +1097,6 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   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
@@ -1158,6 +1156,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DCHECK(current_path_.empty());
   } else {
     SetFullPath(full_path);
+    UpdateObservers();
     MaybeCompleteDownload();
   }
 }
@@ -1271,7 +1270,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   // point we're committed to complete the download.  Cancels (or Interrupts,
   // though it's not clear how they could happen) after this point will be
   // ignored.
-  TransitionTo(COMPLETING_INTERNAL);
+  TransitionTo(COMPLETING_INTERNAL, DONT_UPDATE_OBSERVERS);
 
   if (delegate_->ShouldOpenDownload(
           this, base::Bind(&DownloadItemImpl::DelayedDownloadOpened,
@@ -1279,6 +1278,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
     Completed();
   } else {
     delegate_delayed_complete_ = true;
+    UpdateObservers();

[Randy Smith (Not in Mondays), 2013/06/21 18:31:28]

What are we letting the observers know about here?

[asanka, 2013/06/21 20:30:56]

I removed the UpdateObservers() call in SetFullPath(). So if we passed through
the SetFullPath() call above and get here, we haven't updated the observers
regarding the new path.

   }
 }
 
@@ -1296,7 +1296,7 @@ void DownloadItemImpl::Completed() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL);
+  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
   RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
@@ -1311,7 +1311,6 @@ void DownloadItemImpl::Completed() {
       OpenDownload();
 
     auto_opened_ = true;
-    UpdateObservers();

[Randy Smith (Not in Mondays), 2013/06/21 18:31:28]

I think this was for the auto_opened_ transition; does it make sense to remove
it?

[asanka, 2013/06/21 20:30:56]

Nope. Restored.

   }
 }
 
@@ -1356,12 +1355,20 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   last_reason_ = reason;
 
   ResumeMode resume_mode = GetResumeMode();
-  // Cancel (delete file) if we're going to restart; no point in leaving
-  // data around we aren't going to use.  Also cancel if resumption isn't
-  // enabled for the same reason.
-  ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
-                      resume_mode == RESUME_MODE_USER_RESTART ||
-                      !IsDownloadResumptionEnabled());
+
+  if (state_ == IN_PROGRESS_INTERNAL) {
+    // Cancel (delete file) if we're going to restart; no point in leaving
+    // data around we aren't going to use.  Also cancel if resumption isn't
+    // enabled for the same reason.
+    ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
+                        resume_mode == RESUME_MODE_USER_RESTART ||
+                        !IsDownloadResumptionEnabled());
+
+    // Cancel the originating URL request.
+    request_handle_->CancelRequest();
+  } else {
+    DCHECK(!download_file_.get());

[Randy Smith (Not in Mondays), 2013/06/21 18:31:28]

What's the status of the request handle at this point?  Is there an available
DCHECK there?

[asanka, 2013/06/21 20:30:56]

The request_handle_ may or may not be NULL. E.g. if we restore a download from
history, try to resume it, and the resumption request fails, then there would be
no request_handle. If an in-progress download is interrupted, resumed, and the
resumption request fails, then request_handle_ would be non-NULL.

I don't think we have a readily available condition that we could use to test
that if a non-NULL request_handle_ exists, that it refers to a defunct request.

+  }
 
   // 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.
@@ -1375,13 +1382,11 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // all_data_saved_ to false here.
   all_data_saved_ = false;
 
-  // Cancel the originating URL request.
-  request_handle_->CancelRequest();
-
-  TransitionTo(INTERRUPTED_INTERNAL);
+  TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
   RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
 
   AutoResumeIfValid();
+  UpdateObservers();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
@@ -1444,7 +1449,8 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion(
   return true;
 }
 
-void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
+                                    ShouldUpdateObservers notify_action) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (state_ == new_state)
@@ -1492,10 +1498,6 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
     << " " << InternalToExternalState(old_state)
     << " " << InternalToExternalState(state_);
 
-  // Only update observers on user visible state changes.
-  if (InternalToExternalState(state_) != InternalToExternalState(old_state))
-    UpdateObservers();
-
   bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
                   state_ != COMPLETING_INTERNAL);
   bool was_done = (old_state != IN_PROGRESS_INTERNAL &&
@@ -1512,6 +1514,9 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
                                          this, SRC_ACTIVE_DOWNLOAD,
                                          &file_name));
   }
+
+  if (notify_action == UPDATE_OBSERVERS)
+    UpdateObservers();
 }
 
 void DownloadItemImpl::SetDangerType(DownloadDangerType danger_type) {
@@ -1535,7 +1540,6 @@ void DownloadItemImpl::SetFullPath(const base::FilePath& new_path) {
       base::Bind(&ItemRenamedNetLogCallback, &current_path_, &new_path));
 
   current_path_ = new_path;
-  UpdateObservers();

[Randy Smith (Not in Mondays), 2013/06/21 18:31:28]

Under what circumstances would we not want to update the observers if we're
setting a full path?

[asanka, 2013/06/21 20:30:56]

Two of the three SetFullPath calls are made before a state transition. I didn't
think we need those observer updates.

 }
 
 void DownloadItemImpl::AutoResumeIfValid() {
@@ -1600,7 +1604,7 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   // Just in case we were interrupted while paused.
   is_paused_ = false;
 
-  TransitionTo(RESUMING_INTERNAL);
+  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
 }
 
 // static