Put history insertion for downloads processing inline.

chrome/browser/download/download_manager.cc

 // Copyright (c) 2010 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.
 
 #include "chrome/browser/download/download_manager.h"
 
 #include "app/l10n_util.h"
 #include "app/resource_bundle.h"
 #include "base/callback.h"
 #include "base/file_util.h"
@@ skipping 227 matching lines @@
       new OtherDownloadManagerObserver(this));
 
   return true;
 }
 
 // We have received a message from DownloadFileManager about a new download. We
 // create a download item and store it in our download map, and inform the
 // 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(info);
 
   // Check whether this download is for an extension install or not.
   // Allow extensions to be explicitly saved.
   if (!info->prompt_user_for_save_location) {
     if (UserScript::HasUserScriptFileExtension(info->url) ||
@@ skipping 166 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = new DownloadItem(this, *info,
                                             profile_->IsOffTheRecord());
   DCHECK(!ContainsKey(in_progress_, info->download_id));
   downloads_.insert(download);
 }
 
 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);
   info->path = target_path;
 
   // NOTE(ahendrickson) We will be adding a new map |active_downloads_|, into
   // which we will be adding the download as soon as it's created.  This will
   // make this loop unnecessary.
   // Eventually |active_downloads_| will replace |in_progress_|, but we don't
   // want to change the semantics yet.
   DCHECK(!ContainsKey(in_progress_, info->download_id));
   DownloadItem* download = NULL;
   for (std::set<DownloadItem*>::iterator i = downloads_.begin();
        i != downloads_.end(); ++i) {
     DownloadItem* item = (*i);
     if (item && (item->id() == info->download_id)) {
       download = item;
       break;
     }
   }
   DCHECK(download != NULL);
   download->SetFileCheckResults(info->path,
                                 info->is_dangerous,
                                 info->path_uniquifier,
                                 info->prompt_user_for_save_location,
                                 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,
         NewRunnableMethod(
             file_manager_, &DownloadFileManager::OnIntermediateDownloadName,
             download->id(), download_path, make_scoped_refptr(this)));
     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) {
   DownloadMap::iterator it = in_progress_.find(download_id);
   if (it != in_progress_.end()) {
     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_";
+
+  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;
+
+  MaybeCompleteDownload(download_id, size);
+}
+
+void DownloadManager::MaybeCompleteDownload(int32 download_id, int64 size) {
+  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id;
+
+  // 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;
   }
 
-  // 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_";
+  // 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.

+
+  VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
+           << download->DebugString(false);
+
+  // 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_.
+
+  // 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;
   }
-
-  DownloadItem* download = it->second;
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
-
-  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);
-  }
-
-  UpdateAppIcon();
-
-  // 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;
-  }
-
-  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;
-  }
-
-  download->OnSafeDownloadFinished(file_manager_);
 }
 
 void DownloadManager::DownloadRenamedToFinalName(int download_id,
                                                  const FilePath& full_path) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
@@ skipping 63 matching lines @@
     return;
   DownloadItem* download = it->second;
 
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
   // 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);
+    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,
                                                 int render_process_id,
                                                 int request_id) {
   // Cancel the network request.  RDH is guaranteed to outlive the IO thread.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableFunction(&download_util::CancelDownloadRequest,
                           g_browser_process->resource_dispatcher_host(),
@@ skipping 301 matching lines @@
   DCHECK(history_downloads_.find(download->db_handle()) ==
          history_downloads_.end());
   history_downloads_[download->db_handle()] = download;
 
   // Show in the appropropriate browser UI.
   ShowDownloadInBrowser(info, download);
 
   // 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,
                                             DownloadItem* download) {
   // The 'contents' may no longer exist if the user closed the tab before we
   // get this start completion event. If it does, tell the origin TabContents
   // to display its download shelf.
   TabContents* contents = tab_util::GetTabContentsByID(info.child_id,
                                                        info.render_view_id);
 
@@ skipping 95 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }