Download code cleanup:

- make the code more object-oriented, make the object expose less accessors
- make some parts of code look more obvious, use existing helpers
- make the public interfaces slightly better (less ctor parameters)
- make some names slightly better

TEST=unit_tests, browser_tests, ui_tests
BUG=48913

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

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

15 files changed, 190 insertions, 203 deletions

diff --git a/chrome/browser/cocoa/download_item_controller.mm b/chrome/browser/cocoa/download_item_controller.mm
index 00e2110..1c77eee 100644
--- a/chrome/browser/cocoa/download_item_controller.mm
+++ b/chrome/browser/cocoa/download_item_controller.mm
@@ -244,10 +244,7 @@ class DownloadShelfContextMenuMac : public DownloadShelfContextMenu {
 
 - (IBAction)handleButtonClick:(id)sender {
   DownloadItem* download = bridge_->download_model()->download();
-  if (download->state() == DownloadItem::IN_PROGRESS)
-    download->set_open_when_complete(!download->open_when_complete());
-  else if (download->state() == DownloadItem::COMPLETE)
-    download_util::OpenDownload(download);
+  download->OpenDownload();
 }
 
 - (NSSize)preferredSize {
@@ -318,8 +315,7 @@ class DownloadShelfContextMenuMac : public DownloadShelfContextMenu {
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.save_download",
                            base::Time::Now() - creationTime_);
   // This will change the state and notify us.
-  bridge_->download_model()->download()->manager()->DangerousDownloadValidated(
-      bridge_->download_model()->download());
+  bridge_->download_model()->download()->DangerousDownloadValidated();
 }
 
 - (IBAction)discardDownload:(id)sender {
diff --git a/chrome/browser/download/download_file.cc b/chrome/browser/download/download_file.cc
index ebdaa1c..111236b 100644
--- a/chrome/browser/download/download_file.cc
+++ b/chrome/browser/download/download_file.cc
@@ -34,6 +34,8 @@ DownloadFile::DownloadFile(const DownloadCreateInfo* info)
 
 DownloadFile::~DownloadFile() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  if (in_progress())
+    Cancel();
   Close();
 }
 
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index d9ba1f2..c2cd5d0 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/download/download_file_manager.h"
 
 #include "base/file_util.h"
+#include "base/stl_util-inl.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
@@ -58,15 +59,7 @@ void DownloadFileManager::Shutdown() {
 // Cease download thread operations.
 void DownloadFileManager::OnShutdown() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  // Delete any partial downloads during shutdown.
-  for (DownloadFileMap::iterator it = downloads_.begin();
-       it != downloads_.end(); ++it) {
-    DownloadFile* download = it->second;
-    if (download->in_progress())
-      download->Cancel();
-    delete download;
-  }
-  downloads_.clear();
+  STLDeleteValues(&downloads_);
 }
 
 // Notifications sent from the download thread and run on the UI thread.
@@ -446,11 +439,11 @@ void DownloadFileManager::OnFinalDownloadName(int id,
                                               bool need_delete_crdownload,
                                               DownloadManager* manager) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  DownloadFileMap::iterator it = downloads_.find(id);
-  if (it == downloads_.end())
+
+  DownloadFile* download = GetDownloadFile(id);
+  if (!download)
     return;
 
-  DownloadFile* download = it->second;
   if (download->Rename(full_path, true)) {
 #if defined(OS_MACOSX)
     // Done here because we only want to do this once; see
@@ -474,7 +467,7 @@ void DownloadFileManager::OnFinalDownloadName(int id,
   // If the download has completed before we got this final name, we remove it
   // from our in progress map.
   if (!download->in_progress()) {
-    downloads_.erase(it);
+    downloads_.erase(id);
     delete download;
   }
 
@@ -489,11 +482,11 @@ void DownloadFileManager::OnFinalDownloadName(int id,
 // on the FILE thread.
 void DownloadFileManager::CancelDownloadOnRename(int id) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  DownloadFileMap::iterator it = downloads_.find(id);
-  if (it == downloads_.end())
+
+  DownloadFile* download = GetDownloadFile(id);
+  if (!download)
     return;
 
-  DownloadFile* download = it->second;
   DownloadManagerMap::iterator dmit = managers_.find(download->id());
   if (dmit != managers_.end()) {
     DownloadManager* dlm = dmit->second;
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index f4a6cb1..f67b1a1 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/history/download_types.h"
+#include "chrome/common/extensions/extension.h"
 
 namespace {
 
@@ -18,7 +19,8 @@ const int kUpdateTimeMs = 1000;
 }  // namespace
 
 // Constructor for reading from the history service.
-DownloadItem::DownloadItem(const DownloadCreateInfo& info)
+DownloadItem::DownloadItem(DownloadManager* download_manager,
+                           const DownloadCreateInfo& info)
     : id_(-1),
       full_path_(info.path),
       url_(info.url),
@@ -31,7 +33,7 @@ DownloadItem::DownloadItem(const DownloadCreateInfo& info)
       state_(static_cast<DownloadState>(info.state)),
       start_time_(info.start_time),
       db_handle_(info.db_handle),
-      manager_(NULL),
+      download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
       safety_state_(SAFE),
@@ -50,60 +52,75 @@ DownloadItem::DownloadItem(const DownloadCreateInfo& info)
   Init(false /* don't start progress timer */);
 }
 
-// Constructor for DownloadItem created via user action in the main thread.
-DownloadItem::DownloadItem(int32 download_id,
+// Constructing for a regular download:
+DownloadItem::DownloadItem(DownloadManager* download_manager,
+                           const DownloadCreateInfo& info,
+                           bool is_otr)
+    : id_(info.download_id),
+      full_path_(info.path),
+      path_uniquifier_(info.path_uniquifier),
+      url_(info.url),
+      referrer_url_(info.referrer_url),
+      mime_type_(info.mime_type),
+      original_mime_type_(info.original_mime_type),
+      total_bytes_(info.total_bytes),
+      received_bytes_(0),
+      start_tick_(base::TimeTicks::Now()),
+      state_(IN_PROGRESS),
+      start_time_(info.start_time),
+      db_handle_(DownloadManager::kUninitializedHandle),
+      download_manager_(download_manager),
+      is_paused_(false),
+      open_when_complete_(false),
+      safety_state_(info.is_dangerous ? DANGEROUS : SAFE),
+      auto_opened_(false),
+      original_name_(info.original_name),
+      render_process_id_(info.child_id),
+      request_id_(info.request_id),
+      save_as_(info.prompt_user_for_save_location),
+      is_otr_(is_otr),
+      is_extension_install_(info.is_extension_install),
+      name_finalized_(false),
+      is_temporary_(!info.save_info.file_path.empty()),
+      need_final_rename_(false) {
+  Init(true /* start progress timer */);
+}
+
+// Constructing for the "Save Page As..." feature:
+DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const FilePath& path,
-                           int path_uniquifier,
                            const GURL& url,
-                           const GURL& referrer_url,
-                           const std::string& mime_type,
-                           const std::string& original_mime_type,
-                           const FilePath& original_name,
-                           const base::Time start_time,
-                           int64 download_size,
-                           int render_process_id,
-                           int request_id,
-                           bool is_dangerous,
-                           bool save_as,
-                           bool is_otr,
-                           bool is_extension_install,
-                           bool is_temporary)
-    : id_(download_id),
+                           bool is_otr)
+    : id_(1),
       full_path_(path),
-      path_uniquifier_(path_uniquifier),
+      path_uniquifier_(0),
       url_(url),
-      referrer_url_(referrer_url),
-      mime_type_(mime_type),
-      original_mime_type_(original_mime_type),
-      total_bytes_(download_size),
+      referrer_url_(GURL()),
+      mime_type_(std::string()),
+      original_mime_type_(std::string()),
+      total_bytes_(0),
       received_bytes_(0),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS),
-      start_time_(start_time),
+      start_time_(base::Time::Now()),
       db_handle_(DownloadManager::kUninitializedHandle),
-      manager_(NULL),
+      download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
-      safety_state_(is_dangerous ? DANGEROUS : SAFE),
+      safety_state_(SAFE),
       auto_opened_(false),
-      original_name_(original_name),
-      render_process_id_(render_process_id),
-      request_id_(request_id),
-      save_as_(save_as),
+      original_name_(FilePath()),
+      render_process_id_(-1),
+      request_id_(-1),
+      save_as_(false),
       is_otr_(is_otr),
-      is_extension_install_(is_extension_install),
+      is_extension_install_(false),
       name_finalized_(false),
-      is_temporary_(is_temporary),
+      is_temporary_(false),
       need_final_rename_(false) {
   Init(true /* start progress timer */);
 }
 
-void DownloadItem::Init(bool start_timer) {
-  file_name_ = full_path_.BaseName();
-  if (start_timer)
-    StartProgressTimer();
-}
-
 DownloadItem::~DownloadItem() {
   state_ = REMOVING;
   UpdateObservers();
@@ -125,14 +142,45 @@ void DownloadItem::NotifyObserversDownloadFileCompleted() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadFileCompleted(this));
 }
 
-void DownloadItem::NotifyObserversDownloadOpened() {
-  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
+bool DownloadItem::CanOpenDownload() {
+  FilePath file_to_use = full_path();
+  if (!original_name().value().empty())
+    file_to_use = original_name();
+
+  return !Extension::IsExtension(file_to_use) &&
+         !download_manager_->IsExecutableFile(file_to_use);
+}
+
+bool DownloadItem::ShouldOpenFileBasedOnExtension() {
+  return download_manager_->ShouldOpenFileBasedOnExtension(full_path());
+}
+
+void DownloadItem::OpenFilesBasedOnExtension(bool open) {
+  return download_manager_->OpenFilesBasedOnExtension(full_path(), open);
+}
+
+void DownloadItem::OpenDownload() {
+  if (state() == DownloadItem::IN_PROGRESS) {
+    open_when_complete_ = !open_when_complete_;
+  } else if (state() == DownloadItem::COMPLETE) {
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
+    download_manager_->OpenDownload(this, NULL);
+  }
+}
+
+void DownloadItem::ShowDownloadInShell() {
+  download_manager_->ShowDownloadInShell(this);
+}
+
+void DownloadItem::DangerousDownloadValidated() {
+  download_manager_->DangerousDownloadValidated(this);
 }
 
-// If we've received more data than we were expecting (bad server info?), revert
-// to 'unknown size mode'.
 void DownloadItem::UpdateSize(int64 bytes_so_far) {
   received_bytes_ = bytes_so_far;
+
+  // If we've received more data than we were expecting (bad server info?),
+  // revert to 'unknown size mode'.
   if (received_bytes_ > total_bytes_)
     total_bytes_ = 0;
 }
@@ -168,7 +216,7 @@ void DownloadItem::Cancel(bool update_history) {
   UpdateObservers();
   StopProgressTimer();
   if (update_history)
-    manager_->DownloadCancelled(id_);
+    download_manager_->DownloadCancelled(id_);
 }
 
 void DownloadItem::Finished(int64 size) {
@@ -181,8 +229,8 @@ void DownloadItem::Remove(bool delete_on_disk) {
   Cancel(true);
   state_ = REMOVING;
   if (delete_on_disk)
-    manager_->DeleteDownload(full_path_);
-  manager_->RemoveDownload(db_handle_);
+    download_manager_->DeleteDownload(full_path_);
+  download_manager_->RemoveDownload(db_handle_);
   // We have now been deleted.
 }
 
@@ -220,7 +268,7 @@ void DownloadItem::Rename(const FilePath& full_path) {
 
 void DownloadItem::TogglePause() {
   DCHECK(state_ == IN_PROGRESS);
-  manager_->PauseDownload(id_, !is_paused_);
+  download_manager_->PauseDownload(id_, !is_paused_);
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
@@ -246,3 +294,9 @@ FilePath DownloadItem::GetFileName() const {
   }
   return original_name_;
 }
+
+void DownloadItem::Init(bool start_timer) {
+  file_name_ = full_path_.BaseName();
+  if (start_timer)
+    StartProgressTimer();
+}
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index f8c71d0..1a9a5ef 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -65,33 +65,22 @@ class DownloadItem {
   };
 
   // Constructing from persistent store:
-  explicit DownloadItem(const DownloadCreateInfo& info);
+  DownloadItem(DownloadManager* download_manager,
+               const DownloadCreateInfo& info);
 
-  // Constructing from user action:
-  DownloadItem(int32 download_id,
+  // Constructing for a regular download:
+  DownloadItem(DownloadManager* download_manager,
+               const DownloadCreateInfo& info,
+               bool is_otr);
+
+  // Constructing for the "Save Page As..." feature:
+  DownloadItem(DownloadManager* download_manager,
                const FilePath& path,
-               int path_uniquifier,
                const GURL& url,
-               const GURL& referrer_url,
-               const std::string& mime_type,
-               const std::string& original_mime_type,
-               const FilePath& original_name,
-               const base::Time start_time,
-               int64 download_size,
-               int render_process_id,
-               int request_id,
-               bool is_dangerous,
-               bool save_as,
-               bool is_otr,
-               bool is_extension_install,
-               bool is_temporary);
+               bool is_otr);
 
   ~DownloadItem();
 
-  void Init(bool start_timer);
-
-  // Public API
-
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
@@ -101,8 +90,26 @@ class DownloadItem {
   // Notifies our observers the downloaded file has been completed.
   void NotifyObserversDownloadFileCompleted();
 
-  // Notifies our observers the downloaded file has been opened.
-  void NotifyObserversDownloadOpened();
+  // Whether it is OK to open this download.
+  bool CanOpenDownload();
+
+  // Tests if a file type should be opened automatically.
+  bool ShouldOpenFileBasedOnExtension();
+
+  // Registers this file extension for automatic opening upon download
+  // completion if 'open' is true, or prevents the extension from automatic
+  // opening if 'open' is false.
+  void OpenFilesBasedOnExtension(bool open);
+
+  // Open the file associated with this download (wait for the download to
+  // complete if it is in progress).
+  void OpenDownload();
+
+  // Show the download via the OS shell.
+  void ShowDownloadInShell();
+
+  // Called when the user has validated the download of a dangerous file.
+  void DangerousDownloadValidated();
 
   // Received a new chunk of data
   void Update(int64 bytes_so_far);
@@ -163,8 +170,6 @@ class DownloadItem {
   base::Time start_time() const { return start_time_; }
   void set_db_handle(int64 handle) { db_handle_ = handle; }
   int64 db_handle() const { return db_handle_; }
-  DownloadManager* manager() const { return manager_; }
-  void set_manager(DownloadManager* manager) { manager_ = manager; }
   bool is_paused() const { return is_paused_; }
   void set_is_paused(bool pause) { is_paused_ = pause; }
   bool open_when_complete() const { return open_when_complete_; }
@@ -194,6 +199,8 @@ class DownloadItem {
   FilePath GetFileName() const;
 
  private:
+  void Init(bool start_timer);
+
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
@@ -252,7 +259,7 @@ class DownloadItem {
   base::RepeatingTimer<DownloadItem> update_timer_;
 
   // Our owning object
-  DownloadManager* manager_;
+  DownloadManager* download_manager_;
 
   // In progress downloads may be paused by the user, we note it here
   bool is_paused_;
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index ea740ed..ae17044 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -465,6 +465,7 @@ void DownloadManager::StartDownload(DownloadCreateInfo* info) {
 }
 
 void DownloadManager::CheckIfSuggestedPathExists(DownloadCreateInfo* info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   DCHECK(info);
 
   // Check writability of the suggested path. If we can't write to it, default
@@ -524,7 +525,6 @@ void DownloadManager::CheckIfSuggestedPathExists(DownloadCreateInfo* info) {
           info->suggested_path), "", 0);
   }
 
-  // Now we return to the UI thread.
   ChromeThread::PostTask(
       ChromeThread::UI, FROM_HERE,
       NewRunnableMethod(this,
@@ -564,39 +564,18 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
 
 void DownloadManager::ContinueStartDownload(DownloadCreateInfo* info,
                                             const FilePath& target_path) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   scoped_ptr<DownloadCreateInfo> infop(info);
   info->path = target_path;
 
-  DownloadItem* download = NULL;
-  DownloadMap::iterator it = in_progress_.find(info->download_id);
-  if (it == in_progress_.end()) {
-    download = new DownloadItem(info->download_id,
-                                info->path,
-                                info->path_uniquifier,
-                                info->url,
-                                info->referrer_url,
-                                info->mime_type,
-                                info->original_mime_type,
-                                info->original_name,
-                                info->start_time,
-                                info->total_bytes,
-                                info->child_id,
-                                info->request_id,
-                                info->is_dangerous,
-                                info->prompt_user_for_save_location,
-                                profile_->IsOffTheRecord(),
-                                info->is_extension_install,
-                                !info->save_info.file_path.empty());
-    download->set_manager(this);
-    in_progress_[info->download_id] = download;
-  } else {
-    NOTREACHED();  // Should not exist!
-    return;
-  }
+  DownloadItem* download = new DownloadItem(this, *info,
+                                            profile_->IsOffTheRecord());
+  DCHECK(!ContainsKey(in_progress_, info->download_id));
+  in_progress_[info->download_id] = download;
 
-  PendingFinishedMap::iterator pending_it =
-      pending_finished_downloads_.find(info->download_id);
-  bool download_finished = (pending_it != pending_finished_downloads_.end());
+  bool download_finished = ContainsKey(pending_finished_downloads_,
+                                       info->download_id);
 
   if (download_finished || info->is_dangerous) {
     // The download has already finished or the download is not safe.
@@ -622,7 +601,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(pending_it->first, pending_it->second);
+    DownloadFinished(info->download_id,
+                     pending_finished_downloads_[info->download_id]);
   }
 
   download->Rename(target_path);
@@ -762,6 +742,8 @@ void DownloadManager::DownloadFinished(int32 download_id, int64 size) {
 
 void DownloadManager::DownloadRenamedToFinalName(int download_id,
                                                  const FilePath& full_path) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
@@ -776,11 +758,11 @@ void DownloadManager::DownloadRenamedToFinalName(int download_id,
 }
 
 void DownloadManager::ContinueDownloadFinished(DownloadItem* download) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
   // If this was a dangerous download, it has now been approved and must be
   // removed from dangerous_finished_ so it does not get deleted on shutdown.
-  DownloadMap::iterator it = dangerous_finished_.find(download->id());
-  if (it != dangerous_finished_.end())
-    dangerous_finished_.erase(it);
+  dangerous_finished_.erase(download->id());
 
   // Handle chrome extensions explicitly and skip the shell execute.
   if (download->is_extension_install()) {
@@ -1495,10 +1477,9 @@ void DownloadManager::GenerateSafeFileName(const std::string& mime_type,
 void DownloadManager::OnQueryDownloadEntriesComplete(
     std::vector<DownloadCreateInfo>* entries) {
   for (size_t i = 0; i < entries->size(); ++i) {
-    DownloadItem* download = new DownloadItem(entries->at(i));
-    DCHECK(downloads_.find(download->db_handle()) == downloads_.end());
+    DownloadItem* download = new DownloadItem(this, entries->at(i));
+    DCHECK(!ContainsKey(downloads_, download->db_handle()));
     downloads_[download->db_handle()] = download;
-    download->set_manager(this);
   }
   NotifyModelChanged();
 }
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 28fa82e..ce7d76c 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -238,7 +238,7 @@ class DownloadManager : public base::RefCountedThreadSafe<DownloadManager>,
   // Deletes the specified path on the file thread.
   void DeleteDownload(const FilePath& path);
 
-  // Called when the user has validated the donwload of a dangerous file.
+  // Called when the user has validated the download of a dangerous file.
   void DangerousDownloadValidated(DownloadItem* download);
 
   // Used to make sure we have a safe file extension and filename for a
diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc
index fa6ab09..dc69d02 100644
--- a/chrome/browser/download/download_shelf.cc
+++ b/chrome/browser/download/download_shelf.cc
@@ -29,13 +29,10 @@ DownloadShelfContextMenu::~DownloadShelfContextMenu() {
 
 bool DownloadShelfContextMenu::IsCommandIdChecked(int command_id) const {
   switch (command_id) {
-    case OPEN_WHEN_COMPLETE: {
+    case OPEN_WHEN_COMPLETE:
       return download_->open_when_complete();
-    }
-    case ALWAYS_OPEN_TYPE: {
-      return download_->manager()->ShouldOpenFileBasedOnExtension(
-          download_->full_path());
-    }
+    case ALWAYS_OPEN_TYPE:
+      return download_->ShouldOpenFileBasedOnExtension();
   }
   return false;
 }
@@ -70,7 +67,7 @@ bool DownloadShelfContextMenu::IsCommandIdEnabled(int command_id) const {
     case OPEN_WHEN_COMPLETE:
       return download_->state() != DownloadItem::CANCELLED;
     case ALWAYS_OPEN_TYPE:
-      return download_util::CanOpenDownload(download_);
+      return download_->CanOpenDownload();
     case CANCEL:
       return download_->state() == DownloadItem::IN_PROGRESS;
     case TOGGLE_PAUSE:
@@ -83,14 +80,14 @@ bool DownloadShelfContextMenu::IsCommandIdEnabled(int command_id) const {
 void DownloadShelfContextMenu::ExecuteCommand(int command_id) {
   switch (command_id) {
     case SHOW_IN_FOLDER:
-      download_->manager()->ShowDownloadInShell(download_);
+      download_->ShowDownloadInShell();
       break;
     case OPEN_WHEN_COMPLETE:
-      download_util::OpenDownload(download_);
+      download_->OpenDownload();
       break;
     case ALWAYS_OPEN_TYPE: {
-      download_->manager()->OpenFilesBasedOnExtension(
-          download_->full_path(), !IsCommandIdChecked(ALWAYS_OPEN_TYPE));
+      download_->OpenFilesBasedOnExtension(
+          !IsCommandIdChecked(ALWAYS_OPEN_TYPE));
       break;
     }
     case CANCEL:
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index ee30f04..240ccee 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -29,7 +29,6 @@
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/common/chrome_paths.h"
-#include "chrome/common/extensions/extension.h"
 #include "chrome/common/time_format.h"
 #include "gfx/canvas_skia.h"
 #include "gfx/rect.h"
@@ -70,26 +69,6 @@ namespace download_util {
 // so that the animation ends faded out.
 static const int kCompleteAnimationCycles = 5;
 
-// Download opening ------------------------------------------------------------
-
-bool CanOpenDownload(DownloadItem* download) {
-  FilePath file_to_use = download->full_path();
-  if (!download->original_name().value().empty())
-    file_to_use = download->original_name();
-
-  return !Extension::IsExtension(file_to_use) &&
-         !download->manager()->IsExecutableFile(file_to_use);
-}
-
-void OpenDownload(DownloadItem* download) {
-  if (download->state() == DownloadItem::IN_PROGRESS) {
-    download->set_open_when_complete(!download->open_when_complete());
-  } else if (download->state() == DownloadItem::COMPLETE) {
-    download->NotifyObserversDownloadOpened();
-    download->manager()->OpenDownload(download, NULL);
-  }
-}
-
 // Download temporary file creation --------------------------------------------
 
 class DefaultDownloadDirectory {
diff --git a/chrome/browser/download/download_util.h b/chrome/browser/download/download_util.h
index 421ccce..dcd2441 100644
--- a/chrome/browser/download/download_util.h
+++ b/chrome/browser/download/download_util.h
@@ -35,15 +35,6 @@ struct DownloadSaveInfo;
 
 namespace download_util {
 
-// Download opening ------------------------------------------------------------
-
-// Whether it is OK to open this download.
-bool CanOpenDownload(DownloadItem* download);
-
-// Open the file associated with this download (wait for the download to
-// complete if it is in progress).
-void OpenDownload(DownloadItem* download);
-
 // Download temporary file creation --------------------------------------------
 
 // Return the default download directory.
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 7be57c2..3ec45b9 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -377,10 +377,9 @@ bool SavePackage::Init() {
   request_context_getter_ = profile->GetRequestContext();
 
   // Create the fake DownloadItem and display the view.
-  download_ = new DownloadItem(1, saved_main_file_path_, 0, page_url_, GURL(),
-      "", "", FilePath(), Time::Now(), 0, -1, -1, false, false,
-      profile->IsOffTheRecord(), false, false);
-  download_->set_manager(tab_contents_->profile()->GetDownloadManager());
+  download_ = new DownloadItem(tab_contents_->profile()->GetDownloadManager(),
+                               saved_main_file_path_, page_url_,
+                               profile->IsOffTheRecord());
   tab_contents_->OnStartDownload(download_);
 
   // Check save type and process the save page job.
diff --git a/chrome/browser/gtk/download_item_gtk.cc b/chrome/browser/gtk/download_item_gtk.cc
index 6744af0..7a68dc1 100644
--- a/chrome/browser/gtk/download_item_gtk.cc
+++ b/chrome/browser/gtk/download_item_gtk.cc
@@ -781,15 +781,7 @@ gboolean DownloadItemGtk::OnExpose(GtkWidget* widget, GdkEventExpose* e) {
 void DownloadItemGtk::OnClick(GtkWidget* widget) {
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.open_download",
                            base::Time::Now() - creation_time_);
-
-  DownloadItem* download = get_download();
-
-  if (download->state() == DownloadItem::IN_PROGRESS) {
-    download->set_open_when_complete(
-        !download->open_when_complete());
-  } else if (download->state() == DownloadItem::COMPLETE) {
-    download_util::OpenDownload(download);
-  }
+  get_download()->OpenDownload();
 }
 
 gboolean DownloadItemGtk::OnProgressAreaExpose(GtkWidget* widget,
@@ -858,7 +850,7 @@ gboolean DownloadItemGtk::OnDangerousPromptExpose(GtkWidget* widget,
 void DownloadItemGtk::OnDangerousAccept(GtkWidget* button) {
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.save_download",
                            base::Time::Now() - creation_time_);
-  get_download()->manager()->DangerousDownloadValidated(get_download());
+  get_download()->DangerousDownloadValidated();
 }
 
 void DownloadItemGtk::OnDangerousDecline(GtkWidget* button) {
diff --git a/chrome/browser/renderer_host/download_resource_handler.cc b/chrome/browser/renderer_host/download_resource_handler.cc
index 735a168..922fb4e 100644
--- a/chrome/browser/renderer_host/download_resource_handler.cc
+++ b/chrome/browser/renderer_host/download_resource_handler.cc
@@ -21,7 +21,7 @@ DownloadResourceHandler::DownloadResourceHandler(
     int render_view_id,
     int request_id,
     const GURL& url,
-    DownloadFileManager* manager,
+    DownloadFileManager* download_file_manager,
     URLRequest* request,
     bool save_as,
     const DownloadSaveInfo& save_info)
@@ -30,7 +30,7 @@ DownloadResourceHandler::DownloadResourceHandler(
       render_view_id_(render_view_id),
       url_(url),
       content_length_(0),
-      download_manager_(manager),
+      download_file_manager_(download_file_manager),
       request_(request),
       save_as_(save_as),
       save_info_(save_info),
@@ -63,8 +63,8 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   set_content_disposition(content_disposition);
   set_content_length(response->response_head.content_length);
 
-  download_id_ = download_manager_->GetNextId();
-  // |download_manager_| consumes (deletes):
+  download_id_ = download_file_manager_->GetNextId();
+  // |download_file_manager_| consumes (deletes):
   DownloadCreateInfo* info = new DownloadCreateInfo;
   info->url = url_;
   info->referrer_url = GURL(request_->referrer());
@@ -93,7 +93,7 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
       NewRunnableMethod(
-          download_manager_, &DownloadFileManager::StartDownload, info));
+          download_file_manager_, &DownloadFileManager::StartDownload, info));
   return true;
 }
 
@@ -133,7 +133,7 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read) {
   if (need_update) {
     ChromeThread::PostTask(
         ChromeThread::FILE, FROM_HERE,
-        NewRunnableMethod(download_manager_,
+        NewRunnableMethod(download_file_manager_,
                           &DownloadFileManager::UpdateDownload,
                           download_id_,
                           buffer_));
@@ -153,7 +153,7 @@ bool DownloadResourceHandler::OnResponseCompleted(
     const std::string& security_info) {
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(download_manager_,
+      NewRunnableMethod(download_file_manager_,
                         &DownloadFileManager::DownloadFinished,
                         download_id_,
                         buffer_));
diff --git a/chrome/browser/renderer_host/download_resource_handler.h b/chrome/browser/renderer_host/download_resource_handler.h
index e982cfa..adfff13 100644
--- a/chrome/browser/renderer_host/download_resource_handler.h
+++ b/chrome/browser/renderer_host/download_resource_handler.h
@@ -27,7 +27,7 @@ class DownloadResourceHandler : public ResourceHandler {
                           int render_view_id,
                           int request_id,
                           const GURL& url,
-                          DownloadFileManager* manager,
+                          DownloadFileManager* download_file_manager,
                           URLRequest* request,
                           bool save_as,
                           const DownloadSaveInfo& save_info);
@@ -77,7 +77,7 @@ class DownloadResourceHandler : public ResourceHandler {
   std::string content_disposition_;
   GURL url_;
   int64 content_length_;
-  DownloadFileManager* download_manager_;
+  DownloadFileManager* download_file_manager_;
   URLRequest* request_;
   bool save_as_;  // Request was initiated via "Save As" by the user.
   DownloadSaveInfo save_info_;
diff --git a/chrome/browser/views/download_item_view.cc b/chrome/browser/views/download_item_view.cc
index 348c86d..bc5f0c5 100644
--- a/chrome/browser/views/download_item_view.cc
+++ b/chrome/browser/views/download_item_view.cc
@@ -433,7 +433,7 @@ void DownloadItemView::ButtonPressed(
     UMA_HISTOGRAM_LONG_TIMES("clickjacking.save_download",
                              base::Time::Now() - creation_time_);
     // This will change the state and notify us.
-    download_->manager()->DangerousDownloadValidated(download_);
+    download_->DangerousDownloadValidated();
   }
 }
 
@@ -907,11 +907,7 @@ void DownloadItemView::OpenDownload() {
   // open downloads super quickly, we should be concerned about clickjacking.
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.open_download",
                            base::Time::Now() - creation_time_);
-  if (download_->state() == DownloadItem::IN_PROGRESS) {
-    download_->set_open_when_complete(!download_->open_when_complete());
-  } else if (download_->state() == DownloadItem::COMPLETE) {
-    download_util::OpenDownload(download_);
-  }
+  download_->OpenDownload();
 }
 
 void DownloadItemView::OnExtractIconComplete(IconManager::Handle handle,