Move creation and ownership of DownloadManager from the embedder to content. This matches all the o…

chrome/browser/download/download_service.cc

Index: chrome/browser/download/download_service.cc
===================================================================
--- chrome/browser/download/download_service.cc	(revision 140592)
+++ chrome/browser/download/download_service.cc	(working copy)
@@ -14,7 +14,9 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/download_manager.h"
 
+using content::BrowserContext;
 using content::DownloadManager;
+using content::DownloadManagerDelegate;
 
 DownloadService::DownloadService(Profile* profile)
     : download_manager_created_(false),
@@ -25,37 +27,39 @@
 
 void DownloadService::OnManagerCreated(
     const DownloadService::OnManagerCreatedCallback& cb) {
+  DownloadManager* dm = BrowserContext::GetDownloadManager(profile_);

[Randy Smith (Not in Mondays), 2012/06/06 15:35:52]

This looks like it will unilaterally create a download manager whenever
OnManagerCreated is called.  This is a behavior we were trying to avoid--there's
a fair amount of machinery (spec. reading in old downloads, of which there are
sometimes a metric ton in the history) that gets invoked when a download manager
is created, and it's not needed for every browser invocation.  Am I wrong?  If
not, can we keep the old semantics somehow?

[jam, 2012/06/06 15:49:36]

my bad, I meant to call GetDownloadManager in the if statement. I'll send you a
fix.

[Randy Smith (Not in Mondays), 2012/06/06 16:34:45]

Cool, thanks.  I'll probably ask Ben to review it :-} (which I suspect will give
you shorter turn around time).

   if (download_manager_created_) {
-    cb.Run(manager_.get());
+    cb.Run(dm);
   } else {
     on_manager_created_callbacks_.push_back(cb);
   }
 }
 
-DownloadManager* DownloadService::GetDownloadManager() {
-  if (!download_manager_created_) {
-    // In case the delegate has already been set by
-    // SetDownloadManagerDelegateForTesting.
-    if (!manager_delegate_.get())
-      manager_delegate_ = new ChromeDownloadManagerDelegate(profile_);
-    manager_ = DownloadManager::Create(manager_delegate_.get(),
-                                       g_browser_process->net_log());
-    manager_->Init(profile_);
-    manager_delegate_->SetDownloadManager(manager_);
+DownloadManagerDelegate* DownloadService::GetDownloadManagerDelegate() {

[Randy Smith (Not in Mondays), 2012/06/06 15:51:59]

This basic change (shifting from GetDownloadManager() to
GetDownloadManagerDelegate()) confuses me a bit as regards to ownership between
the download manager and the delegate.  Could you include a comment somewhere
(top of this file would be fine) as to which class owns which and what the
shutdown pathway looks like?  (The last comment partially based on the addition
of ProfileShutdown() to the delegate).

[jam, 2012/06/06 16:06:15]

I've added a comment to browser_context.h, since that's the boundary of where
content asks the embedder for it.

+  DCHECK(!download_manager_created_);
+  download_manager_created_ = true;
 
-    // Include this download manager in the set monitored by the
-    // global status updater.
-    g_browser_process->download_status_updater()->AddManager(manager_);
+  // In case the delegate has already been set by
+  // SetDownloadManagerDelegateForTesting.
+  if (!manager_delegate_.get())
+    manager_delegate_ = new ChromeDownloadManagerDelegate(profile_);
 
-    download_manager_created_ = true;
-    for (std::vector<OnManagerCreatedCallback>::iterator cb
-         = on_manager_created_callbacks_.begin();
-         cb != on_manager_created_callbacks_.end(); ++cb) {
-      cb->Run(manager_.get());
-    }
-    on_manager_created_callbacks_.clear();
+  DownloadManager* dm = BrowserContext::GetDownloadManager(profile_);
+  manager_delegate_->SetDownloadManager(dm);
+
+  // Include this download manager in the set monitored by the
+  // global status updater.
+  g_browser_process->download_status_updater()->AddManager(dm);
+
+  download_manager_created_ = true;
+  for (std::vector<OnManagerCreatedCallback>::iterator cb
+        = on_manager_created_callbacks_.begin();
+        cb != on_manager_created_callbacks_.end(); ++cb) {
+    cb->Run(dm);
   }
-  return manager_.get();
+  on_manager_created_callbacks_.clear();
+
+  return manager_delegate_.get();
 }
 
 bool DownloadService::HasCreatedDownloadManager() {
@@ -63,7 +67,8 @@
 }
 
 int DownloadService::DownloadCount() const {
-  return download_manager_created_ ? manager_->InProgressCount() : 0;
+  DownloadManager* dm = BrowserContext::GetDownloadManager(profile_);
+  return download_manager_created_ ? dm->InProgressCount() : 0;
 }
 
 // static
@@ -86,27 +91,15 @@
 void DownloadService::SetDownloadManagerDelegateForTesting(
     ChromeDownloadManagerDelegate* new_delegate) {
   // Guarantee everything is properly initialized.
-  GetDownloadManager();
-
-  manager_->SetDownloadManagerDelegate(new_delegate);
-  new_delegate->SetDownloadManager(manager_);
+  DownloadManager* dm = BrowserContext::GetDownloadManager(profile_);
+  dm->SetDownloadManagerDelegate(new_delegate);
+  new_delegate->SetDownloadManager(dm);
   manager_delegate_ = new_delegate;
 }
 
 void DownloadService::Shutdown() {
-  if (manager_.get()) {
-    manager_->Shutdown();
-
-    // The manager reference can be released any time after shutdown;
-    // it will be destroyed when the last reference is released on the
-    // FILE thread.
-    // Resetting here will guarantee that any attempts to get the
-    // DownloadManager after shutdown will return null.
-    //
-    // TODO(rdsmith): Figure out how to guarantee when the last reference
-    // will be released and make DownloadManager not RefCountedThreadSafe<>.
-    manager_.release();
-  }
-  if (manager_delegate_.get())
+  if (manager_delegate_.get()) {
+    manager_delegate_->ProfileShutdown();
     manager_delegate_.release();
+  }
 }