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 da878b0f2687bf7823138c1062daaa22da64b5f1..5a2a3e458d6ae61aef9a3adb27273d8cb3b2aa99 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -245,7 +245,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) {
@@ -432,6 +432,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);
@@ -460,29 +462,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_.

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

nit: Just two spaces between code and comment. Please fix in entire CL.

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

 
-  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,
@@ -492,17 +490,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) {
@@ -511,73 +500,75 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
     DownloadItem* download = it->second;
     download->Update(size);
     download_history_->UpdateEntry(download);
+    UpdateAppIcon();                      // Reflect size update.
   }
-  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.
-    PendingFinishedMap::iterator erase_it =
-        pending_finished_downloads_.find(download_id);
-    DCHECK(erase_it == pending_finished_downloads_.end());
-    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(pending_finished_downloads_.count(download_id), 0U);

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

nit: 0U should be first parameter (convention).

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

+  pending_finished_downloads_[download_id] = size;
 
-  DownloadItem* download = it->second;
+  MaybeCompleteDownload(download_id, size);
+}
 
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
+void DownloadManager::MaybeCompleteDownload(int32 download_id, int64 size) {
+  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id;
 
-  download->OnAllDataSaved(size);
-
-  // 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);
+  // Only complete the download if we've received all the data AND
+  // we've completed all UI initialization.
+  if (!(pending_finished_downloads_.count(download_id) != 0 &&

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

Can we extract this logic to a separate method?

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

+        in_progress_.count(download_id) != 0 &&
+        (in_progress_[download_id]->db_handle() !=
+         DownloadHistory::kUninitializedHandle))) {
+    return;
   }
 
-  UpdateAppIcon();
+  // Confirm we're in the proper set of states to be here;
+  // in in_progress_, in pending_finished_, have a history handle.
+  DCHECK(in_progress_.count(download_id) == 1);

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

nit: DCHECK_EQ?

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

+  DCHECK(pending_finished_downloads_.count(download_id) == 1);

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

nit: DCHECK_EQ?

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

+  DownloadItem* download = in_progress_[download_id];
+  DCHECK(download->db_handle() != DownloadHistory::kUninitializedHandle);
+  DCHECK(history_downloads_.count(download->db_handle()) == 1);

[Paweł Hajdan Jr., 2011/01/03 09:43:03]

nit: DCHECK_EQ?

[Randy Smith (Not in Mondays), 2011/01/04 19:29:21]

Done.

 
-  // 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) {
-    return;
-  }
+  VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
+           << download->DebugString(false);
 
-  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;
-  }
+  // Remove the id from in_progress and pending_finished.
+  pending_finished_downloads_.erase(download_id);
+  in_progress_.erase(download_id);
+  UpdateAppIcon();                      // Reflect removal from in_progress_.
 
-  download->OnSafeDownloadFinished(file_manager_);
+  // Final update of download item and history.
+  download->OnAllDataSaved(size);
+  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::DownloadRenamedToFinalName(int download_id,
@@ -661,13 +652,13 @@ void DownloadManager::DownloadCancelled(int32 download_id) {
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(it);
+    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,
@@ -989,17 +980,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(download->state() == DownloadItem::CANCELLED);
     in_progress_.erase(it);
     download_history_->UpdateEntry(download);
     download->UpdateObservers();
+  } else {
+    MaybeCompleteDownload(info.download_id,
+                          pending_finished_downloads_[info.download_id]);
   }
-
-  UpdateAppIcon();
 }
 
 void DownloadManager::ShowDownloadInBrowser(const DownloadCreateInfo& info,