Create the DownloadItem earlier in the download process.

Since DownloadItem is now created before we analyze the file to determine its characteristics and final name/location, I have added a function SetFileCheckResults() to update DownloadItem after this process has been done.

BUG=None
TEST=None

Review URL: http://codereview.chromium.org/5738007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@70096 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 115 insertions, 16 deletions

diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index 5743b78..035714e 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -176,6 +176,8 @@ void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
     return;
   }
 
+  manager->CreateDownloadItem(info);
+
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(this, &DownloadFileManager::CreateDownloadFile,
                         info, make_scoped_refptr(manager)));
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 43321aa..f6bee14 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -7,6 +7,7 @@
 #include "app/l10n_util.h"
 #include "base/basictypes.h"
 #include "base/file_util.h"
+#include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "base/timer.h"
@@ -374,6 +375,9 @@ int DownloadItem::PercentComplete() const {
 }
 
 void DownloadItem::Rename(const FilePath& full_path) {
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " full_path = " << full_path.value()
+           << DebugString(true);
   DCHECK(!full_path.empty());
   full_path_ = full_path;
 }
@@ -413,6 +417,8 @@ void DownloadItem::OnSafeDownloadFinished(DownloadFileManager* file_manager) {
 }
 
 void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " full_path = " << full_path.value();
   bool needed_rename = NeedsRename();
 
   Rename(full_path);
@@ -453,6 +459,36 @@ bool DownloadItem::MatchesQuery(const string16& query) const {
   return false;
 }
 
+void DownloadItem::SetFileCheckResults(const FilePath& path,
+                                       bool is_dangerous,
+                                       int path_uniquifier,
+                                       bool prompt,
+                                       bool is_extension_install,
+                                       const FilePath& original_name) {
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " path = \"" << path.value() << "\""
+           << " is_dangerous = " << is_dangerous
+           << " path_uniquifier = " << path_uniquifier
+           << " prompt = " << prompt
+           << " is_extension_install = " << is_extension_install
+           << " path = \"" << path.value() << "\""
+           << " original_name = \"" << original_name.value() << "\""
+           << " " << DebugString(true);
+  // Make sure the initial file name is set only once.
+  DCHECK(full_path_.empty());
+  DCHECK(!path.empty());
+
+  full_path_ = path;
+  safety_state_ = is_dangerous ? DANGEROUS : SAFE;
+  path_uniquifier_ = path_uniquifier;
+  save_as_ = prompt;
+  is_extension_install_ = is_extension_install;
+  target_name_ = original_name;
+
+  if (target_name_.value().empty())
+    target_name_ = full_path_.BaseName();
+}
+
 FilePath DownloadItem::GetTargetFilePath() const {
   return full_path_.DirName().Append(target_name_);
 }
@@ -477,24 +513,41 @@ void DownloadItem::Init(bool start_timer) {
     target_name_ = full_path_.BaseName();
   if (start_timer)
     StartProgressTimer();
+  VLOG(20) << " " << __FUNCTION__ << "() " << DebugString(true);
 }
 
 std::string DownloadItem::DebugString(bool verbose) const {
   std::string description =
-      base::StringPrintf("{ url = \"%s\"", url().spec().c_str());
+      base::StringPrintf("{ id_ = %d"
+                         " state = %s",
+                         id_,
+                         DebugDownloadStateString(state()));
 
   if (verbose) {
     description += base::StringPrintf(
-        " target_name_ = " "\"%s\""
-        " full_path = " "\"%s\""
-        " safety_state = " "%s",
+        " db_handle = %" PRId64
+        " total_bytes = %" PRId64
+        " is_paused = " "%c"
+        " is_extension_install = " "%c"
+        " is_otr = " "%c"
+        " safety_state = " "%s"
+        " url = " "\"%s\""
+        " target_name_ = \"%" PRFilePath "\""
+        " full_path = \"%" PRFilePath "\"",
+        db_handle(),
+        total_bytes(),
+        is_paused() ? 'T' : 'F',
+        is_extension_install() ? 'T' : 'F',
+        is_otr() ? 'T' : 'F',
+        DebugSafetyStateString(safety_state()),
+        url().spec().c_str(),
         target_name_.value().c_str(),
-        full_path().value().c_str(),
-        DebugSafetyStateString(safety_state()));
+        full_path().value().c_str());
+  } else {
+    description += base::StringPrintf(" url = \"%s\"", url().spec().c_str());
   }
 
-  description += base::StringPrintf(" state = %s }",
-                                    DebugDownloadStateString(state()));
+  description += " }";
 
   return description;
 }
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index 4f963fb..788ea98 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -150,6 +150,16 @@ class DownloadItem {
   // total size).
   int PercentComplete() const;
 
+  // Update the fields that may have changed in DownloadCreateInfo as a
+  // result of analyzing the file and figuring out its type, location, etc.
+  // May only be called once.
+  void SetFileCheckResults(const FilePath& path,
+                           bool is_dangerous,
+                           int path_uniquifier,
+                           bool prompt,
+                           bool is_extension_install,
+                           const FilePath& original_name);
+
   // Update the download's path, the actual file is renamed on the download
   // thread.
   void Rename(const FilePath& full_path);
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 135cfbd..da878b0 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -417,29 +417,58 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
     FOR_EACH_OBSERVER(Observer, observers_, SelectFileDialogDisplayed());
   } else {
     // No prompting for download, just continue with the suggested name.
-    CreateDownloadItem(info, info->suggested_path);
+    AttachDownloadItem(info, info->suggested_path);
   }
 }
 
-void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info,
+void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
+  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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_ptr<DownloadCreateInfo> infop(info);
   info->path = target_path;
 
-  DownloadItem* download = new DownloadItem(this, *info,
-                                            profile_->IsOffTheRecord());
+  // 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;
-  downloads_.insert(download);
 
   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();
+           << " info = " << info->DebugString()
+           << " download = " << download->DebugString(true);
 
   if (download_finished || info->is_dangerous) {
     // The download has already finished or the download is not safe.
@@ -877,7 +906,7 @@ void DownloadManager::FileSelected(const FilePath& path,
   DownloadCreateInfo* info = reinterpret_cast<DownloadCreateInfo*>(params);
   if (info->prompt_user_for_save_location)
     last_download_path_ = path.DirName();
-  CreateDownloadItem(info, path);
+  AttachDownloadItem(info, path);
 }
 
 void DownloadManager::FileSelectionCanceled(void* params) {
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 843efa5..4f5ebcf 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -189,6 +189,9 @@ class DownloadManager
 
   DownloadPrefs* download_prefs() { return download_prefs_.get(); }
 
+  // Creates the download item.  Must be called on the UI thread.
+  void CreateDownloadItem(DownloadCreateInfo* info);
+
   // Clears the last download path, used to initialize "save as" dialogs.
   void ClearLastDownloadPath();
 
@@ -249,7 +252,7 @@ class DownloadManager
   // Called back after a target path for the file to be downloaded to has been
   // determined, either automatically based on the suggested file name, or by
   // the user in a Save As dialog box.
-  void CreateDownloadItem(DownloadCreateInfo* info,
+  void AttachDownloadItem(DownloadCreateInfo* info,
                           const FilePath& target_path);
 
   // Download cancel helper function.
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index fdb088d..210788d 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -247,6 +247,8 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     if (kDownloadRenameCases[i].will_delete_crdownload)
       EXPECT_CALL(*download, DeleteCrDownload()).Times(1);
 
+    download_manager_->CreateDownloadItem(info);
+
     if (kDownloadRenameCases[i].finish_before_rename) {
       download_manager_->OnAllDataSaved(i, 1024);
       download_manager_->FileSelected(new_path, i, info);