Download code cleanup:

Changed the code to be more object-oriented. Instead of exposing 150 accessors
we should do as much as possible inside the object, exposing a nice API.
DownloadFile just got a small step closer to that.

Also, reduce amount of state duplication. For example, information about
the download progress was both in DownloadFile and DownloadFileManager
(maybe it's still somewhere else). The download path information
is still duplicated, removing it is going to be harder.

Finally, removed completely unused state variables from DownloadFile.

TEST=unit_tests, browser_tests, ui_tests
BUG=48913

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

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

3 files changed, 71 insertions, 48 deletions

diff --git a/chrome/browser/download/download_file.cc b/chrome/browser/download/download_file.cc
index 837d37e..ebdaa1c 100644
--- a/chrome/browser/download/download_file.cc
+++ b/chrome/browser/download/download_file.cc
@@ -6,6 +6,7 @@
 
 #include "base/file_util.h"
 #include "build/build_config.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/history/download_types.h"
@@ -24,38 +25,39 @@ DownloadFile::DownloadFile(const DownloadCreateInfo* info)
       referrer_url_(info->referrer_url),
       id_(info->download_id),
       child_id_(info->child_id),
-      render_view_id_(info->render_view_id),
       request_id_(info->request_id),
-      bytes_so_far_(0),
       full_path_(info->save_info.file_path),
       path_renamed_(false),
-      in_progress_(true),
-      dont_sleep_(true),
-      save_info_(info->save_info) {
+      dont_sleep_(true) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
 }
 
 DownloadFile::~DownloadFile() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   Close();
 }
 
 bool DownloadFile::Initialize() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   if (!full_path_.empty() ||
       download_util::CreateTemporaryFileForDownload(&full_path_))
     return Open();
   return false;
 }
 
-bool DownloadFile::AppendDataToFile(const char* data, int data_len) {
-  if (file_stream_.get()) {
-    // FIXME bug 595247: handle errors on file writes.
-    size_t written = file_stream_->Write(data, data_len, NULL);
-    bytes_so_far_ += written;
-    return true;
-  }
-  return false;
+bool DownloadFile::AppendDataToFile(const char* data, size_t data_len) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
+  if (!file_stream_.get())
+    return false;
+
+  // FIXME bug 595247: handle errors on file writes.
+  size_t written = file_stream_->Write(data, data_len, NULL);
+  return (written == data_len);
 }
 
 void DownloadFile::Cancel() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   Close();
   if (!full_path_.empty())
     file_util::Delete(full_path_, false);
@@ -63,13 +65,19 @@ void DownloadFile::Cancel() {
 
 // The UI has provided us with our finalized name.
 bool DownloadFile::Rename(const FilePath& new_path, bool is_final_rename) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
+  // Save the information whether the download is in progress because
+  // it will be overwritten by closing the file.
+  bool saved_in_progress = in_progress();
+
   // If the new path is same as the old one, there is no need to perform the
   // following renaming logic.
   if (new_path == full_path_) {
     path_renamed_ = is_final_rename;
 
     // Don't close the file if we're not done (finished or canceled).
-    if (!in_progress_)
+    if (!saved_in_progress)
       Close();
 
     return true;
@@ -111,7 +119,7 @@ bool DownloadFile::Rename(const FilePath& new_path, bool is_final_rename) {
   path_renamed_ = is_final_rename;
 
   // We don't need to re-open the file if we're done (finished or canceled).
-  if (!in_progress_)
+  if (!saved_in_progress)
     return true;
 
   if (!Open())
@@ -129,7 +137,13 @@ void DownloadFile::DeleteCrDownload() {
   file_util::Delete(crdownload, false);
 }
 
+void DownloadFile::Finish() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  Close();
+}
+
 void DownloadFile::Close() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   if (file_stream_.get()) {
 #if defined(OS_CHROMEOS)
     // Currently we don't really care about the return value, since if it fails
@@ -142,6 +156,7 @@ void DownloadFile::Close() {
 }
 
 bool DownloadFile::Open() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   DCHECK(!full_path_.empty());
 
   // Create a new file steram if it is not provided.
@@ -162,6 +177,7 @@ bool DownloadFile::Open() {
 }
 
 void DownloadFile::AnnotateWithSourceInformation() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
 #if defined(OS_WIN)
   // Sets the Zone to tell Windows that this file comes from the internet.
   // We ignore the return value because a failure is not fatal.
@@ -173,3 +189,13 @@ void DownloadFile::AnnotateWithSourceInformation() {
                                          referrer_url_);
 #endif
 }
+
+void DownloadFile::CancelDownloadRequest(ResourceDispatcherHost* rdh) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  ChromeThread::PostTask(
+      ChromeThread::IO, FROM_HERE,
+      NewRunnableFunction(&download_util::CancelDownloadRequest,
+                          rdh,
+                          child_id_,
+                          request_id_));
+}
diff --git a/chrome/browser/download/download_file.h b/chrome/browser/download/download_file.h
index 6fd605e..9c9fdf6 100644
--- a/chrome/browser/download/download_file.h
+++ b/chrome/browser/download/download_file.h
@@ -18,6 +18,7 @@
 #include "googleurl/src/gurl.h"
 
 struct DownloadCreateInfo;
+class ResourceDispatcherHost;
 
 // These objects live exclusively on the download thread and handle the writing
 // operations for one download. These objects live only for the duration that
@@ -30,8 +31,9 @@ class DownloadFile {
 
   bool Initialize();
 
-  // Write a new chunk of data to the file. Returns true on success.
-  bool AppendDataToFile(const char* data, int data_len);
+  // Write a new chunk of data to the file. Returns true on success (all bytes
+  // written to the file).
+  bool AppendDataToFile(const char* data, size_t data_len);
 
   // Abort the download and automatically close the file.
   void Cancel();
@@ -41,6 +43,9 @@ class DownloadFile {
   // Marked virtual for testing.
   virtual bool Rename(const FilePath& full_path, bool is_final_rename);
 
+  // Indicate that the download has finished. No new data will be received.
+  void Finish();
+
   // Informs the OS that this file came from the internet.
   void AnnotateWithSourceInformation();
 
@@ -48,16 +53,13 @@ class DownloadFile {
   // Marked virtual for testing.
   virtual void DeleteCrDownload();
 
+  // Cancels the download request associated with this file.
+  void CancelDownloadRequest(ResourceDispatcherHost* rdh);
+
   // Accessors.
-  int64 bytes_so_far() const { return bytes_so_far_; }
   int id() const { return id_; }
-  FilePath full_path() const { return full_path_; }
-  int child_id() const { return child_id_; }
-  int render_view_id() const { return render_view_id_; }
-  int request_id() const { return request_id_; }
   bool path_renamed() const { return path_renamed_; }
   bool in_progress() const { return file_stream_ != NULL; }
-  void set_in_progress(bool in_progress) { in_progress_ = in_progress; }
 
  private:
   // Open or Close the OS file stream. The stream is opened in the constructor
@@ -81,30 +83,19 @@ class DownloadFile {
 
   // IDs for looking up the tab we are associated with.
   int child_id_;
-  int render_view_id_;
 
   // Handle for informing the ResourceDispatcherHost of a UI based cancel.
   int request_id_;
 
-  // Amount of data received up to this point. We may not know in advance how
-  // much data to expect since some servers don't provide that information.
-  int64 bytes_so_far_;
-
   // Full path to the downloaded file.
   FilePath full_path_;
 
   // Whether the download is still using its initial temporary path.
   bool path_renamed_;
 
-  // Whether the download is still receiving data.
-  bool in_progress_;
-
   // RAII handle to keep the system from sleeping while we're downloading.
   PowerSaveBlocker dont_sleep_;
 
-  // The provider used to save the download data.
-  DownloadSaveInfo save_info_;
-
   DISALLOW_COPY_AND_ASSIGN(DownloadFile);
 };
 
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index ea0e42b..d9ba1f2 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -202,7 +202,8 @@ void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
 
   DCHECK(GetDownloadFile(info->download_id) == NULL);
   downloads_[info->download_id] = download;
-  info->path = download->full_path();
+  // TODO(phajdan.jr): fix the duplication of path info below.
+  info->path = info->save_info.file_path;
   {
     AutoLock lock(progress_lock_);
     ui_progress_[info->download_id] = info->received_bytes;
@@ -226,18 +227,24 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
     contents.swap(buffer->contents);
   }
 
+  // Keep track of how many bytes we have successfully saved to update
+  // our progress status in the UI.
+  int64 progress_bytes = 0;
+
   DownloadFile* download = GetDownloadFile(id);
   for (size_t i = 0; i < contents.size(); ++i) {
     net::IOBuffer* data = contents[i].first;
     const int data_len = contents[i].second;
-    if (download)
-      download->AppendDataToFile(data->data(), data_len);
+    if (download) {
+      if (download->AppendDataToFile(data->data(), data_len))
+        progress_bytes += data_len;
+    }
     data->Release();
   }
 
   if (download) {
     AutoLock lock(progress_lock_);
-    ui_progress_[download->id()] = download->bytes_so_far();
+    ui_progress_[download->id()] += progress_bytes;
   }
 }
 
@@ -247,13 +254,19 @@ void DownloadFileManager::DownloadFinished(int id, DownloadBuffer* buffer) {
   DownloadFileMap::iterator it = downloads_.find(id);
   if (it != downloads_.end()) {
     DownloadFile* download = it->second;
-    download->set_in_progress(false);
+    download->Finish();
+
+    int64 download_size = -1;
+    {
+      AutoLock lock(progress_lock_);
+      download_size = ui_progress_[download->id()];
+    }
 
     ChromeThread::PostTask(
         ChromeThread::UI, FROM_HERE,
         NewRunnableMethod(
             this, &DownloadFileManager::OnDownloadFinished,
-            id, download->bytes_so_far()));
+            id, download_size));
 
     // We need to keep the download around until the UI thread has finalized
     // the name.
@@ -277,8 +290,6 @@ void DownloadFileManager::CancelDownload(int id) {
   DownloadFileMap::iterator it = downloads_.find(id);
   if (it != downloads_.end()) {
     DownloadFile* download = it->second;
-    download->set_in_progress(false);
-
     download->Cancel();
 
     ChromeThread::PostTask(
@@ -490,11 +501,6 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
         ChromeThread::UI, FROM_HERE,
         NewRunnableMethod(dlm, &DownloadManager::DownloadCancelled, id));
   } else {
-    ChromeThread::PostTask(
-        ChromeThread::IO, FROM_HERE,
-        NewRunnableFunction(&download_util::CancelDownloadRequest,
-                            resource_dispatcher_host_,
-                            download->child_id(),
-                            download->request_id()));
+    download->CancelDownloadRequest(resource_dispatcher_host_);
   }
 }