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

content/public/browser/browser_context.h

Index: content/public/browser/browser_context.h
===================================================================
--- content/public/browser/browser_context.h	(revision 140592)
+++ content/public/browser/browser_context.h	(working copy)
@@ -37,6 +37,7 @@
 
 class DOMStorageContext;
 class DownloadManager;
+class DownloadManagerDelegate;
 class GeolocationPermissionContext;
 class IndexedDBContext;
 class ResourceContext;
@@ -46,6 +47,7 @@
 // It lives on the UI thread.
 class CONTENT_EXPORT BrowserContext : public base::SupportsUserData {
  public:
+  static DownloadManager* GetDownloadManager(BrowserContext* browser_context);
   static quota::QuotaManager* GetQuotaManager(BrowserContext* browser_context);
   static DOMStorageContext* GetDOMStorageContext(
       BrowserContext* browser_context);
@@ -80,9 +82,6 @@
   // This doesn't belong here; http://crbug.com/89628
   virtual bool IsOffTheRecord() const = 0;
 
-  // Returns the DownloadManager associated with this context.
-  virtual content::DownloadManager* GetDownloadManager() = 0;
-
   // Returns the request context information associated with this context.  Call
   // this only on the UI thread, since it can send notifications that should
   // happen on the UI thread.
@@ -104,6 +103,10 @@
   // Returns the resource context.
   virtual ResourceContext* GetResourceContext() = 0;
 
+  // Returns the DownloadManagerDelegate for this context. This will be called
+  // once per context. It's valid to return NULL.
+  virtual DownloadManagerDelegate* GetDownloadManagerDelegate() = 0;

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

What's the conceptual reason for not having the DownloadManager accessible from
the BrowserContext?  My naive reading of the code makes it look like the
BrowserContext owns the DM now, and the DM is a content level concept, unlike
the DMD, which is an content/embedder interface concept.

(Asking not because I'm wanting this to change, but because I don't think I'll
be able to do a good review without understanding it.)

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

The reason is that BrowserContext is an interface that the embedder implements.
When we started the chrome/content split, chrome still had to provide DM and a
lot of other classes (see lines 51-60 above). This places a lot of burden on
embedders to create these objects in the right order etc. The goal has been to
make embedding content easier, so we've moved ownership/creation of any object
that content needs to content.

So now since content creates this object, we don't want to ask the embedder for
something that content created. That, and also to follow the content API's
convention of having the embedder interfaces not have any implementations.
correct
right, for now DMD is owned by the embedder. we can change this if you like.

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

Makes sense; thanks for the explanation.
Nope, that seems fine to me as long as we document it properly.

+
   // Returns the geolocation permission context for this context.
   virtual GeolocationPermissionContext* GetGeolocationPermissionContext() = 0;