Reduced the lifetime of DownloadCreateInfo.

chrome/browser/download/download_manager.cc

 // Copyright (c) 2011 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 "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/stl_util-inl.h"
 #include "base/stringprintf.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/download/download_create_info.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_process_handle.h"
 #include "chrome/browser/download/download_safe_browsing_client.h"
 #include "chrome/browser/download/download_status_updater.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/history/download_create_info.h"
+#include "chrome/browser/history/download_history_info.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/download/download_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/renderer_host/render_process_host.h"
@@ skipping 205 matching lines @@
 }
 
 // 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. OnCreateDownloadEntryComplete() handles that
 // finalization of the the download creation as a callback from the
 // history thread.
-void DownloadManager::StartDownload(DownloadCreateInfo* info) {
+void DownloadManager::StartDownload(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
   // Create a client to verify download URL with safebrowsing.
   // It deletes itself after the callback.
   scoped_refptr<DownloadSBClient> sb_client = new DownloadSBClient(
-      info->download_id, info->url_chain, info->referrer_url);
+      download_id, download->url_chain(), download->referrer_url());
   sb_client->CheckDownloadUrl(
-      info, NewCallback(this, &DownloadManager::CheckDownloadUrlDone));
+      NewCallback(this, &DownloadManager::CheckDownloadUrlDone));
 }
 
-void DownloadManager::CheckDownloadUrlDone(DownloadCreateInfo* info,
+void DownloadManager::CheckDownloadUrlDone(int32 download_id,
                                            bool is_dangerous_url) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(info);
 
-  info->is_dangerous_url = is_dangerous_url;
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  download->set_dangerous_url(is_dangerous_url);
+
+  DownloadItem::DownloadStateInfo state(*download);

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

This is strange. DownloadItem has a DownloadStateInfo member, and here we're
creating one based on the DownloadItem... it's weird. If needed, it's much more
straightforward to just have a DownloadStateInfo accessor in the DownloadItem.

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

This was at my request.  I'm ok with having an accessor, as long as the accessor
doesn't return a reference (implementation/interface confusion, worries about
lifetimes).  The accessor could also fill in a passed in structure if preferred.

[Paweł Hajdan Jr., 2011/05/19 19:11:24]

I'd prefer an accessor that returns a copy then.

[ahendrickson, 2011/05/19 20:16:49]

Done.

 
   // 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::IsURLUserScript(info->url(), info->mime_type) ||
-        info->mime_type == Extension::kMimeType) {
-      info->is_extension_install = true;
+  if (!state.prompt_user_for_save_location) {
+    if (UserScript::IsURLUserScript(download->url(), download->mime_type()) ||
+        (download->mime_type() == Extension::kMimeType)) {
+      state.is_extension_install = true;
     }
   }
 
-  if (info->save_info.file_path.empty()) {
+  if (state.force_file_name.empty()) {
     FilePath generated_name;
-    download_util::GenerateFileNameFromInfo(info, &generated_name);
+    download_util::GenerateFileNameFromRequest(download->url(),
+                                               download->content_disposition(),
+                                               download->referrer_charset(),
+                                               download->mime_type(),
+                                               &generated_name);
 
     // Freeze the user's preference for showing a Save As dialog.  We're going
     // to bounce around a bunch of threads and we don't want to worry about race
     // conditions where the user changes this pref out from under us.
     if (download_prefs_->PromptForDownload()) {
       // But ignore the user's preference for the following scenarios:
       // 1) Extension installation. Note that we only care here about the case
       //    where an extension is installed, not when one is downloaded with
       //    "save as...".
       // 2) Filetypes marked "always open." If the user just wants this file
       //    opened, don't bother asking where to keep it.
-      if (!info->is_extension_install &&
+      if (!state.is_extension_install &&
           !ShouldOpenFileBasedOnExtension(generated_name))
-        info->prompt_user_for_save_location = true;
+        state.prompt_user_for_save_location = true;
     }
     if (download_prefs_->IsDownloadPathManaged()) {
-      info->prompt_user_for_save_location = false;
+      state.prompt_user_for_save_location = false;
     }
 
     // Determine the proper path for a download, by either one of the following:
     // 1) using the default download directory.
     // 2) prompting the user.
-    if (info->prompt_user_for_save_location && !last_download_path_.empty()) {
-      info->suggested_path = last_download_path_;
+    if (state.prompt_user_for_save_location && !last_download_path_.empty()) {
+      state.suggested_path = last_download_path_;
     } else {
-      info->suggested_path = download_prefs_->download_path();
+      state.suggested_path = download_prefs_->download_path();
     }
-    info->suggested_path = info->suggested_path.Append(generated_name);
+    state.suggested_path = state.suggested_path.Append(generated_name);
   } else {
-    info->suggested_path = info->save_info.file_path;
+    state.suggested_path = state.force_file_name;
   }
 
-  if (!info->prompt_user_for_save_location &&
-      info->save_info.file_path.empty()) {
-    info->is_dangerous_file = download_util::IsDangerous(
-        info, profile(), ShouldOpenFileBasedOnExtension(info->suggested_path));
+  if (!state.prompt_user_for_save_location && state.force_file_name.empty()) {
+    state.is_dangerous_file = download_util::IsDangerous(
+        download->url(),
+        download->referrer_url(),
+        state.suggested_path,
+        state.has_user_gesture,
+        state.is_extension_install,
+        profile(),
+        ShouldOpenFileBasedOnExtension(state.suggested_path));
   }
 
   // We need to move over to the download thread because we don't want to stat
   // the suggested path on the UI thread.
   // We can only access preferences on the UI thread, so check the download path
   // now and pass the value to the FILE thread.
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           this,
           &DownloadManager::CheckIfSuggestedPathExists,
-          info,
+          download_id,
+          state,
           download_prefs()->download_path()));
 }
 
-void DownloadManager::CheckIfSuggestedPathExists(DownloadCreateInfo* info,
-                                                 const FilePath& default_path) {
+void DownloadManager::CheckIfSuggestedPathExists(
+    int32 download_id,
+    DownloadItem::DownloadStateInfo state,
+    const FilePath& default_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(info);
 
   // Make sure the default download directory exists.
   // TODO(phajdan.jr): only create the directory when we're sure the user
   // is going to save there and not to another directory of his choice.
   file_util::CreateDirectory(default_path);
 
   // Check writability of the suggested path. If we can't write to it, default
   // to the user's "My Documents" directory. We'll prompt them in this case.
-  FilePath dir = info->suggested_path.DirName();
-  FilePath filename = info->suggested_path.BaseName();
+  FilePath dir = state.suggested_path.DirName();
+  FilePath filename = state.suggested_path.BaseName();
   if (!file_util::PathIsWritable(dir)) {
     VLOG(1) << "Unable to write to directory \"" << dir.value() << "\"";
-    info->prompt_user_for_save_location = true;
-    PathService::Get(chrome::DIR_USER_DOCUMENTS, &info->suggested_path);
-    info->suggested_path = info->suggested_path.Append(filename);
+    state.prompt_user_for_save_location = true;
+    PathService::Get(chrome::DIR_USER_DOCUMENTS, &state.suggested_path);
+    state.suggested_path = state.suggested_path.Append(filename);
   }
 
   // If the download is deemed dangerous, we'll use a temporary name for it.
-  if (info->IsDangerous()) {
-    info->original_name = FilePath(info->suggested_path).BaseName();
+  if (state.IsDangerous()) {
+    state.target_name = FilePath(state.suggested_path).BaseName();
     // Create a temporary file to hold the file until the user approves its
     // download.
     FilePath::StringType file_name;
     FilePath path;
 #if defined(OS_WIN)
     string16 unconfirmed_prefix =
         l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
 #else
     std::string unconfirmed_prefix =
         l10n_util::GetStringUTF8(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
 #endif
 
     while (path.empty()) {
       base::SStringPrintf(
           &file_name,
           unconfirmed_prefix.append(
               FILE_PATH_LITERAL(" %d.crdownload")).c_str(),
           base::RandInt(0, 100000));
       path = dir.Append(file_name);
       if (file_util::PathExists(path))
         path = FilePath();
     }
-    info->suggested_path = path;
+    state.suggested_path = path;
   } else {
     // Do not add the path uniquifier if we are saving to a specific path as in
     // the drag-out case.
-    if (info->save_info.file_path.empty()) {
-      info->path_uniquifier = download_util::GetUniquePathNumberWithCrDownload(
-          info->suggested_path);
+    if (state.force_file_name.empty()) {
+      state.path_uniquifier = download_util::GetUniquePathNumberWithCrDownload(
+          state.suggested_path);
     }
     // We know the final path, build it if necessary.
-    if (info->path_uniquifier > 0) {
-      download_util::AppendNumberToPath(&(info->suggested_path),
-                                        info->path_uniquifier);
+    if (state.path_uniquifier > 0) {
+      download_util::AppendNumberToPath(&(state.suggested_path),
+                                        state.path_uniquifier);
       // Setting path_uniquifier to 0 to make sure we don't try to unique it
       // later on.
-      info->path_uniquifier = 0;
-    } else if (info->path_uniquifier == -1) {
+      state.path_uniquifier = 0;
+    } else if (state.path_uniquifier == -1) {
       // We failed to find a unique path.  We have to prompt the user.
       VLOG(1) << "Unable to find a unique path for suggested path \""
-              << info->suggested_path.value() << "\"";
-      info->prompt_user_for_save_location = true;
+              << state.suggested_path.value() << "\"";
+      state.prompt_user_for_save_location = true;
     }
   }
 
   // Create an empty file at the suggested path so that we don't allocate the
   // same "non-existant" path to multiple downloads.
   // See: http://code.google.com/p/chromium/issues/detail?id=3662
-  if (!info->prompt_user_for_save_location &&
-      info->save_info.file_path.empty()) {
-    if (info->IsDangerous())
-      file_util::WriteFile(info->suggested_path, "", 0);
+  if (!state.prompt_user_for_save_location &&
+      state.force_file_name.empty()) {
+    if (state.IsDangerous())
+      file_util::WriteFile(state.suggested_path, "", 0);
     else
       file_util::WriteFile(download_util::GetCrDownloadPath(
-          info->suggested_path), "", 0);
+          state.suggested_path), "", 0);
   }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this,
                         &DownloadManager::OnPathExistenceAvailable,
-                        info));
+                        download_id,
+                        state));
 }
 
-void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
-  VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
+void DownloadManager::OnPathExistenceAvailable(
+    int32 download_id, DownloadItem::DownloadStateInfo new_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(info);
 
-  if (info->prompt_user_for_save_location) {
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  VLOG(20) << __FUNCTION__ << "()"
+           << " download = " << download->DebugString(true);
+
+  download->SetFileCheckResults(new_state);
+
+  DownloadItem::DownloadStateInfo state(*download);

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

At this point, is there any need for another object?  Can't you just pull things
from the download item via accessors for the rest of the routing?

[ahendrickson, 2011/05/19 20:16:49]

I was trying to avoid adding more accessors to DownloadItem, but I guess I only
need to add one for suggested_path.

Done.

+
+  if (state.prompt_user_for_save_location) {
     // We must ask the user for the place to put the download.
     if (!select_file_dialog_.get())
       select_file_dialog_ = SelectFileDialog::Create(this);
 
-    TabContents* contents = info->process_handle.GetTabContents();
+    DownloadProcessHandle process_handle = download->process_handle();
+    TabContents* contents = process_handle.GetTabContents();
     SelectFileDialog::FileTypeInfo file_type_info;
-    FilePath::StringType extension = info->suggested_path.Extension();
+    FilePath::StringType extension = state.suggested_path.Extension();
     if (!extension.empty()) {
       extension.erase(extension.begin()); // drop the .
       file_type_info.extensions.resize(1);
       file_type_info.extensions[0].push_back(extension);
     }
     file_type_info.include_all_files = true;
     gfx::NativeWindow owning_window =
         contents ? platform_util::GetTopLevel(contents->GetNativeView()) : NULL;
+    // |id_ptr| will be deleted in either FileSelected() or

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

After thinking more about it - if we're using reinterpret_cast anyway, why do we
allocate a new int32 instead of just casting download_id to void*?

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

I thought about requesting this, and have no objections if you want to push for
it.  But I pinged someone more senior to me on the chrome team (cbentzel@) who
indicated that the int->void* cast was less Chroogley (:-}) than the allocation
and freeing.  Neither options awesome, so I'm cool with either.  

[Paweł Hajdan Jr., 2011/05/19 19:11:24]

I code-searched the source, and it seems most invocations of SelectFile use NULL
as the last parameter. I wonder if we could do the same here (I just didn't
check how SelectFile really works).

There is at least one place that just casts an int or something, see
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/ui/w...
(it's not really a good argument though).

I think both of the options here are not pretty, so I don't have a strong
preference. I'd be interested though why the int->void* cast is less Chroogley
than a dummy allocation.

[achuithb, 2011/05/19 19:47:36]

I think download_manager may be the only case of params being non-NULL. It would
be nice if we could get rid of it altogether. 

SelectFile launches a file picker dialog (When you rt-click and select Save As,
you get a file-picker dialog, for instance). In this case, I believe we're
allowing the user to choose the download location.

On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:

[ahendrickson, 2011/05/19 20:16:49]

I actually tried this first, and got GCC complaints when I tried to cast the
void* back to an int (loss of data) in the callbacks.

[ahendrickson, 2011/05/19 20:16:49]

NULL won't work, because we need the download ID in the callbacks.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

I see - hm, I think that means the current version has to be fine.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

I see, thank you for checking.

+    // FileSelectionCancelled().
+    int32* id_ptr = new int32;
+    *id_ptr = download_id;
     select_file_dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
                                     string16(),
-                                    info->suggested_path,
+                                    state.suggested_path,
                                     &file_type_info, 0, FILE_PATH_LITERAL(""),
-                                    contents, owning_window, info);
+                                    contents, owning_window,
+                                    reinterpret_cast<void *>(id_ptr));
     FOR_EACH_OBSERVER(Observer, observers_,
-                      SelectFileDialogDisplayed(info->download_id));
+                      SelectFileDialogDisplayed(download_id));
   } else {
     // No prompting for download, just continue with the suggested name.
-    info->path = info->suggested_path;
-    AttachDownloadItem(info);
+    ContinueDownloadWithPath(download_id, state.suggested_path);
   }
 }
 
 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));
-  DCHECK(!ContainsKey(active_downloads_, info->download_id));
+  int32 download_id = info->download_id;
+  DCHECK(!ContainsKey(in_progress_, download_id));
+  DCHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
-  active_downloads_[info->download_id] = download;
+  active_downloads_[download_id] = download;
 }
 
-void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info) {
-  VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
-
+void DownloadManager::ContinueDownloadWithPath(int32 download_id,
+                                               const FilePath& chosen_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // Life of |info| ends here. No more references to it after this method.
-  scoped_ptr<DownloadCreateInfo> infop(info);
-
   // NOTE(ahendrickson) Eventually |active_downloads_| will replace
   // |in_progress_|, but we don't want to change the semantics yet.
-  DCHECK(!ContainsKey(in_progress_, info->download_id));
-  DCHECK(ContainsKey(active_downloads_, info->download_id));
-  DownloadItem* download = active_downloads_[info->download_id];
-  DCHECK(download != NULL);
+  DCHECK(!ContainsKey(in_progress_, download_id));
+  DownloadItem* download = GetActiveDownloadItem(download_id);
   DCHECK(ContainsKey(downloads_, download));
+  if (!download)

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

If we return here, the DCHECK above should have fired, right? That means we
don't expect download to be NULL here and the NULL check should be removed, or
the DCHECK above should be removed/adjusted.

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

Both the routines that call ContinueDownloadWithPath have the download pointer;
why not just change the method signature to take that?

[ahendrickson, 2011/05/19 20:16:49]

Done.

[ahendrickson, 2011/05/19 20:16:49]

Done.

+    return;
 
-  download->SetFileCheckResults(info->path,
-                                info->is_dangerous_file,
-                                info->is_dangerous_url,
-                                info->path_uniquifier,
-                                info->prompt_user_for_save_location,
-                                info->is_extension_install,
-                                info->original_name);
-  in_progress_[info->download_id] = download;
+  // Make sure the initial file name is set only once.
+  DCHECK(download->full_path().empty());

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

Doesn't the drag-n-drop initiation set the full_path?  

[ahendrickson, 2011/05/19 20:16:49]

I don't believe so; I think it sets save_info.

+  download->set_path(chosen_file);
+  download->UpdateTarget();
+
+  VLOG(20) << __FUNCTION__ << "()"
+           << " download = " << download->DebugString(true);
+
+  in_progress_[download_id] = download;
   UpdateAppIcon();  // Reflect entry into in_progress_.
 
   // Rename to intermediate name.
   FilePath download_path;
-  if (info->IsDangerous()) {
+  if (download->IsDangerous()) {
     // The download is not safe.  We can now rename the file to its
     // tentative name using RenameInProgressDownloadFile.
     // NOTE: The |Rename| below will be a no-op for dangerous files, as we're
     // renaming it to the same name.
-    download_path = info->path;
+    download_path = download->full_path();
   } else {
     // 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::OnDownloadCompleting.
-    download_path = download_util::GetCrDownloadPath(info->path);
+    download_path =
+        download_util::GetCrDownloadPath(download->full_path());
   }
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
           download->id(), download_path));
 
   download->Rename(download_path);
 
-  download_history_->AddEntry(*info, download,
+  download_history_->AddEntry(download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
   if (it != active_downloads_.end()) {
     DownloadItem* download = it->second;
     if (download->IsInProgress()) {
       download->Update(size);
@@ skipping 193 matching lines @@
                                       int os_error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
   // A cancel at the right time could remove the download from the
   // |active_downloads_| map before we get here.
   if (it == active_downloads_.end())
     return;
 
   DownloadItem* download = it->second;
 
-  VLOG(20) << "Error " << os_error << " at offset "
-           << download->received_bytes() << " for download = "
-           << download->DebugString(true);
+  VLOG(20) << __FUNCTION__ << "()" << " Error " << os_error
+           << " at offset " << download->received_bytes()
+           << " for download = " << download->DebugString(true);
+
+  download->Interrupted(size, os_error);
 
   // TODO(ahendrickson) - Remove this when we add resuming of interrupted
   // downloads, as we will keep the download item around in that case.
   //
   // 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(download_id);
     active_downloads_.erase(download_id);
     UpdateAppIcon();  // Reflect removal from in_progress_.
     download_history_->UpdateEntry(download);
   }
 
-  download->Interrupted(size, os_error);
-
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
 void DownloadManager::PauseDownload(int32 download_id, bool pause) {
   DownloadMap::iterator it = in_progress_.find(download_id);
   if (it == in_progress_.end())
     return;
@@ skipping 196 matching lines @@
   int64 total_bytes = 0;
   for (DownloadMap::iterator i = in_progress_.begin();
        i != in_progress_.end(); ++i) {
     total_bytes += i->second->total_bytes();
   }
   return total_bytes;
 }
 
 void DownloadManager::FileSelected(const FilePath& path,
                                    int index, void* params) {
-  DownloadCreateInfo* info = reinterpret_cast<DownloadCreateInfo*>(params);
-  if (info->prompt_user_for_save_location)
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  int32* id_ptr = reinterpret_cast<int32 *>(params);
+  DCHECK(id_ptr != NULL);
+  int32 download_id = *id_ptr;
+  delete id_ptr;
+  DCHECK_EQ(index, download_id);

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

Why are we expecting the index and the download_id to be the same?  I'm really
confused.

[ahendrickson, 2011/05/19 20:16:49]

Oops, my bad.  I was confusing it with the history callback.

+
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+  VLOG(20) << __FUNCTION__ << "()" << " path = \"" << path.value() << "\""
+            << " download = " << download->DebugString(true);
+
+  if (download->save_as())
     last_download_path_ = path.DirName();
 
-  info->path = path;
-  AttachDownloadItem(info);
+  // Make sure the initial file name is set only once.
+  ContinueDownloadWithPath(download_id, path);
 }
 
 void DownloadManager::FileSelectionCanceled(void* params) {
   // The user didn't pick a place to save the file, so need to cancel the
   // download that's already in progress to the temporary location.
-  DownloadCreateInfo* info = reinterpret_cast<DownloadCreateInfo*>(params);
-  DownloadCancelledInternal(info->download_id, info->process_handle);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  int32* id_ptr = reinterpret_cast<int32 *>(params);
+  DCHECK(id_ptr != NULL);
+  int32 download_id = *id_ptr;
+  delete id_ptr;
+
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  VLOG(20) << __FUNCTION__ << "()"
+           << " download = " << download->DebugString(true);
+
+  DownloadCancelledInternal(download_id, download->process_handle());
 }
 
 void DownloadManager::DangerousDownloadValidated(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(DownloadItem::DANGEROUS, download->safety_state());
   download->set_safety_state(DownloadItem::DANGEROUS_BUT_VALIDATED);
   download->UpdateObservers();
 
   MaybeCompleteDownload(download);
 }
 
 // Operations posted to us from the history service ----------------------------
 
 // The history service has retrieved all download entries. 'entries' contains
-// 'DownloadCreateInfo's in sorted order (by ascending start_time).
+// 'DownloadHistoryInfo's in sorted order (by ascending start_time).
 void DownloadManager::OnQueryDownloadEntriesComplete(
-    std::vector<DownloadCreateInfo>* entries) {
+    std::vector<DownloadHistoryInfo>* entries) {
   for (size_t i = 0; i < entries->size(); ++i) {
     DownloadItem* download = new DownloadItem(this, entries->at(i));
     DCHECK(!ContainsKey(history_downloads_, download->db_handle()));
     downloads_.insert(download);
     history_downloads_[download->db_handle()] = download;
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
   }
   NotifyModelChanged();
 }
 
 // Once the new DownloadItem's creation info has been committed to the history
 // service, we associate the DownloadItem with the db handle, update our
 // 'history_downloads_' map and inform observers.
-void DownloadManager::OnCreateDownloadEntryComplete(
-    DownloadCreateInfo info,
-    int64 db_handle) {
+void DownloadManager::OnCreateDownloadEntryComplete(int32 download_id,
+                                                    int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = in_progress_.find(info.download_id);
-  DCHECK(it != in_progress_.end());
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
 
-  DownloadItem* download = it->second;
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
-           << " download_id = " << info.download_id
+           << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadHistory::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
 
   DCHECK(download->db_handle() == DownloadHistory::kUninitializedHandle);
   download->set_db_handle(db_handle);
 
   DCHECK(!ContainsKey(history_downloads_, download->db_handle()));
   history_downloads_[download->db_handle()] = download;
 
   // Show in the appropriate browser UI.
   // This includes buttons to save or cancel, for a dangerous download.
-  ShowDownloadInBrowser(&info.process_handle, download);
+  ShowDownloadInBrowser(download);
 
   // Inform interested objects about the new download.
   NotifyModelChanged();
 
   // If the download is still in progress, try to complete it.
   //
   // Otherwise, download has been cancelled or interrupted before we've
   // received the DB handle.  We 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.
   if (download->IsInProgress()) {
     MaybeCompleteDownload(download);
   } else {
     DCHECK(download->IsCancelled())
         << " download = " << download->DebugString(true);
-    in_progress_.erase(it);
-    active_downloads_.erase(info.download_id);
+    in_progress_.erase(download_id);
+    active_downloads_.erase(download_id);
     download_history_->UpdateEntry(download);
     download->UpdateObservers();
   }
 }
 
-void DownloadManager::ShowDownloadInBrowser(
-    DownloadProcessHandle* process_handle, DownloadItem* download) {
-  if (!process_handle)
-    return;
+void DownloadManager::ShowDownloadInBrowser(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 = process_handle->GetTabContents();
+  DownloadProcessHandle process_handle = download->process_handle();
+  TabContents* contents = process_handle.GetTabContents();
   TabContentsWrapper* wrapper = NULL;
   if (contents)
       wrapper = TabContentsWrapper::GetCurrentWrapperForContents(contents);
 
   // If the contents no longer exists, we start the download in the last active
   // browser. This is not ideal but better than fully hiding the download from
   // the user.
   if (!wrapper) {
     Browser* last_active = BrowserList::GetLastActive();
     if (last_active)
       wrapper = last_active->GetSelectedTabContentsWrapper();
   }
 
   if (!wrapper)
     return;
 
   wrapper->download_tab_helper()->OnStartDownload(download);
 }
 
 // Clears the last download path, used to initialize "save as" dialogs.
 void DownloadManager::ClearLastDownloadPath() {
   last_download_path_ = FilePath();
 }
 
 void DownloadManager::NotifyModelChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, ModelChanged());
 }
 
-DownloadItem* DownloadManager::GetDownloadItem(int id) {
+DownloadItem* DownloadManager::GetDownloadItem(int download_id) {
   for (DownloadMap::iterator it = history_downloads_.begin();
        it != history_downloads_.end(); ++it) {
     DownloadItem* item = it->second;
-    if (item->id() == id)
+    if (item->id() == download_id)
       return item;
   }
   return NULL;
 }
 
+DownloadItem* DownloadManager::GetActiveDownloadItem(int download_id) {
+  DCHECK(ContainsKey(active_downloads_, download_id));

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

Is it possible to make GetDownloadItem and GetActiveDownloadItem more similar to
each other? The former uses a loop, and the latter just does a map lookup.

[ahendrickson, 2011/05/19 20:16:49]

I don't think so, because |history_downloads_| is indexed by the |db_handle|,
where |active_downloads_| is indexed by the download id.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

Ah, I see. Could you add a comment (probably in GetDownloadItem) about that?

[ahendrickson, 2011/05/20 18:31:24]

Done.

[Randy Smith (Not in Mondays), 2011/05/20 19:49:29]

Sorry to keep this thread going, but my scan of the file suggests that
everywhere we use GetDownloadItem we could, and should, use
GetActiveDownloadItem.  Could you check me on that, and if I'm right, nuke
GetDownloadItem?

[ahendrickson, 2011/05/20 22:12:44]

It's used in download_manager_unittest.cc, after the download is done.

That might possibly be worked around, if we retrieve the download item pointer
beforehand.

However, I don't think that belongs in this CL.

+  DownloadItem* download = active_downloads_[download_id];
+  DCHECK(download != NULL);
+  return download;
+}
+
 // Confirm that everything in all maps is also in |downloads_|, and that
 // everything in |downloads_| is also in some other map.
 void DownloadManager::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
   DownloadSet active_set, history_set;
   const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_};
   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++) {
@@ skipping 53 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }