Make DownloadHistory observe manager, items

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 Start, DelayedDownloadOpened, MaybeCompleteDownload, 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 26 matching lines @@
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "net/base/net_util.h"
 
 namespace content {
 namespace {
 
 static void DeleteDownloadedFile(const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Make sure we only delete files.
   if (!file_util::DirectoryExists(path))
@@ skipping 48 matching lines @@
   download_file->Detach(callback);
 }
 
 static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   download_file->Cancel();
 }
 
 }  // namespace
 
-// 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.
-// static
-const int DownloadItem::kUninitializedHandle = 0;
-
 const char DownloadItem::kEmptyFileHash[] = "";
 
-// 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 FilePath& path,
+                                   const GURL& url,
+                                   const GURL& referrer_url,
+                                   const base::Time& start_time,
+                                   const base::Time& end_time,
+                                   int64 received_bytes,
+                                   int64 total_bytes,
+                                   DownloadItem::DownloadState state,
+                                   bool opened,
                                    const net::BoundNetLog& bound_net_log)
     : is_save_package_download_(false),
       download_id_(download_id),
-      current_path_(info.path),
-      target_path_(info.path),
+      current_path_(path),
+      target_path_(path),
       target_disposition_(TARGET_DISPOSITION_OVERWRITE),
-      url_chain_(1, info.url),
-      referrer_url_(info.referrer_url),
+      url_chain_(1, url),
+      referrer_url_(referrer_url),
       transition_type_(PAGE_TRANSITION_LINK),
       has_user_gesture_(false),
-      total_bytes_(info.total_bytes),
-      received_bytes_(info.received_bytes),
+      total_bytes_(total_bytes),
+      received_bytes_(received_bytes),
       bytes_per_sec_(0),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks()),
-      state_(ExternalToInternalState(info.state)),
+      state_(ExternalToInternalState(state)),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
-      start_time_(info.start_time),
-      end_time_(info.end_time),
-      db_handle_(info.db_handle),
+      start_time_(start_time),
+      end_time_(end_time),
       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),
+      opened_(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 (state_ == IN_PROGRESS_INTERNAL)
     state_ = CANCELLED_INTERNAL;
   if (state_ == COMPLETE_INTERNAL)
     all_data_saved_ = true;
   Init(false /* not actively downloading */, SRC_HISTORY_IMPORT);
@@ skipping 22 matching lines @@
       original_mime_type_(info.original_mime_type),
       remote_address_(info.remote_address),
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       bytes_per_sec_(0),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS_INTERNAL),
       danger_type_(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),
       is_temporary_(!info.save_info->file_path.empty()),
       all_data_saved_(false),
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
   Init(true /* actively downloading */, SRC_NEW_DOWNLOAD);
 
@@ skipping 27 matching lines @@
       mime_type_(mime_type),
       original_mime_type_(mime_type),
       total_bytes_(0),
       received_bytes_(0),
       bytes_per_sec_(0),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS_INTERNAL),
       danger_type_(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),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
   Init(true /* actively downloading */, SRC_SAVE_PAGE_AS);
 }
@@ skipping 26 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
 }
 
 void DownloadItemImpl::DangerousDownloadValidated() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(IN_PROGRESS, GetState());
   DCHECK_EQ(DANGEROUS, GetSafetyState());
 
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
+
   if (GetState() != IN_PROGRESS)
     return;
 
   UMA_HISTOGRAM_ENUMERATION("Download.DangerousDownloadValidated",
                             GetDangerType(),
                             DOWNLOAD_DANGER_TYPE_MAX);
 
   safety_state_ = DANGEROUS_BUT_VALIDATED;
 
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
       base::Bind(&ItemCheckedNetLogCallback,
                  GetDangerType(), GetSafetyState()));
 
   UpdateObservers();
 
   MaybeCompleteDownload();
 }
 
 void DownloadItemImpl::TogglePause() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == COMPLETING_INTERNAL);
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   // Ignore pauses when we've passed the commit point.
   if (state_ == COMPLETING_INTERNAL)
     return;
 
   if (is_paused_)
     request_handle_->ResumeRequest();
   else
     request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
@@ skipping 105 matching lines @@
 }
 
 int32 DownloadItemImpl::GetId() const {
   return download_id_.local();
 }
 
 DownloadId DownloadItemImpl::GetGlobalId() const {
   return download_id_;
 }
 
-int64 DownloadItemImpl::GetDbHandle() const {
-  return db_handle_;
-}
-
 DownloadItem::DownloadState DownloadItemImpl::GetState() const {
   return InternalToExternalState(state_);
 }
 
 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 InternalToExternalState(state_) == IN_PROGRESS;
 }
 
 bool DownloadItemImpl::IsInProgress() const {
   return InternalToExternalState(state_) == IN_PROGRESS;
 }
 
@@ skipping 196 matching lines @@
 }
 
 bool DownloadItemImpl::GetAutoOpened() {
   return auto_opened_;
 }
 
 bool DownloadItemImpl::GetOpened() const {
   return opened_;
 }
 
-DownloadPersistentStoreInfo DownloadItemImpl::GetPersistentStoreInfo() const {
-  // TODO(asanka): Persist GetTargetFilePath() as well.
-  return DownloadPersistentStoreInfo(GetFullPath(),
-                                     GetURL(),
-                                     GetReferrerUrl(),
-                                     GetStartTime(),
-                                     GetEndTime(),
-                                     GetReceivedBytes(),
-                                     GetTotalBytes(),
-                                     GetState(),
-                                     GetDbHandle(),
-                                     GetOpened());
-}
-
 BrowserContext* DownloadItemImpl::GetBrowserContext() const {
   return delegate_->GetBrowserContext();
 }
 
 WebContents* DownloadItemImpl::GetWebContents() const {
   // TODO(rdsmith): Remove null check after removing GetWebContents() from
   // paths that might be used by DownloadItems created from history import.
   // Currently such items have null request_handle_s, where other items
   // (regular and SavePackage downloads) have actual objects off the pointer.
   if (request_handle_.get())
     return request_handle_->GetWebContents();
   return NULL;
 }
 
 void DownloadItemImpl::OnContentCheckCompleted(DownloadDangerType danger_type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(AllDataSaved());
+  VLOG(20) << __FUNCTION__ << " danger_type=" << danger_type
+           << " download=" << DebugString(true);
   SetDangerType(danger_type);
   UpdateObservers();
 }
 
 void DownloadItemImpl::SetOpenWhenComplete(bool open) {
   open_when_complete_ = open;
 }
 
 void DownloadItemImpl::SetIsTemporary(bool temporary) {
   is_temporary_ = temporary;
@@ skipping 23 matching lines @@
     ++iter;
     for ( ; verbose && (iter != last); ++iter) {
       url_list += " ->\n\t";
       const GURL& next_url = *iter;
       url_list += next_url.spec();
     }
   }
 
   if (verbose) {
     description += base::StringPrintf(
-        " db_handle = %" PRId64
         " total = %" PRId64
         " received = %" PRId64
         " reason = %s"
         " paused = %c"
         " safety = %s"
         " last_modified = '%s'"
         " etag = '%s'"
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
         " target_path = \"%" PRFilePath "\""
         " has download file = %s",
-        GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
         InterruptReasonDebugString(last_reason_).c_str(),
         IsPaused() ? 'T' : 'F',
         DebugSafetyStateString(GetSafetyState()),
         GetLastModifiedTime().c_str(),
         GetETag().c_str(),
         url_list.c_str(),
         GetFullPath().value().c_str(),
         GetTargetFilePath().value().c_str(),
@@ skipping 10 matching lines @@
 void DownloadItemImpl::MockDownloadOpenForTesting() {
   open_enabled_ = false;
 }
 
 void DownloadItemImpl::NotifyRemoved() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));
 }
 
 void DownloadItemImpl::OnDownloadedFileRemoved() {
   file_externally_removed_ = true;
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   UpdateObservers();
 }
 
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // 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,
@@ skipping 26 matching lines @@
   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));
+  VLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
+           << " per_sec=" << bytes_per_sec << " download=" << DebugString(true);
 
   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
@@ skipping 17 matching lines @@
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   // Store final hash and null out intermediate serialized hash state.
   hash_ = final_hash;
   hash_state_ = "";
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE_INTERNAL);
 }
-
-void DownloadItemImpl::SetIsPersisted() {
-  is_persisted_ = true;
-  UpdateObservers();
-}
-
-void DownloadItemImpl::SetDbHandle(int64 handle) {
-  db_handle_ = handle;
-
-  bound_net_log_.AddEvent(
-      net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
-      net::NetLog::Int64Callback("db_handle", db_handle_));
-}
-
 void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
                                          int64 bytes_per_sec,
                                          const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   if (!IsInProgress()) {
     // 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
@@ skipping 13 matching lines @@
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
         net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
   }
 
   UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
   // The DestinationError and Interrupt routines are being kept separate
-  // to allow for a future merging of the Cancel and Interrupt routines..
+  // to allow for a future merging of the Cancel and Interrupt routines.
   Interrupt(reason);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   if (!IsInProgress())
     return;
   OnAllDataSaved(final_hash);
   MaybeCompleteDownload();
 }
 
 // **** Download progression cascade
 
 void DownloadItemImpl::Init(bool active,
                             DownloadType download_type) {
@@ skipping 12 matching lines @@
     // See if it's set programmatically.
     file_name = forced_file_path_.AsUTF8Unsafe();
     // Possibly has a 'download' attribute for the anchor.
     if (file_name.empty())
       file_name = suggested_filename_;
     // From the URL file name.
     if (file_name.empty())
       file_name = GetURL().ExtractFileName();
   }
 
-  bound_net_log_.BeginEvent(
-      net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE,
-      base::Bind(&ItemActivatedNetLogCallback,
-                 this, download_type, &file_name));
-
-  // If this is not an active download, end the ACTIVE event now.
-  if (!active) {
+  base::Callback<base::Value*(net::NetLog::LogLevel)> active_data = base::Bind(
+      &ItemActivatedNetLogCallback, this, download_type, &file_name);
+  if (active) {
+    bound_net_log_.BeginEvent(
+        net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
+  } else {
     bound_net_log_.AddEvent(
-        net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
-        net::NetLog::Int64Callback("db_handle", db_handle_));
-
-    bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
+        net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
   }
 
   VLOG(20) << __FUNCTION__ << "() " << DebugString(true);
 }
 
 // We're starting the download.
 void DownloadItemImpl::Start(scoped_ptr<DownloadFile> file) {
   DCHECK(!download_file_.get());
   DCHECK(file.get());
   download_file_ = file.Pass();
@@ skipping 34 matching lines @@
     const FilePath& intermediate_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // If the |target_path| is empty, then we consider this download to be
   // canceled.
   if (target_path.empty()) {
     Cancel(true);
     return;
   }
 
+  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());
 
@@ skipping 22 matching lines @@
       base::Bind(&DownloadFile::Rename,
                  // Safe because we control download file lifetime.
                  base::Unretained(download_file_.get()),
                  intermediate_path, false, callback));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
   } else {
     SetFullPath(full_path);
-    UpdateObservers();
+    delegate_->ShowDownloadInBrowser(this);
   }
 
-  delegate_->DownloadRenamedToIntermediateName(this);
+  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.
 // MaybeCompleteDownload() is never the mechanism by which SavePackage completes
 // downloads. SavePackage always uses its own Finish() to mark downloads
 // complete.
 void DownloadItemImpl::MaybeCompleteDownload() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsDownloadReadyForCompletion())
     return;
 
   // TODO(rdsmith): DCHECK that we only pass through this point
   // once per download.  The natural way to do this is by a state
   // transition on the DownloadItem.
 
   // Confirm we're in the proper set of states to be here;
   // have all data, have a history handle, (validated or safe).
   DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK_NE(DownloadItem::DANGEROUS, GetSafetyState());
   DCHECK(all_data_saved_);
-  DCHECK(is_persisted_);
-
-  delegate_->UpdatePersistence(this);
 
   OnDownloadCompleting();
 }
 
 // Called by 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)
@@ skipping 57 matching lines @@
 
   if (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);
 
   ReleaseDownloadFile();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile() {
   // Complete the download and release the DownloadFile.
   DCHECK(!is_save_package_download_);
   DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ skipping 60 matching lines @@
   // has been released.
   if (!is_save_package_download_ && download_file_.get()) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         // Will be deleted at end of task execution.
         base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
   }
 }
 
 bool DownloadItemImpl::IsDownloadReadyForCompletion() {
+  VLOG(20) << __FUNCTION__ << " " << AllDataSaved()
+           << " " << (GetSafetyState() != DownloadItem::DANGEROUS)
+           << " " << (state_ == IN_PROGRESS_INTERNAL)
+           << " " << !GetTargetFilePath().empty()
+           << " " << (target_path_.DirName() == current_path_.DirName());
   // If we don't have all the data, the download is not ready for
   // completion.
   if (!AllDataSaved())
     return false;
 
   // If the download is dangerous, but not yet validated, it's not ready for
   // completion.
   if (GetSafetyState() == DownloadItem::DANGEROUS)
     return false;
 
   // If the download isn't active (e.g. has been cancelled) it's not
   // ready for completion.
   if (state_ != IN_PROGRESS_INTERNAL)
     return false;
 
-  // If the download hasn't been inserted into the history system
-  // (which occurs strictly after file name determination, intermediate
-  // file rename, and UI display) then it's not ready for completion.
-  if (!IsPersisted())
+  // If the target filename hasn't been determined, then it's not ready for
+  // completion. This is checked in ReadyForDownloadCompletionDone().
+  if (GetTargetFilePath().empty())
+    return false;
+
+  // This is checked in NeedsRename(). Without this conditional,
+  // browser_tests:DownloadTest.DownloadMimeType fails the DCHECK.
+  if (target_path_.DirName() != current_path_.DirName())
     return false;
 
   return true;
 }
 
 bool DownloadItemImpl::NeedsRename() const {
   DCHECK(target_path_.DirName() == current_path_.DirName());
   return target_path_ != current_path_;
 }
 
@@ skipping 24 matching lines @@
     case CANCELLED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_CANCELED,
           base::Bind(&ItemCanceledNetLogCallback, received_bytes_,
                      &hash_state_));
       break;
     default:
       break;
   }
 
-  VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
+  VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true)
+    << " " << InternalToExternalState(old_state)
+    << " " << InternalToExternalState(state_);
 
   // Only update observers on user visible state changes.
   if (InternalToExternalState(old_state) != InternalToExternalState(state_))
     UpdateObservers();
 
   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)
@@ skipping 15 matching lines @@
   }
 }
 
 void DownloadItemImpl::SetFullPath(const FilePath& new_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << "()"
            << " new_path = \"" << new_path.value() << "\""
            << " " << DebugString(true);
   DCHECK(!new_path.empty());
   current_path_ = new_path;
+  UpdateObservers();
 
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_ITEM_RENAMED,
       base::Bind(&ItemRenamedNetLogCallback, &current_path_, &new_path));
 }
 
 // static
 DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
     DownloadInternalState internal_state) {
   switch (internal_state) {
@@ skipping 45 matching lines @@
       return "CANCELLED";
     case INTERRUPTED_INTERNAL:
       return "INTERRUPTED";
     default:
       NOTREACHED() << "Unknown download state " << state;
       return "unknown";
   };
 }
 
 }  // namespace content