GTTF: download cleanup, rename things to be more accurate.

Also, moved some code closer to the object it's operating on.

BUG=48913
TEST=unit_tests, ui_tests, browser_tests

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

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

10 files changed, 86 insertions, 163 deletions

diff --git a/chrome/browser/dom_ui/downloads_dom_handler.cc b/chrome/browser/dom_ui/downloads_dom_handler.cc
index 50fa502..80386eb 100644
--- a/chrome/browser/dom_ui/downloads_dom_handler.cc
+++ b/chrome/browser/dom_ui/downloads_dom_handler.cc
@@ -152,7 +152,7 @@ void DownloadsDOMHandler::HandleGetDownloads(const ListValue* args) {
 void DownloadsDOMHandler::HandleOpenFile(const ListValue* args) {
   DownloadItem* file = GetDownloadByValue(args);
   if (file)
-    download_manager_->OpenDownload(file, NULL);
+    file->OpenDownload();
 }
 
 void DownloadsDOMHandler::HandleDrag(const ListValue* args) {
@@ -180,7 +180,7 @@ void DownloadsDOMHandler::HandleDiscardDangerous(const ListValue* args) {
 void DownloadsDOMHandler::HandleShow(const ListValue* args) {
   DownloadItem* file = GetDownloadByValue(args);
   if (file)
-      download_manager_->ShowDownloadInShell(file);
+    file->ShowDownloadInShell();
 }
 
 void DownloadsDOMHandler::HandlePause(const ListValue* args) {
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index df28bbf..b66d1f8 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -202,7 +202,7 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
   }
 }
 
-void DownloadFileManager::DownloadFinished(int id, DownloadBuffer* buffer) {
+void DownloadFileManager::OnResponseCompleted(int id, DownloadBuffer* buffer) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   delete buffer;
   DownloadFileMap::iterator it = downloads_.find(id);
@@ -215,7 +215,7 @@ void DownloadFileManager::DownloadFinished(int id, DownloadBuffer* buffer) {
       ChromeThread::PostTask(
           ChromeThread::UI, FROM_HERE,
           NewRunnableMethod(
-              download_manager, &DownloadManager::DownloadFinished,
+              download_manager, &DownloadManager::OnAllDataSaved,
               id, download->bytes_so_far()));
     }
 
@@ -265,35 +265,6 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 
 // Actions from the UI thread and run on the download thread
 
-// Open a download, or show it in a file explorer window. We run on this
-// thread to avoid blocking the UI with (potentially) slow Shell operations.
-// TODO(paulg): File 'stat' operations.
-#if !defined(OS_MACOSX)
-void DownloadFileManager::OnShowDownloadInShell(const FilePath& full_path) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  platform_util::ShowItemInFolder(full_path);
-}
-#endif
-
-// Launches the selected download using ShellExecute 'open' verb. For windows,
-// if there is a valid parent window, the 'safer' version will be used which can
-// display a modal dialog asking for user consent on dangerous files.
-#if !defined(OS_MACOSX)
-void DownloadFileManager::OnOpenDownloadInShell(const FilePath& full_path,
-                                                const GURL& url,
-                                                gfx::NativeView parent_window) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-#if defined(OS_WIN)
-  if (NULL != parent_window) {
-    win_util::SaferOpenItemViaShell(parent_window, L"", full_path,
-                                    UTF8ToWide(url.spec()));
-    return;
-  }
-#endif
-  platform_util::OpenItem(full_path);
-}
-#endif  // OS_MACOSX
-
 // The DownloadManager in the UI thread has provided an intermediate .crdownload
 // name for the download specified by 'id'. Rename the in progress download.
 void DownloadFileManager::OnIntermediateDownloadName(
diff --git a/chrome/browser/download/download_file_manager.h b/chrome/browser/download/download_file_manager.h
index 9b909d9..5ccf533 100644
--- a/chrome/browser/download/download_file_manager.h
+++ b/chrome/browser/download/download_file_manager.h
@@ -81,24 +81,11 @@ class DownloadFileManager
   // download thread.
   void UpdateDownload(int id, DownloadBuffer* buffer);
   void CancelDownload(int id);
-  void DownloadFinished(int id, DownloadBuffer* buffer);
+  void OnResponseCompleted(int id, DownloadBuffer* buffer);
 
   // Called on FILE thread by DownloadManager at the beginning of its shutdown.
   void OnDownloadManagerShutdown(DownloadManager* manager);
 
-#if !defined(OS_MACOSX)
-  // The open and show methods run on the file thread, which does not work on
-  // Mac OS X (which uses the UI thread for opens).
-
-  // Handler for shell operations sent from the UI to the download thread.
-  void OnShowDownloadInShell(const FilePath& full_path);
-
-  // Handler to open or execute a downloaded file.
-  void OnOpenDownloadInShell(const FilePath& full_path,
-                             const GURL& url,
-                             gfx::NativeView parent_window);
-#endif
-
   // The DownloadManager in the UI thread has provided an intermediate
   // .crdownload name for the download specified by 'id'.
   void OnIntermediateDownloadName(int id, const FilePath& full_path,
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 4422600..8ba789c 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/history/download_types.h"
+#include "chrome/browser/platform_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/common/extensions/extension.h"
@@ -188,17 +189,35 @@ void DownloadItem::OpenDownload() {
   if (state() == DownloadItem::IN_PROGRESS) {
     open_when_complete_ = !open_when_complete_;
   } else if (state() == DownloadItem::COMPLETE) {
-    download_manager_->OpenDownload(this, NULL);
+    opened_ = true;
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
+    if (is_extension_install()) {
+      download_util::OpenChromeExtension(download_manager_->profile(),
+                                         download_manager_,
+                                         *this);
+      return;
+    }
+#if defined(OS_MACOSX)
+    // Mac OS X requires opening downloads on the UI thread.
+    platform_util::OpenItem(full_path());
+#else
+    ChromeThread::PostTask(
+        ChromeThread::FILE, FROM_HERE,
+        NewRunnableFunction(&platform_util::OpenItem, full_path()));
+#endif
   }
 }
 
-void DownloadItem::Opened() {
-  opened_ = true;
-  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
-}
-
 void DownloadItem::ShowDownloadInShell() {
-  download_manager_->ShowDownloadInShell(this);
+#if defined(OS_MACOSX)
+  // Mac needs to run this operation on the UI thread.
+  platform_util::ShowItemInFolder(full_path());
+#else
+  ChromeThread::PostTask(
+      ChromeThread::FILE, FROM_HERE,
+      NewRunnableFunction(&platform_util::ShowItemInFolder,
+                          full_path()));
+#endif
 }
 
 void DownloadItem::DangerousDownloadValidated() {
@@ -248,12 +267,42 @@ void DownloadItem::Cancel(bool update_history) {
     download_manager_->DownloadCancelled(id_);
 }
 
-void DownloadItem::Finished(int64 size) {
+void DownloadItem::OnAllDataSaved(int64 size) {
   state_ = COMPLETE;
   UpdateSize(size);
   StopProgressTimer();
 }
 
+void DownloadItem::Finished() {
+  // Handle chrome extensions explicitly and skip the shell execute.
+  if (is_extension_install()) {
+    download_util::OpenChromeExtension(download_manager_->profile(),
+                                       download_manager_,
+                                       *this);
+    auto_opened_ = true;
+  } else if (open_when_complete() ||
+             download_manager_->ShouldOpenFileBasedOnExtension(full_path()) ||
+             is_temporary()) {
+    // If the download is temporary, like in drag-and-drop, do not open it but
+    // we still need to set it auto-opened so that it can be removed from the
+    // download shelf.
+    if (!is_temporary())
+      OpenDownload();
+    auto_opened_ = true;
+  }
+
+  // Notify our observers that we are complete (the call to OnAllDataSaved()
+  // set the state to complete but did not notify).
+  UpdateObservers();
+
+  // The download file is meant to be completed if both the filename is
+  // finalized and the file data is downloaded. The ordering of these two
+  // actions is indeterministic. Thus, if the filename is not finalized yet,
+  // delay the notification.
+  if (name_finalized())
+    NotifyObserversDownloadFileCompleted();
+}
+
 void DownloadItem::Remove(bool delete_on_disk) {
   Cancel(true);
   state_ = REMOVING;
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index ed7c4f1..3431bc7 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -105,10 +105,6 @@ class DownloadItem {
   // complete if it is in progress).
   void OpenDownload();
 
-  // Callback from the DownloadManager when the item is opened. Sets opened_
-  // to true and notifies observers.
-  void Opened();
-
   // Show the download via the OS shell.
   void ShowDownloadInShell();
 
@@ -129,8 +125,12 @@ class DownloadItem {
   // when resuming a download (assuming the server supports byte ranges).
   void Cancel(bool update_history);
 
-  // Download operation completed.
-  void Finished(int64 size);
+  // Called when all data has been saved.
+  void OnAllDataSaved(int64 size);
+
+  // Called when the entire download operation (including renaming etc)
+  // is finished.
+  void Finished();
 
   // The user wants to remove the download from the views and history. If
   // |delete_file| is true, the file is deleted on the disk.
@@ -187,7 +187,6 @@ class DownloadItem {
     safety_state_ = safety_state;
   }
   bool auto_opened() { return auto_opened_; }
-  void set_auto_opened(bool auto_opened) { auto_opened_ = auto_opened; }
   FilePath original_name() const { return original_name_; }
   bool save_as() const { return save_as_; }
   bool is_otr() const { return is_otr_; }
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 5d503dd..d44bd38 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -403,12 +403,12 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
                                     owning_window, info);
   } else {
     // No prompting for download, just continue with the suggested name.
-    ContinueStartDownload(info, info->suggested_path);
+    CreateDownloadItem(info, info->suggested_path);
   }
 }
 
-void DownloadManager::ContinueStartDownload(DownloadCreateInfo* info,
-                                            const FilePath& target_path) {
+void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info,
+                                         const FilePath& target_path) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
   scoped_ptr<DownloadCreateInfo> infop(info);
@@ -446,8 +446,8 @@ void DownloadManager::ContinueStartDownload(DownloadCreateInfo* info,
   if (download_finished) {
     // If the download already completed by the time we reached this point, then
     // notify observers that it did.
-    DownloadFinished(info->download_id,
-                     pending_finished_downloads_[info->download_id]);
+    OnAllDataSaved(info->download_id,
+                   pending_finished_downloads_[info->download_id]);
   }
 
   download->Rename(target_path);
@@ -468,7 +468,7 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
   UpdateAppIcon();
 }
 
-void DownloadManager::DownloadFinished(int32 download_id, int64 size) {
+void DownloadManager::OnAllDataSaved(int32 download_id, int64 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
@@ -489,7 +489,7 @@ void DownloadManager::DownloadFinished(int32 download_id, int64 size) {
     pending_finished_downloads_.erase(erase_it);
 
   DownloadItem* download = it->second;
-  download->Finished(size);
+  download->OnAllDataSaved(size);
 
   // Clean up will happen when the history system create callback runs if we
   // don't have a valid db_handle yet.
@@ -556,31 +556,7 @@ void DownloadManager::ContinueDownloadFinished(DownloadItem* download) {
   // removed from dangerous_finished_ so it does not get deleted on shutdown.
   dangerous_finished_.erase(download->id());
 
-  // Handle chrome extensions explicitly and skip the shell execute.
-  if (download->is_extension_install()) {
-    download_util::OpenChromeExtension(profile_, this, *download);
-    download->set_auto_opened(true);
-  } else if (download->open_when_complete() ||
-             ShouldOpenFileBasedOnExtension(download->full_path()) ||
-             download->is_temporary()) {
-    // If the download is temporary, like in drag-and-drop, do not open it but
-    // we still need to set it auto-opened so that it can be removed from the
-    // download shelf.
-    if (!download->is_temporary())
-      OpenDownloadInShell(download, NULL);
-    download->set_auto_opened(true);
-  }
-
-  // Notify our observers that we are complete (the call to Finished() set the
-  // state to complete but did not notify).
-  download->UpdateObservers();
-
-  // The download file is meant to be completed if both the filename is
-  // finalized and the file data is downloaded. The ordering of these two
-  // actions is indeterministic. Thus, if the filename is not finalized yet,
-  // delay the notification.
-  if (download->name_finalized())
-    download->NotifyObserversDownloadFileCompleted();
+  download->Finished();
 }
 
 // Called on the file thread.  Renames the downloaded file to its original name.
@@ -778,7 +754,6 @@ int DownloadManager::RemoveDownloadsBetween(const base::Time remove_begin,
         dangerous_finished_.erase(dit);
 
       pending_deletes.push_back(download);
-    // Observer interface.
 
       continue;
     }
@@ -850,52 +825,6 @@ void DownloadManager::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-// Post Windows Shell operations to the Download thread, to avoid blocking the
-// user interface.
-void DownloadManager::ShowDownloadInShell(const DownloadItem* download) {
-  DCHECK(file_manager_);
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-#if defined(OS_MACOSX)
-  // Mac needs to run this operation on the UI thread.
-  platform_util::ShowItemInFolder(download->full_path());
-#else
-  ChromeThread::PostTask(
-      ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::OnShowDownloadInShell,
-          FilePath(download->full_path())));
-#endif
-}
-
-void DownloadManager::OpenDownload(DownloadItem* download,
-                                   gfx::NativeView parent_window) {
-  // Open Chrome extensions with ExtensionsService. For everything else do shell
-  // execute.
-  if (download->is_extension_install()) {
-    download->Opened();
-    download_util::OpenChromeExtension(profile_, this, *download);
-  } else {
-    OpenDownloadInShell(download, parent_window);
-  }
-}
-
-void DownloadManager::OpenDownloadInShell(DownloadItem* download,
-                                          gfx::NativeView parent_window) {
-  DCHECK(file_manager_);
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  download->Opened();
-#if defined(OS_MACOSX)
-  // Mac OS X requires opening downloads on the UI thread.
-  platform_util::OpenItem(download->full_path());
-#else
-  ChromeThread::PostTask(
-      ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::OnOpenDownloadInShell,
-          download->full_path(), download->url(), parent_window));
-#endif
-}
-
 bool DownloadManager::ShouldOpenFileBasedOnExtension(
     const FilePath& path) const {
   FilePath::StringType extension = path.Extension();
@@ -915,7 +844,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();
-  ContinueStartDownload(info, path);
+  CreateDownloadItem(info, path);
 }
 
 void DownloadManager::FileSelectionCanceled(void* params) {
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index bff3822..cb864ae 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -110,7 +110,7 @@ class DownloadManager
   // Notifications sent from the download thread to the UI thread
   void StartDownload(DownloadCreateInfo* info);
   void UpdateDownload(int32 download_id, int64 size);
-  void DownloadFinished(int32 download_id, int64 size);
+  void OnAllDataSaved(int32 download_id, int64 size);
 
   // Called from a view when a user clicks a UI button or link.
   void DownloadCancelled(int32 download_id);
@@ -165,14 +165,6 @@ class DownloadManager
   void ShowDownloadInBrowser(const DownloadCreateInfo& info,
                              DownloadItem* download);
 
-  // Opens a download. For Chrome extensions call
-  // ExtensionsServices::InstallExtension, for everything else call
-  // OpenDownloadInShell.
-  void OpenDownload(DownloadItem* download, gfx::NativeView parent_window);
-
-  // Show a download via the Windows shell.
-  void ShowDownloadInShell(const DownloadItem* download);
-
   // The number of in progress (including paused) downloads.
   int in_progress_count() const {
     return static_cast<int>(in_progress_.size());
@@ -225,10 +217,6 @@ class DownloadManager
 
   ~DownloadManager();
 
-  // Opens a download via the Windows shell.
-  void OpenDownloadInShell(DownloadItem* download,
-                           gfx::NativeView parent_window);
-
   // Called on the download thread to check whether the suggested file path
   // exists.  We don't check if the file exists on the UI thread to avoid UI
   // stalls from interacting with the file system.
@@ -241,8 +229,8 @@ 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 ContinueStartDownload(DownloadCreateInfo* info,
-                             const FilePath& target_path);
+  void CreateDownloadItem(DownloadCreateInfo* info,
+                          const FilePath& target_path);
 
   // Download cancel helper function.
   void DownloadCancelledInternal(int download_id,
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 726f491..913ad76 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -243,11 +243,11 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
       EXPECT_CALL(*download, DeleteCrDownload()).Times(1);
 
     if (kDownloadRenameCases[i].finish_before_rename) {
-      download_manager_->DownloadFinished(i, 1024);
+      download_manager_->OnAllDataSaved(i, 1024);
       download_manager_->FileSelected(new_path, i, info);
     } else {
       download_manager_->FileSelected(new_path, i, info);
-      download_manager_->DownloadFinished(i, 1024);
+      download_manager_->OnAllDataSaved(i, 1024);
     }
 
     message_loop_.RunAllPending();
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 190ba29..42003fa 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -722,9 +722,9 @@ void SavePackage::Finish() {
                         &SaveFileManager::RemoveSavedFileFromFileMap,
                         save_ids));
 
-  download_->Finished(all_save_items_count_);
-  // Notify download observers that we are complete (the call to Finished() set
-  // the state to complete but did not notify).
+  download_->OnAllDataSaved(all_save_items_count_);
+  // Notify download observers that we are complete (the call
+  // to OnAllDataSaved() set the state to complete but did not notify).
   download_->UpdateObservers();
 
   NotificationService::current()->Notify(
diff --git a/chrome/browser/renderer_host/download_resource_handler.cc b/chrome/browser/renderer_host/download_resource_handler.cc
index 3e5f3c0..8b32948 100644
--- a/chrome/browser/renderer_host/download_resource_handler.cc
+++ b/chrome/browser/renderer_host/download_resource_handler.cc
@@ -160,7 +160,7 @@ bool DownloadResourceHandler::OnResponseCompleted(
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
       NewRunnableMethod(download_file_manager_,
-                        &DownloadFileManager::DownloadFinished,
+                        &DownloadFileManager::OnResponseCompleted,
                         download_id_,
                         buffer_));
   read_buffer_ = NULL;