Download code cleanup:

- convert some private static members to anonymous functions
- move some methods from public to private

This should make the interface of some download classes smaller,
and hopefully easier to understand.

BUG=48913
TEST=unit_tests, browser_tests, ui_tests

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

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

5 files changed, 169 insertions, 202 deletions

diff --git a/chrome/browser/download/download_file.cc b/chrome/browser/download/download_file.cc
index 1ccd966..521a5c8 100644
--- a/chrome/browser/download/download_file.cc
+++ b/chrome/browser/download/download_file.cc
@@ -37,24 +37,6 @@
 // cause it to become unresponsive (in milliseconds).
 static const int kUpdatePeriodMs = 500;
 
-// Timer task for posting UI updates. This task is created and maintained by
-// the DownloadFileManager long as there is an in progress download. The task
-// is cancelled when all active downloads have completed, or in the destructor
-// of the DownloadFileManager.
-class DownloadFileUpdateTask : public Task {
- public:
-  explicit DownloadFileUpdateTask(DownloadFileManager* manager)
-      : manager_(manager) {}
-  virtual void Run() {
-    manager_->UpdateInProgressDownloads();
-  }
-
- private:
-  DownloadFileManager* manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadFileUpdateTask);
-};
-
 // DownloadFile implementation -------------------------------------------------
 
 DownloadFile::DownloadFile(const DownloadCreateInfo* info)
@@ -243,6 +225,84 @@ void DownloadFileManager::OnShutdown() {
   downloads_.clear();
 }
 
+// Initiate a request for URL to be downloaded. Called from UI thread,
+// runs on IO thread.
+void DownloadFileManager::OnDownloadUrl(
+    const GURL& url,
+    const GURL& referrer,
+    const std::string& referrer_charset,
+    const DownloadSaveInfo& save_info,
+    int render_process_host_id,
+    int render_view_id,
+    URLRequestContextGetter* request_context_getter) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+
+  URLRequestContext* context = request_context_getter->GetURLRequestContext();
+  context->set_referrer_charset(referrer_charset);
+
+  resource_dispatcher_host_->BeginDownload(url,
+                                           referrer,
+                                           save_info,
+                                           render_process_host_id,
+                                           render_view_id,
+                                           context);
+}
+
+// Notifications sent from the download thread and run on the UI thread.
+
+// Lookup the DownloadManager for this TabContents' profile and inform it of
+// a new download.
+// TODO(paulg): When implementing download restart via the Downloads tab,
+//              there will be no 'render_process_id' or 'render_view_id'.
+void DownloadFileManager::OnStartDownload(DownloadCreateInfo* info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DownloadManager* manager = DownloadManagerFromRenderIds(info->child_id,
+                                                          info->render_view_id);
+  if (!manager) {
+    ChromeThread::PostTask(
+        ChromeThread::IO, FROM_HERE,
+        NewRunnableFunction(&DownloadManager::OnCancelDownloadRequest,
+                            resource_dispatcher_host_,
+                            info->child_id,
+                            info->request_id));
+    delete info;
+    return;
+  }
+
+  StartUpdateTimer();
+
+  // Add the download manager to our request maps for future updates. We want to
+  // be able to cancel all in progress downloads when a DownloadManager is
+  // deleted, such as when a profile is closed. We also want to be able to look
+  // up the DownloadManager associated with a given request without having to
+  // rely on using tab information, since a tab may be closed while a download
+  // initiated from that tab is still in progress.
+  DownloadRequests& downloads = requests_[manager];
+  downloads.insert(info->download_id);
+
+  // TODO(paulg): The manager will exist when restarts are implemented.
+  DownloadManagerMap::iterator dit = managers_.find(info->download_id);
+  if (dit == managers_.end())
+    managers_[info->download_id] = manager;
+  else
+    NOTREACHED();
+
+  // StartDownload will clean up |info|.
+  manager->StartDownload(info);
+}
+
+// Update the Download Manager with the finish state, and remove the request
+// tracking entries.
+void DownloadFileManager::OnDownloadFinished(int id,
+                                             int64 bytes_so_far) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DownloadManager* manager = LookupManager(id);
+  if (manager)
+    manager->DownloadFinished(id, bytes_so_far);
+  RemoveDownload(id, manager);
+  RemoveDownloadFromUIProgress(id);
+}
+
 // Lookup one in-progress download.
 DownloadFile* DownloadFileManager::LookupDownload(int id) {
   DownloadFileMap::iterator it = downloads_.find(id);
@@ -272,6 +332,20 @@ void DownloadFileManager::StopUpdateTimer() {
   update_timer_.Stop();
 }
 
+// Our periodic timer has fired so send the UI thread updates on all in progress
+// downloads.
+void DownloadFileManager::UpdateInProgressDownloads() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AutoLock lock(progress_lock_);
+  ProgressMap::iterator it = ui_progress_.begin();
+  for (; it != ui_progress_.end(); ++it) {
+    const int id = it->first;
+    DownloadManager* manager = LookupManager(id);
+    if (manager)
+      manager->UpdateDownload(id, it->second);
+  }
+}
+
 // Called on the IO thread once the ResourceDispatcherHost has decided that a
 // request is a download.
 int DownloadFileManager::GetNextId() {
@@ -405,75 +479,6 @@ void DownloadFileManager::CancelDownload(int id) {
   }
 }
 
-// Our periodic timer has fired so send the UI thread updates on all in progress
-// downloads.
-void DownloadFileManager::UpdateInProgressDownloads() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  AutoLock lock(progress_lock_);
-  ProgressMap::iterator it = ui_progress_.begin();
-  for (; it != ui_progress_.end(); ++it) {
-    const int id = it->first;
-    DownloadManager* manager = LookupManager(id);
-    if (manager)
-      manager->UpdateDownload(id, it->second);
-  }
-}
-
-// Notifications sent from the download thread and run on the UI thread.
-
-// Lookup the DownloadManager for this TabContents' profile and inform it of
-// a new download.
-// TODO(paulg): When implementing download restart via the Downloads tab,
-//              there will be no 'render_process_id' or 'render_view_id'.
-void DownloadFileManager::OnStartDownload(DownloadCreateInfo* info) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  DownloadManager* manager = DownloadManagerFromRenderIds(info->child_id,
-                                                          info->render_view_id);
-  if (!manager) {
-    ChromeThread::PostTask(
-        ChromeThread::IO, FROM_HERE,
-        NewRunnableFunction(&DownloadManager::OnCancelDownloadRequest,
-                            resource_dispatcher_host_,
-                            info->child_id,
-                            info->request_id));
-    delete info;
-    return;
-  }
-
-  StartUpdateTimer();
-
-  // Add the download manager to our request maps for future updates. We want to
-  // be able to cancel all in progress downloads when a DownloadManager is
-  // deleted, such as when a profile is closed. We also want to be able to look
-  // up the DownloadManager associated with a given request without having to
-  // rely on using tab information, since a tab may be closed while a download
-  // initiated from that tab is still in progress.
-  DownloadRequests& downloads = requests_[manager];
-  downloads.insert(info->download_id);
-
-  // TODO(paulg): The manager will exist when restarts are implemented.
-  DownloadManagerMap::iterator dit = managers_.find(info->download_id);
-  if (dit == managers_.end())
-    managers_[info->download_id] = manager;
-  else
-    NOTREACHED();
-
-  // StartDownload will clean up |info|.
-  manager->StartDownload(info);
-}
-
-// Update the Download Manager with the finish state, and remove the request
-// tracking entries.
-void DownloadFileManager::OnDownloadFinished(int id,
-                                             int64 bytes_so_far) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  DownloadManager* manager = LookupManager(id);
-  if (manager)
-    manager->DownloadFinished(id, bytes_so_far);
-  RemoveDownload(id, manager);
-  RemoveDownloadFromUIProgress(id);
-}
-
 void DownloadFileManager::DownloadUrl(
     const GURL& url,
     const GURL& referrer,
@@ -567,31 +572,6 @@ void DownloadFileManager::RemoveDownloadManager(DownloadManager* manager) {
   requests_.erase(it);
 }
 
-
-// Notifications from the UI thread and run on the IO thread
-
-// Initiate a request for URL to be downloaded.
-void DownloadFileManager::OnDownloadUrl(
-    const GURL& url,
-    const GURL& referrer,
-    const std::string& referrer_charset,
-    const DownloadSaveInfo& save_info,
-    int render_process_host_id,
-    int render_view_id,
-    URLRequestContextGetter* request_context_getter) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-
-  URLRequestContext* context = request_context_getter->GetURLRequestContext();
-  context->set_referrer_charset(referrer_charset);
-
-  resource_dispatcher_host_->BeginDownload(url,
-                                           referrer,
-                                           save_info,
-                                           render_process_host_id,
-                                           render_view_id,
-                                           context);
-}
-
 // 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
@@ -681,9 +661,3 @@ void DownloadFileManager::OnFinalDownloadName(int id,
   }
 }
 
-// static
-void DownloadFileManager::DeleteFile(const FilePath& path) {
-  // Make sure we only delete files.
-  if (!file_util::DirectoryExists(path))
-    file_util::Delete(path, false);
-}
diff --git a/chrome/browser/download/download_file.h b/chrome/browser/download/download_file.h
index caabee4..b905d83 100644
--- a/chrome/browser/download/download_file.h
+++ b/chrome/browser/download/download_file.h
@@ -197,11 +197,6 @@ class DownloadFileManager
   void CancelDownload(int id);
   void DownloadFinished(int id, DownloadBuffer* buffer);
 
-  // Handlers for notifications sent from the download thread and run on
-  // the UI thread.
-  void OnStartDownload(DownloadCreateInfo* info);
-  void OnDownloadFinished(int id, int64 bytes_so_far);
-
   // Download the URL. Called on the UI thread and forwarded to the
   // ResourceDispatcherHost on the IO thread.
   void DownloadUrl(const GURL& url,
@@ -212,15 +207,6 @@ class DownloadFileManager
                    int render_view_id,
                    URLRequestContextGetter* request_context_getter);
 
-  // Run on the IO thread to initiate the download of a URL.
-  void OnDownloadUrl(const GURL& url,
-                     const GURL& referrer,
-                     const std::string& referrer_charset,
-                     const DownloadSaveInfo& save_info,
-                     int render_process_host_id,
-                     int render_view_id,
-                     URLRequestContextGetter* request_context_getter);
-
   // Called on the UI thread to remove a download item or manager.
   void RemoveDownloadManager(DownloadManager* manager);
   void RemoveDownload(int id, DownloadManager* manager);
@@ -243,12 +229,6 @@ class DownloadFileManager
   void OnFinalDownloadName(int id, const FilePath& full_path,
                            DownloadManager* download_manager);
 
-  // Timer notifications.
-  void UpdateInProgressDownloads();
-
-  // Called by the download manager to delete non validated dangerous downloads.
-  static void DeleteFile(const FilePath& path);
-
  private:
   friend class base::RefCountedThreadSafe<DownloadFileManager>;
 
@@ -257,10 +237,25 @@ class DownloadFileManager
   // Timer helpers for updating the UI about the current progress of a download.
   void StartUpdateTimer();
   void StopUpdateTimer();
+  void UpdateInProgressDownloads();
 
   // Clean up helper that runs on the download thread.
   void OnShutdown();
 
+  // Run on the IO thread to initiate the download of a URL.
+  void OnDownloadUrl(const GURL& url,
+                     const GURL& referrer,
+                     const std::string& referrer_charset,
+                     const DownloadSaveInfo& save_info,
+                     int render_process_host_id,
+                     int render_view_id,
+                     URLRequestContextGetter* request_context_getter);
+
+  // Handlers for notifications sent from the download thread and run on
+  // the UI thread.
+  void OnStartDownload(DownloadCreateInfo* info);
+  void OnDownloadFinished(int id, int64 bytes_so_far);
+
   // Called only on UI thread to get the DownloadManager for a tab's profile.
   static DownloadManager* DownloadManagerFromRenderIds(int render_process_id,
                                                        int review_view_id);
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index fe4a463..4fe4d5a 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -84,6 +84,14 @@ bool CompareStartTime(DownloadItem* first, DownloadItem* second) {
   return first->start_time() > second->start_time();
 }
 
+void DeleteDownloadedFile(const FilePath& path) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
+  // Make sure we only delete files.
+  if (!file_util::DirectoryExists(path))
+    file_util::Delete(path, false);
+}
+
 }  // namespace
 
 // DownloadItem implementation -------------------------------------------------
@@ -1661,10 +1669,9 @@ void DownloadManager::FileSelectionCanceled(void* params) {
 void DownloadManager::DeleteDownload(const FilePath& path) {
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
-      NewRunnableFunction(&DownloadFileManager::DeleteFile, FilePath(path)));
+      NewRunnableFunction(&DeleteDownloadedFile, path));
 }
 
-
 void DownloadManager::DangerousDownloadValidated(DownloadItem* download) {
   DCHECK_EQ(DownloadItem::DANGEROUS, download->safety_state());
   download->set_safety_state(DownloadItem::DANGEROUS_BUT_VALIDATED);
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index f95ba7c..a9a8a6f 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -126,6 +126,58 @@ FilePath::StringType StripOrdinalNumber(
   return pure_file_name.substr(0, l_paren_index);
 }
 
+// Check whether we can save page as complete-HTML for the contents which
+// have specified a MIME type. Now only contents which have the MIME type
+// "text/html" can be saved as complete-HTML.
+bool CanSaveAsComplete(const std::string& contents_mime_type) {
+  return contents_mime_type == "text/html" ||
+         contents_mime_type == "application/xhtml+xml";
+}
+
+// File name is considered being consist of pure file name, dot and file
+// extension name. File name might has no dot and file extension, or has
+// multiple dot inside file name. The dot, which separates the pure file
+// name and file extension name, is last dot in the whole file name.
+// This function is for making sure the length of specified file path is not
+// great than the specified maximum length of file path and getting safe pure
+// file name part if the input pure file name is too long.
+// The parameter |dir_path| specifies directory part of the specified
+// file path. The parameter |file_name_ext| specifies file extension
+// name part of the specified file path (including start dot). The parameter
+// |max_file_path_len| specifies maximum length of the specified file path.
+// The parameter |pure_file_name| input pure file name part of the specified
+// file path. If the length of specified file path is great than
+// |max_file_path_len|, the |pure_file_name| will output new pure file name
+// part for making sure the length of specified file path is less than
+// specified maximum length of file path. Return false if the function can
+// not get a safe pure file name, otherwise it returns true.
+bool GetSafePureFileName(const FilePath& dir_path,
+                         const FilePath::StringType& file_name_ext,
+                         uint32 max_file_path_len,
+                         FilePath::StringType* pure_file_name) {
+  DCHECK(!pure_file_name->empty());
+  int available_length = static_cast<int>(
+    max_file_path_len - dir_path.value().length() - file_name_ext.length());
+  // Need an extra space for the separator.
+  if (!file_util::EndsWithSeparator(dir_path))
+    --available_length;
+
+  // Plenty of room.
+  if (static_cast<int>(pure_file_name->length()) <= available_length)
+    return true;
+
+  // Limited room. Truncate |pure_file_name| to fit.
+  if (available_length > 0) {
+    *pure_file_name =
+        pure_file_name->substr(0, available_length);
+    return true;
+  }
+
+  // Not enough room to even use a shortened |pure_file_name|.
+  pure_file_name->clear();
+  return false;
+}
+
 // This task creates a directory and then posts a task on the given thread.
 class CreateDownloadDirectoryTask : public Task {
  public:
@@ -1319,40 +1371,6 @@ bool SavePackage::IsSavableContents(const std::string& contents_mime_type) {
          net::IsSupportedJavascriptMimeType(contents_mime_type.c_str());
 }
 
-// Static
-bool SavePackage::CanSaveAsComplete(const std::string& contents_mime_type) {
-  return contents_mime_type == "text/html" ||
-         contents_mime_type == "application/xhtml+xml";
-}
-
-// Static
-bool SavePackage::GetSafePureFileName(const FilePath& dir_path,
-                                      const FilePath::StringType& file_name_ext,
-                                      uint32 max_file_path_len,
-                                      FilePath::StringType* pure_file_name) {
-  DCHECK(!pure_file_name->empty());
-  int available_length = static_cast<int>(
-    max_file_path_len - dir_path.value().length() - file_name_ext.length());
-  // Need an extra space for the separator.
-  if (!file_util::EndsWithSeparator(dir_path))
-    --available_length;
-
-  // Plenty of room.
-  if (static_cast<int>(pure_file_name->length()) <= available_length)
-    return true;
-
-  // Limited room. Truncate |pure_file_name| to fit.
-  if (available_length > 0) {
-    *pure_file_name =
-        pure_file_name->substr(0, available_length);
-    return true;
-  }
-
-  // Not enough room to even use a shortened |pure_file_name|.
-  pure_file_name->clear();
-  return false;
-}
-
 // SelectFileDialog::Listener interface.
 void SavePackage::FileSelected(const FilePath& path,
                                int index, void* params) {
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 8d842d7..ab78eb8 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -159,34 +159,7 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   // have the specified MIME type.
   static bool IsSavableContents(const std::string& contents_mime_type);
 
-  // Check whether we can save page as complete-HTML for the contents which
-  // have specified a MIME type. Now only contents which have the MIME type
-  // "text/html" can be saved as complete-HTML.
-  static bool CanSaveAsComplete(const std::string& contents_mime_type);
-
-  // File name is considered being consist of pure file name, dot and file
-  // extension name. File name might has no dot and file extension, or has
-  // multiple dot inside file name. The dot, which separates the pure file
-  // name and file extension name, is last dot in the whole file name.
-  // This function is for making sure the length of specified file path is not
-  // great than the specified maximum length of file path and getting safe pure
-  // file name part if the input pure file name is too long.
-  // The parameter |dir_path| specifies directory part of the specified
-  // file path. The parameter |file_name_ext| specifies file extension
-  // name part of the specified file path (including start dot). The parameter
-  // |max_file_path_len| specifies maximum length of the specified file path.
-  // The parameter |pure_file_name| input pure file name part of the specified
-  // file path. If the length of specified file path is great than
-  // |max_file_path_len|, the |pure_file_name| will output new pure file name
-  // part for making sure the length of specified file path is less than
-  // specified maximum length of file path. Return false if the function can
-  // not get a safe pure file name, otherwise it returns true.
-  static bool GetSafePureFileName(const FilePath& dir_path,
-                                  const FilePath::StringType& file_name_ext,
-                                  uint32 max_file_path_len,
-                                  FilePath::StringType* pure_file_name);
-
-  // SelectFileDialog::Listener interface.
+  // SelectFileDialog::Listener ------------------------------------------------
   virtual void FileSelected(const FilePath& path, int index, void* params);
   virtual void FileSelectionCanceled(void* params);