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

Move creation and ownership of DownloadManager from the embedder to content. This matches all the other objects that content depends on.

In a followup change, I'll make content support NULL DownloadManagerDelegates to match the rest of the delegate interfaces.

BUG=98716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140761

[jam, 2012-06-05 23:29:48]

(most of the changes are just changing how one gets a DownloadManager)

[Avi (use Gerrit), 2012-06-05 23:34:41]

http://codereview.chromium.org/10535026/diff/1/content/browser/browser_contex...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10535026/diff/1/content/browser/browser_contex...
content/browser/browser_context.cc:142: DownloadManager*
BrowserContext::GetDownloadManager(
This DCHECKs if you call it more than once? Don't you call it all over the
place?

[Avi (use Gerrit), 2012-06-05 23:38:22]

http://codereview.chromium.org/10535026/diff/1/content/public/browser/browser...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/10535026/diff/1/content/public/browser/browser...
content/public/browser/browser_context.h:50: static DownloadManager*
GetDownloadManager(BrowserContext* browser_context);
My concern is that this is a call-once-only function among the others
immediately below which you can call all you want.

[jam, 2012-06-05 23:47:15]

http://codereview.chromium.org/10535026/diff/1/content/browser/browser_contex...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10535026/diff/1/content/browser/browser_contex...
content/browser/browser_context.cc:142: DownloadManager*
BrowserContext::GetDownloadManager(
On 2012/06/05 23:34:41, Avi wrote:
> This DCHECKs if you call it more than once? Don't you call it all over the
> place?

I hadn't tested it yet (was parallelizing review+testing) :) fixed, verified
basic downloading now but going through all the tests still.

[Avi (use Gerrit), 2012-06-06 14:49:44]

lgtm with nit

http://codereview.chromium.org/10535026/diff/2090/content/browser/browser_con...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10535026/diff/2090/content/browser/browser_con...
content/browser/browser_context.cc:144:
if(!context->GetUserData(kDownloadManagerKeyName)) {
style nit: space between if (

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

In process of doing a review but I think this is real, so sending this out in
hopes you'll see it before you land.  CCing Ben, as he did the original
implementation of this functionality.

Any chance I (or Ben or Asanka) could be included in reviewing download system
changes in the future?

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:30: DownloadManager* dm =
BrowserContext::GetDownloadManager(profile_);
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?

[tfarina, 2012-06-06 15:44:15]

https://chromiumcodereview.appspot.com/10535026/diff/2090/content/public/brow...
File content/public/browser/download_manager.h (right):

https://chromiumcodereview.appspot.com/10535026/diff/2090/content/public/brow...
content/public/browser/download_manager.h:69: virtual void
SetDelegate(DownloadManagerDelegate* delegate) = 0;
There is already a SetDownloadManagerDelegate in this interface, see line 238

https://chromiumcodereview.appspot.com/10535026/diff/2090/content/public/brow...
content/public/browser/download_manager.h:234: virtual DownloadManagerDelegate*
delegate() const = 0;
nit: s/delegate/GetDelegate

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

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:30: DownloadManager* dm =
BrowserContext::GetDownloadManager(profile_);
On 2012/06/06 15:35:52, rdsmith wrote:
> 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?

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 15:51:59]

A couple more comments, mostly about clarifying interface contracts.

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:38: DownloadManagerDelegate*
DownloadService::GetDownloadManagerDelegate() {
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).

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
content/public/browser/browser_context.h:108: virtual DownloadManagerDelegate*
GetDownloadManagerDelegate() = 0;
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.)

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/down...
content/public/browser/download_manager.h:68: // its Shutdown method being
called.
Which is under the control of the DownloadManager?  I don't see how the contract
is useful to the DM/Delegate interface unless there's also a guarantee that the
delegate's Shutdown method won't be called except by the DM, and the DM is
required to call it?

[jam, 2012-06-06 15:53:04]

https://chromiumcodereview.appspot.com/10535026/diff/2090/content/public/brow...
File content/public/browser/download_manager.h (right):

https://chromiumcodereview.appspot.com/10535026/diff/2090/content/public/brow...
content/public/browser/download_manager.h:69: virtual void
SetDelegate(DownloadManagerDelegate* delegate) = 0;
On 2012/06/06 15:44:15, tfarina wrote:
> There is already a SetDownloadManagerDelegate in this interface, see line 238

thanks, I'll remove SetDownloadManagerDelegate

[jam, 2012-06-06 16:06:14]

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:38: DownloadManagerDelegate*
DownloadService::GetDownloadManagerDelegate() {
On 2012/06/06 15:51:59, rdsmith wrote:
> 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).

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

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
content/public/browser/browser_context.h:108: virtual DownloadManagerDelegate*
GetDownloadManagerDelegate() = 0;
On 2012/06/06 15:51:59, rdsmith wrote:
> What's the conceptual reason for not having the DownloadManager accessible
from
> the BrowserContext?

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.

> My naive reading of the code makes it look like the
> BrowserContext owns the DM now, and the DM is a content level concept, 

correct

> unlike
> the DMD, which is an content/embedder interface concept.

right, for now DMD is owned by the embedder. we can change this if you like.
> 
> (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.)

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/down...
content/public/browser/download_manager.h:68: // its Shutdown method being
called.
On 2012/06/06 15:51:59, rdsmith wrote:
> Which is under the control of the DownloadManager?  I don't see how the
contract
> is useful to the DM/Delegate interface unless there's also a guarantee that
the
> delegate's Shutdown method won't be called except by the DM, and the DM is
> required to call it?

I've clarified the comment to say that Shutdown is called by DM.

[Timur Iskhodzhanov, 2012-06-06 16:07:07]

Valgrind is barking on a leak report
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...
Mind taking a look?

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

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
File chrome/browser/download/download_service.cc (right):

http://codereview.chromium.org/10535026/diff/2090/chrome/browser/download/dow...
chrome/browser/download/download_service.cc:30: DownloadManager* dm =
BrowserContext::GetDownloadManager(profile_);
On 2012/06/06 15:49:36, John Abd-El-Malek wrote:
> On 2012/06/06 15:35:52, rdsmith wrote:
> > 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?
> 
> my bad, I meant to call GetDownloadManager in the if statement. I'll send you
a
> fix.

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

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
File content/public/browser/browser_context.h (right):

http://codereview.chromium.org/10535026/diff/2090/content/public/browser/brow...
content/public/browser/browser_context.h:108: virtual DownloadManagerDelegate*
GetDownloadManagerDelegate() = 0;
On 2012/06/06 16:06:15, John Abd-El-Malek wrote:
> On 2012/06/06 15:51:59, rdsmith wrote:
> > What's the conceptual reason for not having the DownloadManager accessible
> from
> > the BrowserContext?
> 
> 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.

Makes sense; thanks for the explanation.

> 
> > My naive reading of the code makes it look like the
> > BrowserContext owns the DM now, and the DM is a content level concept, 
> 
> correct
> 
> > unlike
> > the DMD, which is an content/embedder interface concept.
> 
> right, for now DMD is owned by the embedder. we can change this if you like.

Nope, that seems fine to me as long as we document it properly.

> > 
> > (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.)
>