Put history insertion for downloads processing inline.

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 7200ba8062f3c760cb7121b85749ec43dd216075..0f6151bd24508d8faf46af821589e8b969bcf05c 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -242,7 +242,7 @@ bool DownloadManager::Init(Profile* profile) {
 // history system of a new download. Since this method can be called while the
 // history service thread is still reading the persistent state, we do not
 // insert the new DownloadItem into 'history_downloads_' or inform our
-// observers at this point. OnCreateDatabaseEntryComplete() handles that
+// observers at this point. OnCreateDownloadEntryComplete() handles that
 // finalization of the the download creation as a callback from the
 // history thread.
 void DownloadManager::StartDownload(DownloadCreateInfo* info) {
@@ -431,6 +431,8 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
 
 void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
                                          const FilePath& target_path) {
+  VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
+
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<DownloadCreateInfo> infop(info);
@@ -451,29 +453,25 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
                                 info->is_extension_install,
                                 info->original_name);
   in_progress_[info->download_id] = download;
+  UpdateAppIcon();  // Reflect entry into in_progress_.
 
-  bool download_finished = ContainsKey(pending_finished_downloads_,
-                                       info->download_id);
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " target_path = \"" << target_path.value() << "\""
-           << " download_finished = " << download_finished
-           << " info = " << info->DebugString()
-           << " download = " << download->DebugString(true);
-
-  if (download_finished || info->is_dangerous) {
-    // The download has already finished or the download is not safe.
-    // We can now rename the file to its final name (or its tentative name
-    // in dangerous download cases).
+  // Rename to intermediate name.
+  if (info->is_dangerous) {
+    // The download is not safe.  We can now rename the file to its
+    // tentative name using OnFinalDownloadName (the actual final
+    // name after user confirmation will be set in
+    // ProceedWithFinishedDangerousDownload).
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(
             file_manager_, &DownloadFileManager::OnFinalDownloadName,
-            download->id(), target_path, !info->is_dangerous,
+            download->id(), target_path, false,
             make_scoped_refptr(this)));
   } else {
-    // The download hasn't finished and it is a safe download.  We need to
-    // rename it to its intermediate '.crdownload' path.
+    // The download is a safe download.  We need to
+    // rename it to its intermediate '.crdownload' path.  The final
+    // name after user confirmation will be set from
+    // DownloadItem::OnSafeDownloadFinished.
     FilePath download_path = download_util::GetCrDownloadPath(target_path);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
@@ -483,17 +481,8 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info,
     download->Rename(download_path);
   }
 
-  if (download_finished) {
-    // If the download already completed by the time we reached this point, then
-    // notify observers that it did.
-    OnAllDataSaved(info->download_id,
-                   pending_finished_downloads_[info->download_id]);
-  }
-
   download_history_->AddEntry(*info, download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
-
-  UpdateAppIcon();
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
@@ -503,73 +492,95 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
     DownloadItem* download = it->second;
     if (download->state() == DownloadItem::IN_PROGRESS) {
       download->Update(size);
+      UpdateAppIcon();  // Reflect size updates.

[ahendrickson, 2011/01/13 03:15:17]

Why not set a flag if we update a download, then call |UpdateAppIcon()| only if
the flag is set, after the loop?

The current way would call it multiple times if there is more than one download
in progress.

[Randy Smith (Not in Mondays), 2011/01/16 20:43:56]

I'm very confused.  What loop?  We call find, get a single download, and test if
the state is IN_PROGRESS.  What am I missing?

       download_history_->UpdateEntry(download);
     }
   }
-  UpdateAppIcon();
 }
 
 void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end()) {
-    // The download is done, but the user hasn't selected a final location for
-    // it yet (the Save As dialog box is probably still showing), so just keep
-    // track of the fact that this download id is complete, when the
-    // DownloadItem is constructed later we'll notify its completion then.
-    DCHECK(!ContainsKey(pending_finished_downloads_, download_id));
-    pending_finished_downloads_[download_id] = size;
-    VLOG(20) << __FUNCTION__ << "()" << " Added download_id = " << download_id
-             << " to pending_finished_downloads_";
-    return;
-  }
 
-  // Remove the id from the list of pending ids.
-  PendingFinishedMap::iterator erase_it =
-      pending_finished_downloads_.find(download_id);
-  if (erase_it != pending_finished_downloads_.end()) {
-    pending_finished_downloads_.erase(erase_it);
-    VLOG(20) << __FUNCTION__ << "()" << " Removed download_id = " << download_id
-             << " from pending_finished_downloads_";
-  }
+  DCHECK_EQ(0U, pending_finished_downloads_.count(download_id));

[ahendrickson, 2011/01/13 03:15:17]

Would |ContainsKey()| work here, and in succeeding instances?

|count()| will only return 0 or 1 for a set or a map (as opposed to a
multi-map), right?

I find it more readable.

[Randy Smith (Not in Mondays), 2011/01/16 20:43:56]

So if you ask me for this change again I'll do it.  But I don't like
ContainsKey(), as it's not part of the standard library and count() is; it's yet
another routine that readers of the code have to remember what it does.  When I
can accomplish the same functionality in the same number of lines using a
smaller subset of routines, I prefer to do that.

+  pending_finished_downloads_[download_id] = size;
 
-  DownloadItem* download = it->second;
+  MaybeCompleteDownload(download_id);
+}
 
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
+bool DownloadManager::IsDownloadReadyForCompletion(int32 download_id) {
+  // If we don't have all the data, the download is not ready for
+  // completion.
+  if (pending_finished_downloads_.count(download_id) == 0)
+    return false;
 
-  download->OnAllDataSaved(size);
+  // If the download isn't active (e.g. has been cancelled) it's not
+  // ready for completion.
+  if (active_downloads_.count(download_id) == 0)
+    return false;
 
-  // Clean up will happen when the history system create callback runs if we
-  // don't have a valid db_handle yet.
-  if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(it);
-    download_history_->UpdateEntry(download);
-  }
+  // 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.
+  return (active_downloads_[download_id]->db_handle() !=
+          DownloadHistory::kUninitializedHandle);
+}
 
-  UpdateAppIcon();
+void DownloadManager::MaybeCompleteDownload(int32 download_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id;
 
-  // If this a dangerous download not yet validated by the user, don't do
-  // anything. When the user notifies us, it will trigger a call to
-  // ProceedWithFinishedDangerousDownload.
-  if (download->safety_state() == DownloadItem::DANGEROUS) {
+  if (!IsDownloadReadyForCompletion(download_id))
     return;
-  }
 
-  if (download->safety_state() == DownloadItem::DANGEROUS_BUT_VALIDATED) {
-    // We first need to rename the downloaded file from its temporary name to
-    // its final name before we can continue.
-    BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            this, &DownloadManager::ProceedWithFinishedDangerousDownload,
-            download->db_handle(),
-            download->full_path(), download->target_name()));
-    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;
+  // in in_progress_, in pending_finished_, have a history handle.
+  DCHECK_EQ(1u, in_progress_.count(download_id));
+  DCHECK_EQ(1u, pending_finished_downloads_.count(download_id));
+  DownloadItem* download = in_progress_[download_id];
+  DCHECK(download->db_handle() != DownloadHistory::kUninitializedHandle);
+  DCHECK_EQ(1u, history_downloads_.count(download->db_handle()));
+
+  VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
+           << download->DebugString(false);
 
-  download->OnSafeDownloadFinished(file_manager_);
+  // Remove the id from in_progress and pending_finished.
+  int size = pending_finished_downloads_[download_id];
+  pending_finished_downloads_.erase(download_id);
+  in_progress_.erase(download_id);
+  UpdateAppIcon();  // Reflect removal from in_progress_.
+
+  // Final update of download item and history.
+  download->OnAllDataSaved(size);

[ahendrickson, 2011/01/13 03:15:17]

It seems odd not to call |download->OnAllDataSaved()| as soon as all the data
has been saved (that is, wait on the final name).

Perhaps the function name needs to be changed.

[Paweł Hajdan Jr., 2011/01/13 08:43:10]

Yes, definitely. I thought that OnAllDataSaved should trigger running the body
of MaybeCompleteDownload, not the other way around.

[Randy Smith (Not in Mondays), 2011/01/16 20:43:56]

Well, DownloadManager::OnAllDataSaved triggers
DownloadManager::MaybeCompleteDownload, so that much is right in the world :-}. 
Some of the point for me of the overall refactor is to have anything happening
in the background be recognized at very specific points in the linear chain, and
this is where I want to recognize that all the data has been saved in the
download item (and make the transition to the COMPLETE) state.  

I've changed the name to OnReadyToFinish, and changed the documentation around
the COMPLETE state in the DownloadItem to indicate that that state indicates
that we've gotten all the data, chosen the final filename, entered ourselves in
the history, and been displayed ot the user.  But I'm not sure if that'll really
address your concerns; any suggestions for a better name?


On 2011/01/13 08:43:10, Paweł Hajdan Jr. wrote:

[Paweł Hajdan Jr., 2011/01/17 18:13:23]

I'm sorry, I actually think that OnReadyToFinish is a step backwards (the name
is more vague than OnAllDataSaved). The main concern is that it seems calling
OnAllDataSaved here wasn't needed before. Is it now really required, or can we
do the related refactoring in a separate change?

[Randy Smith (Not in Mondays), 2011/01/17 19:56:17]

Sorry, now you've gotten me confused.  download->OnAllDataSaved() was there
before, in the DownloadManager::OnAllDataSaved() function.  I've moved a lot
(close to all) of the processing out of DownloadManager::OnAllDataSaved() and
into MaybeCompleteDownload() as part of linearizing the processing of the
downloads.  I thought about moving download->OnAllDataSaved() back into
DownloadManager::OnAllDataSaved() in response to your comment, but
download->OnAllDataSaved() changes the DownloadItem state, and if there's one
thing that I really want to delay into MaybeCompleteDownload(), it's that.

Did that explanation clarify any?  If not, let's try to sync up by phone on
Tuesday--there's only so much confusion email is good for resolving :-} :-|.

+  download_history_->UpdateEntry(download);
+
+  switch (download->safety_state()) {
+    case DownloadItem::DANGEROUS:
+      // If this a dangerous download not yet validated by the user, don't do
+      // anything. When the user notifies us, it will trigger a call to
+      // ProceedWithFinishedDangerousDownload.
+      return;
+    case DownloadItem::DANGEROUS_BUT_VALIDATED:
+      // The dangerous download has been validated by the user.  We first
+      // need to rename the downloaded file from its temporary name to
+      // its final name.  We will continue the download processing in the
+      // callback.
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          NewRunnableMethod(
+              this, &DownloadManager::ProceedWithFinishedDangerousDownload,
+              download->db_handle(),
+              download->full_path(), download->target_name()));
+      return;
+    case DownloadItem::SAFE:
+      // The download is safe; just finish it.
+      download->OnSafeDownloadFinished(file_manager_);
+      return;
+  }
 }
 
 void DownloadManager::RemoveFromActiveList(int32 download_id) {
@@ -660,13 +671,13 @@ void DownloadManager::DownloadCancelled(int32 download_id) {
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(it);
     active_downloads_.erase(download_id);
+    UpdateAppIcon();  // Reflect removal from in_progress_.
     download_history_->UpdateEntry(download);
   }
 
   DownloadCancelledInternal(download_id,
                             download->render_process_id(),
                             download->request_id());
-  UpdateAppIcon();
 }
 
 void DownloadManager::DownloadCancelledInternal(int download_id,
@@ -986,23 +997,21 @@ void DownloadManager::OnCreateDownloadEntryComplete(
   // Inform interested objects about the new download.
   NotifyModelChanged();
 
-  // If this download has been completed before we've received the db handle,
+  // If this download has been cancelled before we've received the DB handle,
   // post one final message to the history service so that it can be properly
   // in sync with the DownloadItem's completion status, and also inform any
   // observers so that they get more than just the start notification.
+  //
+  // Otherwise, try to complete the download
   if (download->state() != DownloadItem::IN_PROGRESS) {
+    DCHECK_EQ(DownloadItem::CANCELLED, download->state());
     in_progress_.erase(it);
-    // TODO(ahendrickson) -- We don't actually know whether or not we can
-    // remove the download item from the |active_downloads_| map, as there
-    // is no state in |DownloadItem::DownloadState| to indicate that the
-    // downloads system is done with an item.  Fix this when we have a
-    // proper final state to check for.
     active_downloads_.erase(info.download_id);
     download_history_->UpdateEntry(download);
     download->UpdateObservers();
+  } else {
+    MaybeCompleteDownload(info.download_id);
   }
-
-  UpdateAppIcon();
 }
 
 void DownloadManager::ShowDownloadInBrowser(const DownloadCreateInfo& info,
@@ -1050,9 +1059,9 @@ DownloadItem* DownloadManager::GetDownloadItem(int id) {
 void DownloadManager::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
-  DownloadSet in_progress_set, history_set;
+  DownloadSet active_set, history_set;
   const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_};
-  DownloadSet* local_sets[] = {&in_progress_set, &history_set};
+  DownloadSet* local_sets[] = {&active_set, &history_set};
   DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(local_sets));
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input_maps); i++) {
     for (DownloadMap::const_iterator it = input_maps[i]->begin();
@@ -1062,7 +1071,7 @@ void DownloadManager::AssertContainersConsistent() const {
   }
 
   // Check if each set is fully present in downloads, and create a union.
-  const DownloadSet* all_sets[] = {&in_progress_set, &history_set,
+  const DownloadSet* all_sets[] = {&active_set, &history_set,
                                    &save_page_as_downloads_};
   DownloadSet downloads_union;
   for (int i = 0; i < static_cast<int>(ARRAYSIZE_UNSAFE(all_sets)); i++) {