[Downloads] Move UpdateObservers out of TransitionTo in DownloadItemImpl

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
 // interface Start is 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 this file.
@@ skipping 386 matching lines @@
 
   // 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(&DeleteDownloadedFile, current_path_));
     current_path_.clear();
   }
 
-  TransitionTo(CANCELLED_INTERNAL);
+  TransitionTo(CANCELLED_INTERNAL, UPDATE_OBSERVERS);
 }
 
 void DownloadItemImpl::Remove() {
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   delegate_->AssertStateConsistent(this);
   Cancel(true);
   delegate_->AssertStateConsistent(this);
 
@@ skipping 495 matching lines @@
   hash_state_ = "";
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
            << " per_sec=" << bytes_per_sec << " download=" << DebugString(true);
 
   if (GetState() != IN_PROGRESS) {
     // 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.
@@ skipping 92 matching lines @@
   request_handle_ = req_handle.Pass();
 
   if (GetState() == CANCELLED) {
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
     request_handle_->CancelRequest();
     return;
   }
 
-  TransitionTo(IN_PROGRESS_INTERNAL);
-
-  last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+  TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFile::Initialize,
                  // Safe because we control download file lifetime.
                  base::Unretained(download_file_.get()),
                  base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
                             weak_ptr_factory_.GetWeakPtr())));
 }
 
@@ skipping 41 matching lines @@
   // UI) or automatic.  If we keep the "ignore an error on download until file
   // name determination complete" semantics, we need to make sure that the
   // error is kept completely invisible until that point.
 
   VLOG(20) << __FUNCTION__ << " " << target_path.value() << " " << disposition
            << " " << danger_type << " " << DebugString(true);
 
   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());
 
   // During resumption, we may choose to proceed with the same intermediate
   // file. No rename is necessary if our intermediate file already has the
   // correct name.
   //
@@ skipping 39 matching lines @@
     Interrupt(destination_error_);
     destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
   } else if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     // All file errors result in file deletion above; no need to cleanup.  The
     // current_path_ should be empty. Resuming this download will force a
     // restart and a re-doing of filename determination.
     DCHECK(current_path_.empty());
   } else {
     SetFullPath(full_path);
+    UpdateObservers();
     MaybeCompleteDownload();
   }
 }
 
 // When SavePackage downloads MHTML to GData (see
 // SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
 // does for non-SavePackage downloads, but SavePackage downloads never satisfy
 // IsDownloadReadyForCompletion(). GDataDownloadObserver manually calls
 // DownloadItem::UpdateObservers() when the upload completes so that SavePackage
 // notices that the upload has completed and runs its normal Finish() pathway.
@@ skipping 93 matching lines @@
   }
 
   // Complete the download and release the DownloadFile.
   DCHECK(download_file_.get());
   ReleaseDownloadFile(false);
 
   // We're not completely done with the download item yet, but at this
   // 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,
                            weak_ptr_factory_.GetWeakPtr()))) {
     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.

   }
 }
 
 void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   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_INTERNAL);
+  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
   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
     // download shelf.
     if (!IsTemporary())
       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.

   }
 }
 
 void DownloadItemImpl::OnResumeRequestStarted(DownloadItem* item,
                                               net::Error error) {
   // If |item| is not NULL, then Start() has been called already, and nothing
   // more needs to be done here.
   if (item) {
     DCHECK_EQ(net::OK, error);
     DCHECK_EQ(static_cast<DownloadItem*>(this), item);
@@ skipping 24 matching lines @@
   // 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 (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)
     return;
 
   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.
   // 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;
 
-  // 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (destroy_file) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         // Will be deleted at end of task execution.
         base::Bind(&DownloadFileCancel, base::Passed(&download_file_)));
@@ skipping 42 matching lines @@
     return false;
 
   // 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.
   if (!delegate_->ShouldCompleteDownload(this, state_change_notification))
     return false;
 
   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)
     return;
 
   DownloadInternalState old_state = state_;
   state_ = new_state;
 
   switch (state_) {
     case COMPLETING_INTERNAL:
@@ skipping 27 matching lines @@
                      &hash_state_));
       break;
     default:
       break;
   }
 
   VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true)
     << " " << 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 &&
                    old_state != COMPLETING_INTERNAL);
   // Termination
   if (is_done && !was_done)
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
 
   // Resumption
   if (was_done && !is_done) {
     std::string file_name(target_path_.BaseName().AsUTF8Unsafe());
     bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE,
                               base::Bind(&ItemActivatedNetLogCallback,
                                          this, SRC_ACTIVE_DOWNLOAD,
                                          &file_name));
   }
+
+  if (notify_action == UPDATE_OBSERVERS)
+    UpdateObservers();
 }
 
 void DownloadItemImpl::SetDangerType(DownloadDangerType danger_type) {
   if (danger_type != danger_type_) {
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
         base::Bind(&ItemCheckedNetLogCallback, danger_type));
   }
   danger_type_ = danger_type;
 }
 
 void DownloadItemImpl::SetFullPath(const base::FilePath& new_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << "()"
            << " new_path = \"" << new_path.value() << "\""
            << " " << DebugString(true);
   DCHECK(!new_path.empty());
 
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_RENAMED,
       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() {
   DVLOG(20) << __FUNCTION__ << "() " << DebugString(true);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   ResumeMode mode = GetResumeMode();
 
   if (mode != RESUME_MODE_IMMEDIATE_RESTART &&
       mode != RESUME_MODE_IMMEDIATE_CONTINUE) {
     return;
@@ skipping 44 matching lines @@
   download_params->set_last_modified(GetLastModifiedTime());
   download_params->set_etag(GetETag());
   download_params->set_callback(
       base::Bind(&DownloadItemImpl::OnResumeRequestStarted,
                  weak_ptr_factory_.GetWeakPtr()));
 
   delegate_->ResumeInterruptedDownload(download_params.Pass(), GetGlobalId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
 
-  TransitionTo(RESUMING_INTERNAL);
+  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
 }
 
 // static
 DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
     DownloadInternalState internal_state) {
   switch (internal_state) {
     case IN_PROGRESS_INTERNAL:
       return IN_PROGRESS;
     case COMPLETING_INTERNAL:
       return IN_PROGRESS;
@@ skipping 64 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content