Rewrite download manager unit to be actual unit tests.

Rewrite download manager unit to be actual unit tests.

Note that this isn't intended to be complete coverage; it's an attempt to preserve coverage from the old tests while making these "real" unit tests, i.e. with every class except for the main one being tested mocked.  Thorough unit tests are intended for the future after we're more completely done with refactoring.  

BUG=107264
BUG=105200
BUG=110886
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141674

[Randy Smith (Not in Mondays), 2012-06-06 20:01:19]

Asanka, could you take a look?  Two caveats:
* The rewrite of dmiu.cc is a pretty close to from scratch replacement; you may
want to just review the design doc I sent you and then confirm it matches the
target of that doc appropriately.

* This'll need to be merged with John's recent change, but I don't expect that
to make changes outside of the creation of the DownloadManager.

[jam, 2012-06-07 03:21:45]

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
File content/browser/download/download_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl.h:30: class DownloadItemFactory {
nit: can this be put in new h/cc files for readability?

[asanka, 2012-06-07 17:04:53]

Looks much cleaner!

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
File content/browser/download/download_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl.cc:217: return new
DownloadManagerImpl(delegate, file_manager, NULL, net_log);
Why not create the DownloadItemFactoryImpl instance here?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
File content/browser/download/download_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl.h:57:
DownloadManagerImpl(content::DownloadManagerDelegate* delegate,
Nit: Document ownership semantics?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:11: #include
"base/memory/scoped_ptr.h"
Duplicate

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:98: const
DownloadRequestHandle& request_handle) {
Nit: OVERRIDE

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:128: class Observer {
Nit: Would it be cleaner if MockDownloadItemFactory maintained and exposed a
download item map of items it has created?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:260:
next_download_id_(0) {
Nit: Initialize scalars (mock_download_item_factory_)

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:305:
mock_download_item_factory_ = NULL; // Owned by DownloadManager.
Could we test that it gets deleted? Perhaps stuff in a WeakPtr?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:315: int
AddItemToManager() {
Why not return a MockDownloadItem& directly?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:557:
download_manager_->DownloadCancelled(&item);
Check GetActiveDownloadItem() afterwards?

https://chromiumcodereview.appspot.com/10344024/diff/15001/content/browser/do...
content/browser/download/download_manager_impl_unittest.cc:579:
download_manager_->DownloadCancelled(&item);
Check GetActiveDownloadItem() afterwards?

[jam, 2012-06-07 21:35:40]

http://codereview.chromium.org/10344024/diff/13002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10344024/diff/13002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:168: content::DownloadItem*
DownloadItemFactoryImpl::CreatePersistedItem(
this is personal style so feel free to ignore, but in cases like this I prefer
to put the implementation inline. one knows what the functions are from the
interface header, which is where the function documentation is anyways. so
splitting it makes the code take longer to scan

[Randy Smith (Not in Mondays), 2012-06-07 23:27:21]

PTAL.  

High-level request for opinion: I recreated the DownloadManagerImpl::Create()
function, so that I could allow the regular code to just specify a NetLog, and
the unit test code to also specify file manager and item factory.  I'd be
interested in opinions as proper ways to do this separation.  I've also thought
of doing it via two constructors, but that seems to run afoul of the style
guides "no default arguments" rule.  Possibly if the unit test constructor was
private?

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:217: return new
DownloadManagerImpl(delegate, file_manager, NULL, net_log);
On 2012/06/07 17:04:53, asanka wrote:
> Why not create the DownloadItemFactoryImpl instance here?

Fair question, and I'll try to implement the spirt of it, but actually
implementation is dependent on how I resolve the unit-test targeted constructor
vs. externally targeted constructor pattern (see top level question to John).

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.h:30: class DownloadItemFactory {
On 2012/06/07 03:21:45, John Abd-El-Malek wrote:
> nit: can this be put in new h/cc files for readability? 

I moved the declaration out into its own .h file.  The default implementation is
specific to the download manager and in the anonymous namespace currently, so it
didn't feel appropriate to me to move that out--let me know if you disagree.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.h:57:
DownloadManagerImpl(content::DownloadManagerDelegate* delegate,
On 2012/06/07 17:04:53, asanka wrote:
> Nit: Document ownership semantics?

Delegate argument gone, file_manager (lack of) ownership documented, and factory
changed over to scoped_ptr<> so self-documenting.  Let me know what you think.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:11: #include
"base/memory/scoped_ptr.h"
On 2012/06/07 17:04:53, asanka wrote:
> Duplicate

Whoops; done.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:98: const
DownloadRequestHandle& request_handle) {
On 2012/06/07 17:04:53, asanka wrote:
> Nit: OVERRIDE

Done.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:128: class Observer {
On 2012/06/07 17:04:53, asanka wrote:
> Nit: Would it be cleaner if MockDownloadItemFactory maintained and exposed a
> download item map of items it has created?

Good idea.  I'm not certain about the best way to manage the exposure of the
map; we could just make it a public data member, but that seems a bit yucky. 
I'm tempted to publicly inherit from std::map<>, which is slightly less yucky,
but still makes me twitch a bit.  And going to the hassle of actually doing a
proper iterator interface seems like overkill.  So I just exposed the
information needed in accessors.  Let me know what you think.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:260:
next_download_id_(0) {
On 2012/06/07 17:04:53, asanka wrote:
> Nit: Initialize scalars (mock_download_item_factory_)

Done.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:305:
mock_download_item_factory_ = NULL; // Owned by DownloadManager.
On 2012/06/07 17:04:53, asanka wrote:
> Could we test that it gets deleted? Perhaps stuff in a WeakPtr?

Done.  That also makes me feel more comfortable about keeping a pointer to an
unowned object that I'm expecting to be deleted while I'm holding the pointer
:-}.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:315: int
AddItemToManager() {
On 2012/06/07 17:04:53, asanka wrote:
> Why not return a MockDownloadItem& directly?

Done.

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:557:
download_manager_->DownloadCancelled(&item);
On 2012/06/07 17:04:53, asanka wrote:
> Check GetActiveDownloadItem() afterwards?

The current behavior (which is that non-persisted downloads stick around as
active even when cancelled, because they're waiting to rendezvous with the
callback from the history system) is one that I'd like to change, and that I
don't think external consumers of the DownloadManager should be relying on, so
I'm not inclined to test GetActiveDownloadItem().  I've put in a TODO about
changing this when we change the download manager behavior.  WDYT?

http://codereview.chromium.org/10344024/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:579:
download_manager_->DownloadCancelled(&item);
On 2012/06/07 17:04:53, asanka wrote:
> Check GetActiveDownloadItem() afterwards?

Done.

http://codereview.chromium.org/10344024/diff/13002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10344024/diff/13002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:168: content::DownloadItem*
DownloadItemFactoryImpl::CreatePersistedItem(
On 2012/06/07 21:35:41, John Abd-El-Malek wrote:
> this is personal style so feel free to ignore, but in cases like this I prefer
> to put the implementation inline. one knows what the functions are from the
> interface header, which is where the function documentation is anyways. so
> splitting it makes the code take longer to scan

Done.  I have a strong preference for fewer lines of code (all other things
being equal) which I have to stomp on sometimes for cleanliness.  So I'm happy
to unstomp :-}.

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
content/browser/download/download_manager_impl_unittest.cc:190:
result.push_back(it->first);
I'm sure there's an STL-fancy way to do this, but my STL documentation and I are
separated at the moment, I'm uncertain as to the overall architecture of this
interface, and it would save me max two lines.  Let me know if you think it's
worth the effort :-}.

[jam, 2012-06-08 20:46:40]

On 2012/06/07 23:27:21, rdsmith wrote:
> PTAL.  
> 
> High-level request for opinion: I recreated the DownloadManagerImpl::Create()
> function, so that I could allow the regular code to just specify a NetLog, and
> the unit test code to also specify file manager and item factory.  I'd be
> interested in opinions as proper ways to do this separation.  I've also
thought
> of doing it via two constructors, but that seems to run afoul of the style
> guides "no default arguments" rule.  Possibly if the unit test constructor was
> private?

(apologies for the delay, just saw this since I was overloaded with sheriffing.
feel free to IM me if I don't respond in an hour or two)


DownloadManager::Create existed before because the embedder needed to create the
implementation of the interface and it didn't have access to the header. Now
that this is called (in one place) from content, why not just have another
constructor with these three arguments?

[Randy Smith (Not in Mondays), 2012-06-08 21:27:31]

On 2012/06/08 20:46:40, John Abd-El-Malek wrote:
> On 2012/06/07 23:27:21, rdsmith wrote:
> > PTAL.  
> > 
> > High-level request for opinion: I recreated the
DownloadManagerImpl::Create()
> > function, so that I could allow the regular code to just specify a NetLog,
and
> > the unit test code to also specify file manager and item factory.  I'd be
> > interested in opinions as proper ways to do this separation.  I've also
> thought
> > of doing it via two constructors, but that seems to run afoul of the style
> > guides "no default arguments" rule.  Possibly if the unit test constructor
was
> > private?
> 
> (apologies for the delay, just saw this since I was overloaded with
sheriffing.
> feel free to IM me if I don't respond in an hour or two)
> 
> 
> DownloadManager::Create existed before because the embedder needed to create
the
> implementation of the interface and it didn't have access to the header. Now
> that this is called (in one place) from content, why not just have another
> constructor with these three arguments?

If I get guidelines from a senior developer on the chrome team (:-}) that that's
ok, I'm good with it.  The reason I was casting about for something else to do
was this passage from the style guide:

----
Except as described below, we require all arguments to be explicitly specified,
to force programmers to consider the API and the values they are passing for
each argument rather than silently accepting defaults they may not be aware of.
----

I considered having functions that were equivalent except for left out arguments
that were defaulted to be a variant on this.

[jabdelmalek, 2012-06-08 21:39:33]

On 2012/06/08 21:27:31, rdsmith wrote:
> On 2012/06/08 20:46:40, John Abd-El-Malek wrote:
> > On 2012/06/07 23:27:21, rdsmith wrote:
> > > PTAL.  
> > > 
> > > High-level request for opinion: I recreated the
> DownloadManagerImpl::Create()
> > > function, so that I could allow the regular code to just specify a NetLog,
> and
> > > the unit test code to also specify file manager and item factory.  I'd be
> > > interested in opinions as proper ways to do this separation.  I've also
> > thought
> > > of doing it via two constructors, but that seems to run afoul of the style
> > > guides "no default arguments" rule.  Possibly if the unit test constructor
> was
> > > private?
> > 
> > (apologies for the delay, just saw this since I was overloaded with
> sheriffing.
> > feel free to IM me if I don't respond in an hour or two)
> > 
> > 
> > DownloadManager::Create existed before because the embedder needed to create
> the
> > implementation of the interface and it didn't have access to the header. Now
> > that this is called (in one place) from content, why not just have another
> > constructor with these three arguments?
> 
> If I get guidelines from a senior developer on the chrome team (:-}) that
that's
> ok, I'm good with it.  The reason I was casting about for something else to do
> was this passage from the style guide:
> 
> ----
> Except as described below, we require all arguments to be explicitly
specified,
> to force programmers to consider the API and the values they are passing for
> each argument rather than silently accepting defaults they may not be aware
of.
> ----
> 
> I considered having functions that were equivalent except for left out
arguments
> that were defaulted to be a variant on this.

I'm not sure I understand what you mean. To be clear, I wasn't saying we should
have default arguments, since that's against the style guide. I meant that we
have a constructor with 3 arguments, and the current place that was calling
Create() instead does what Create does to get these arguments and passes them to
the constructor.

[Randy Smith (Not in Mondays), 2012-06-08 21:42:45]

On 2012/06/08 21:39:33, jabdelmalek wrote:
> On 2012/06/08 21:27:31, rdsmith wrote:
> > On 2012/06/08 20:46:40, John Abd-El-Malek wrote:
> > > On 2012/06/07 23:27:21, rdsmith wrote:
> > > > PTAL.  
> > > > 
> > > > High-level request for opinion: I recreated the
> > DownloadManagerImpl::Create()
> > > > function, so that I could allow the regular code to just specify a
NetLog,
> > and
> > > > the unit test code to also specify file manager and item factory.  I'd
be
> > > > interested in opinions as proper ways to do this separation.  I've also
> > > thought
> > > > of doing it via two constructors, but that seems to run afoul of the
style
> > > > guides "no default arguments" rule.  Possibly if the unit test
constructor
> > was
> > > > private?
> > > 
> > > (apologies for the delay, just saw this since I was overloaded with
> > sheriffing.
> > > feel free to IM me if I don't respond in an hour or two)
> > > 
> > > 
> > > DownloadManager::Create existed before because the embedder needed to
create
> > the
> > > implementation of the interface and it didn't have access to the header.
Now
> > > that this is called (in one place) from content, why not just have another
> > > constructor with these three arguments?
> > 
> > If I get guidelines from a senior developer on the chrome team (:-}) that
> that's
> > ok, I'm good with it.  The reason I was casting about for something else to
do
> > was this passage from the style guide:
> > 
> > ----
> > Except as described below, we require all arguments to be explicitly
> specified,
> > to force programmers to consider the API and the values they are passing for
> > each argument rather than silently accepting defaults they may not be aware
> of.
> > ----
> > 
> > I considered having functions that were equivalent except for left out
> arguments
> > that were defaulted to be a variant on this.
> 
> I'm not sure I understand what you mean. To be clear, I wasn't saying we
should
> have default arguments, since that's against the style guide. I meant that we
> have a constructor with 3 arguments, and the current place that was calling
> Create() instead does what Create does to get these arguments and passes them
to
> the constructor.

Hmmm.  I don't mind consing up the download manager from a random place in the
code, but consing up a DownloadItemFactoryImpl bothers me--it's a pretty tightly
download-internal concept that doesn't really have much to do with
BrowserContext.  OTOH, passing in a NULL pointer for the DIFI to indicate that
the DownloadManager should cons up its own would be fine, and doesn't run me
into any problems with the style guide that I can see.  I'll do that (and just
have the three element constructor) unless you or Asanka indicate that that's
objectionable.

[Randy Smith (Not in Mondays), 2012-06-08 21:43:15]

On 2012/06/08 21:42:45, rdsmith wrote:
> On 2012/06/08 21:39:33, jabdelmalek wrote:
> > On 2012/06/08 21:27:31, rdsmith wrote:
> > > On 2012/06/08 20:46:40, John Abd-El-Malek wrote:
> > > > On 2012/06/07 23:27:21, rdsmith wrote:
> > > > > PTAL.  
> > > > > 
> > > > > High-level request for opinion: I recreated the
> > > DownloadManagerImpl::Create()
> > > > > function, so that I could allow the regular code to just specify a
> NetLog,
> > > and
> > > > > the unit test code to also specify file manager and item factory.  I'd
> be
> > > > > interested in opinions as proper ways to do this separation.  I've
also
> > > > thought
> > > > > of doing it via two constructors, but that seems to run afoul of the
> style
> > > > > guides "no default arguments" rule.  Possibly if the unit test
> constructor
> > > was
> > > > > private?
> > > > 
> > > > (apologies for the delay, just saw this since I was overloaded with
> > > sheriffing.
> > > > feel free to IM me if I don't respond in an hour or two)
> > > > 
> > > > 
> > > > DownloadManager::Create existed before because the embedder needed to
> create
> > > the
> > > > implementation of the interface and it didn't have access to the header.
> Now
> > > > that this is called (in one place) from content, why not just have
another
> > > > constructor with these three arguments?
> > > 
> > > If I get guidelines from a senior developer on the chrome team (:-}) that
> > that's
> > > ok, I'm good with it.  The reason I was casting about for something else
to
> do
> > > was this passage from the style guide:
> > > 
> > > ----
> > > Except as described below, we require all arguments to be explicitly
> > specified,
> > > to force programmers to consider the API and the values they are passing
for
> > > each argument rather than silently accepting defaults they may not be
aware
> > of.
> > > ----
> > > 
> > > I considered having functions that were equivalent except for left out
> > arguments
> > > that were defaulted to be a variant on this.
> > 
> > I'm not sure I understand what you mean. To be clear, I wasn't saying we
> should
> > have default arguments, since that's against the style guide. I meant that
we
> > have a constructor with 3 arguments, and the current place that was calling
> > Create() instead does what Create does to get these arguments and passes
them
> to
> > the constructor.
> 
> Hmmm.  I don't mind consing up the download manager from a random place in

Sorry, that should have been "consing up the DownloadFileManager".  

> the
> code, but consing up a DownloadItemFactoryImpl bothers me--it's a pretty
tightly
> download-internal concept that doesn't really have much to do with
> BrowserContext.  OTOH, passing in a NULL pointer for the DIFI to indicate that
> the DownloadManager should cons up its own would be fine, and doesn't run me
> into any problems with the style guide that I can see.  I'll do that (and just
> have the three element constructor) unless you or Asanka indicate that that's
> objectionable.

[jam, 2012-06-08 21:57:03]

> > Hmmm.  I don't mind consing up the download manager from a random place in
> 
> Sorry, that should have been "consing up the DownloadFileManager".  
> 
> > the
> > code, but consing up a DownloadItemFactoryImpl bothers me--it's a pretty
> tightly
> > download-internal concept that doesn't really have much to do with
> > BrowserContext.  OTOH, passing in a NULL pointer for the DIFI to indicate
that
> > the DownloadManager should cons up its own would be fine, and doesn't run me
> > into any problems with the style guide that I can see.  I'll do that (and
just
> > have the three element constructor) unless you or Asanka indicate that
that's
> > objectionable.

ah, sorry I didn't look closely enough at all the parameters. I agree that the
downloaditemfactory stuff can stay inside the download_manager_impl.cc.

How about instead of taking a paramter that you're passing in null for now,
don't take it all? i.e. just 

DownloadManagerImpl(DownloadFileManager* file_manager, net::NetLog* net_log)

?

[asanka, 2012-06-08 22:14:18]

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
content/browser/download/download_manager_impl_unittest.cc:132: explicit
MockDownloadItemFactory();
Minor nit: no need for 'explicit' for 0 argument constructor.

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
content/browser/download/download_manager_impl_unittest.cc:318: std::vector<int>
active_ids(mock_download_item_factory_->Ids());
It seems something like mock_download_item_factory_->PopItem() that removes an
item and returns it would make this simpler.

[Randy Smith (Not in Mondays), 2012-06-08 22:16:23]

On 2012/06/08 21:57:03, John Abd-El-Malek wrote:
> > > Hmmm.  I don't mind consing up the download manager from a random place in
> > 
> > Sorry, that should have been "consing up the DownloadFileManager".  
> > 
> > > the
> > > code, but consing up a DownloadItemFactoryImpl bothers me--it's a pretty
> > tightly
> > > download-internal concept that doesn't really have much to do with
> > > BrowserContext.  OTOH, passing in a NULL pointer for the DIFI to indicate
> that
> > > the DownloadManager should cons up its own would be fine, and doesn't run
me
> > > into any problems with the style guide that I can see.  I'll do that (and
> just
> > > have the three element constructor) unless you or Asanka indicate that
> that's
> > > objectionable.
> 
> ah, sorry I didn't look closely enough at all the parameters. I agree that the
> downloaditemfactory stuff can stay inside the download_manager_impl.cc.
> 
> How about instead of taking a paramter that you're passing in null for now,
> don't take it all? i.e. just 
> 
> DownloadManagerImpl(DownloadFileManager* file_manager, net::NetLog* net_log)
> 
> ?

That means I can't do the unit testing, which is the major point of this CL :-}.

[Randy Smith (Not in Mondays), 2012-06-09 17:32:39]

PTAL.

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
content/browser/download/download_manager_impl_unittest.cc:132: explicit
MockDownloadItemFactory();
On 2012/06/08 22:14:18, asanka wrote:
> Minor nit: no need for 'explicit' for 0 argument constructor.

Done.

http://codereview.chromium.org/10344024/diff/5003/content/browser/download/do...
content/browser/download/download_manager_impl_unittest.cc:318: std::vector<int>
active_ids(mock_download_item_factory_->Ids());
On 2012/06/08 22:14:18, asanka wrote:
> It seems something like mock_download_item_factory_->PopItem() that removes an
> item and returns it would make this simpler.

Done.

[jam, 2012-06-11 05:34:22]

On 2012/06/08 22:16:23, rdsmith wrote:
> On 2012/06/08 21:57:03, John Abd-El-Malek wrote:
> > > > Hmmm.  I don't mind consing up the download manager from a random place
in
> > > 
> > > Sorry, that should have been "consing up the DownloadFileManager".  
> > > 
> > > > the
> > > > code, but consing up a DownloadItemFactoryImpl bothers me--it's a pretty
> > > tightly
> > > > download-internal concept that doesn't really have much to do with
> > > > BrowserContext.  OTOH, passing in a NULL pointer for the DIFI to
indicate
> > that
> > > > the DownloadManager should cons up its own would be fine, and doesn't
run
> me
> > > > into any problems with the style guide that I can see.  I'll do that
(and
> > just
> > > > have the three element constructor) unless you or Asanka indicate that
> > that's
> > > > objectionable.
> > 
> > ah, sorry I didn't look closely enough at all the parameters. I agree that
the
> > downloaditemfactory stuff can stay inside the download_manager_impl.cc.
> > 
> > How about instead of taking a paramter that you're passing in null for now,
> > don't take it all? i.e. just 
> > 
> > DownloadManagerImpl(DownloadFileManager* file_manager, net::NetLog* net_log)
> > 
> > ?
> 
> That means I can't do the unit testing, which is the major point of this CL
:-}.

ah, ok finally I got it :) ok, this is fine with me. or you can also have two
constructors, one with and one without. this is all style, so up to you

[asanka, 2012-06-11 17:51:51]

LGTM

[commit-bot: I haz the power, 2012-06-11 22:17:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10344024/13014

[commit-bot: I haz the power, 2012-06-11 22:17:51]

Presubmit check for 10344024-13014 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/browser

Presubmit checks took 4.2s to calculate.

[Randy Smith (Not in Mondays), 2012-06-12 02:08:14]

John, could I have your official LGTM for content/ (gypi files) and
content/browser (browser_context.cc)?

[jam, 2012-06-12 15:46:06]

lgtm

[commit-bot: I haz the power, 2012-06-12 15:47:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10344024/13014

[commit-bot: I haz the power, 2012-06-12 17:05:05]

Change committed as 141674