Download code cleanup:

- remove unnecessary accessors
- make more methods private

TEST=unit_tests, browser_tests, ui_tests
BUG=48913

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

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

5 files changed, 51 insertions, 45 deletions

diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index d885576..f4a6cb1 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -137,6 +137,15 @@ void DownloadItem::UpdateSize(int64 bytes_so_far) {
     total_bytes_ = 0;
 }
 
+void DownloadItem::StartProgressTimer() {
+  update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdateTimeMs), this,
+                      &DownloadItem::UpdateObservers);
+}
+
+void DownloadItem::StopProgressTimer() {
+  update_timer_.Stop();
+}
+
 // Updates from the download thread may have been posted while this download
 // was being cancelled in the UI thread, so we'll accept them unless we're
 // complete.
@@ -177,15 +186,6 @@ void DownloadItem::Remove(bool delete_on_disk) {
   // We have now been deleted.
 }
 
-void DownloadItem::StartProgressTimer() {
-  update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdateTimeMs), this,
-                      &DownloadItem::UpdateObservers);
-}
-
-void DownloadItem::StopProgressTimer() {
-  update_timer_.Stop();
-}
-
 bool DownloadItem::TimeRemaining(base::TimeDelta* remaining) const {
   if (total_bytes_ <= 0)
     return false;  // We never received the content_length for this download.
@@ -225,6 +225,17 @@ void DownloadItem::TogglePause() {
   UpdateObservers();
 }
 
+void DownloadItem::OnNameFinalized() {
+  name_finalized_ = true;
+
+  // 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 we are still in downloading the
+  // file, delay the notification.
+  if (state() == DownloadItem::COMPLETE)
+    NotifyObserversDownloadFileCompleted();
+}
+
 FilePath DownloadItem::GetFileName() const {
   if (safety_state_ == DownloadItem::SAFE)
     return file_name_;
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index dc6ed2a..d404b3a 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -124,10 +124,6 @@ class DownloadItem {
   // |delete_file| is true, the file is deleted on the disk.
   void Remove(bool delete_file);
 
-  // Start/stop sending periodic updates to our observers
-  void StartProgressTimer();
-  void StopProgressTimer();
-
   // Simple calculation of the amount of time remaining to completion. Fills
   // |*remaining| with the amount of time remaining if successful. Fails and
   // returns false if we do not have the number of bytes or the speed so can
@@ -148,13 +144,12 @@ class DownloadItem {
   // Allow the user to temporarily pause a download or resume a paused download.
   void TogglePause();
 
+  // Called when the name of the download is finalized.
+  void OnNameFinalized();
+
   // Accessors
   DownloadState state() const { return state_; }
-  FilePath file_name() const { return file_name_; }
-  void set_file_name(const FilePath& name) { file_name_ = name; }
   FilePath full_path() const { return full_path_; }
-  void set_full_path(const FilePath& path) { full_path_ = path; }
-  int path_uniquifier() const { return path_uniquifier_; }
   void set_path_uniquifier(int uniquifier) { path_uniquifier_ = uniquifier; }
   GURL url() const { return url_; }
   GURL referrer_url() const { return referrer_url_; }
@@ -182,16 +177,11 @@ class DownloadItem {
   bool auto_opened() { return auto_opened_; }
   void set_auto_opened(bool auto_opened) { auto_opened_ = auto_opened; }
   FilePath original_name() const { return original_name_; }
-  void set_original_name(const FilePath& name) { original_name_ = name; }
   bool save_as() const { return save_as_; }
   bool is_otr() const { return is_otr_; }
   bool is_extension_install() const { return is_extension_install_; }
   bool name_finalized() const { return name_finalized_; }
-  void set_name_finalized(bool name_finalized) {
-    name_finalized_ = name_finalized;
-  }
   bool is_temporary() const { return is_temporary_; }
-  void set_is_temporary(bool is_temporary) { is_temporary_ = is_temporary; }
   bool need_final_rename() const { return need_final_rename_; }
   void set_need_final_rename(bool need_final_rename) {
     need_final_rename_ = need_final_rename;
@@ -206,6 +196,10 @@ class DownloadItem {
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
+  // Start/stop sending periodic updates to our observers
+  void StartProgressTimer();
+  void StopProgressTimer();
+
   // Request ID assigned by the ResourceDispatcherHost.
   int32 id_;
 
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index ed83daf..ea740ed 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -762,27 +762,16 @@ void DownloadManager::DownloadFinished(int32 download_id, int64 size) {
 
 void DownloadManager::DownloadRenamedToFinalName(int download_id,
                                                  const FilePath& full_path) {
-  DownloadMap::iterator it = downloads_.begin();
-  while (it != downloads_.end()) {
-    DownloadItem* download = it->second;
-    if (download->id() == download_id) {
-      // 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 we are still in downloading the
-      // file, delay the notification.
-      download->set_name_finalized(true);
-      if (download->state() == DownloadItem::COMPLETE)
-        download->NotifyObserversDownloadFileCompleted();
-
-      // This was called from DownloadFinished; continue to call
-      // ContinueDownloadFinished.
-      if (download->need_final_rename()) {
-        download->set_need_final_rename(false);
-        ContinueDownloadFinished(download);
-      }
-      return;
-    }
-    it++;
+  DownloadItem* item = GetDownloadItem(download_id);
+  if (!item)
+    return;
+  item->OnNameFinalized();
+
+  // This was called from DownloadFinished; continue to call
+  // ContinueDownloadFinished.
+  if (item->need_final_rename()) {
+    item->set_need_final_rename(false);
+    ContinueDownloadFinished(item);
   }
 }
 
@@ -1608,6 +1597,16 @@ void DownloadManager::NotifyModelChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, ModelChanged());
 }
 
+DownloadItem* DownloadManager::GetDownloadItem(int id) {
+  for (DownloadMap::iterator it = downloads_.begin();
+       it != downloads_.end(); ++it) {
+    DownloadItem* item = it->second;
+    if (item->id() == id)
+      return item;
+  }
+  return NULL;
+}
+
 // DownloadManager::OtherDownloadManagerObserver implementation ----------------
 
 DownloadManager::OtherDownloadManagerObserver::OtherDownloadManagerObserver(
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 1f23b4f..94f0dd7 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -383,6 +383,8 @@ class DownloadManager : public base::RefCountedThreadSafe<DownloadManager>,
   // Inform observers that the model has changed.
   void NotifyModelChanged();
 
+  DownloadItem* GetDownloadItem(int id);
+
   // 'downloads_' is map of all downloads in this profile. The key is the handle
   // returned by the history system, which is unique across sessions. This map
   // owns all the DownloadItems once they have been created in the history
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index 3f97452..ee30f04 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -308,7 +308,7 @@ void DragDownload(const DownloadItem* download,
   OSExchangeData data;
 
   if (icon) {
-    drag_utils::CreateDragImageForFile(download->file_name().value(), icon,
+    drag_utils::CreateDragImageForFile(download->GetFileName().value(), icon,
                                        &data);
   }
 
@@ -322,7 +322,7 @@ void DragDownload(const DownloadItem* download,
   // Add URL so that we can load supported files when dragged to TabContents.
   if (net::IsSupportedMimeType(mime_type)) {
     data.SetURL(GURL(WideToUTF8(full_path.ToWStringHack())),
-                     download->file_name().ToWStringHack());
+                     download->GetFileName().ToWStringHack());
   }
 
 #if defined(OS_WIN)