Added a download file factory to the download file manager, for testing.

Added a download file factory to the download file manager, for testing.


BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113658

[ahendrickson, 2011-12-02 22:54:30]

PTAL.

[Randy Smith (Not in Mondays), 2011-12-05 22:25:32]

Chris, a chromium style question for you: For a class as trivial as
StandardDownloadFileFactory, can we just declare it in download_file_manager.h
even though DownloadFileManager's constructor takes its base class?  I wince at
creating a new file for such a trivial class.

http://codereview.chromium.org/8770024/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/8770024/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:152: file_manager_ = new
DownloadFileManager(NULL, NULL);
You should either specify what happens when you pass in a NULL at the
constructor declaration, or (preferred) throw together a quick mock here that
asserts when the factor GetFile is called.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
File content/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.h:76: virtual DownloadFile*
GetFile(DownloadCreateInfo* info,
nit, suggestion: I'd call this CreateFile.

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:123: class
StandardDownloadFileFactory
I don't think this belongs in ResourceDispatcherHost; it's really part of the
download code.  I'm inclined to think that means a new file, but that seems like
a real waste of directory space.  I'll ping Chris on whether it's reasonable to
just declare DownloadFileFactoryImpl in download_file_manager.h when I respond
to this review.

I'd also call it DownloadFileFactoryImpl.

[cbentzel, 2011-12-07 18:58:20]

I went beyond the simple question, sorry.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.cc:41: :
resource_dispatcher_host_(rdh), download_file_factory_(factory) {
Should DCHECK here that both rdh and factory are non-null.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.cc:72:
download_file_factory_->GetFile(info, request_handle, download_manager));
that one unit test passes in a NULL DownloadFileFactory, you probably want a
fake one.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
File content/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.h:179: DownloadFileFactory*
download_file_factory_;
this should be a scoped_ptr if it takes ownership

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:123: class
StandardDownloadFileFactory
On 2011/12/05 22:25:38, rdsmith wrote:
> I don't think this belongs in ResourceDispatcherHost; it's really part of the
> download code.  I'm inclined to think that means a new file, but that seems
like
> a real waste of directory space.  I'll ping Chris on whether it's reasonable
to
> just declare DownloadFileFactoryImpl in download_file_manager.h when I respond
> to this review.
> 
> I'd also call it DownloadFileFactoryImpl.

I agree that the definition doesn't belong in RDH.

I think it would be fine to put it in download_file_manager.h

I'd also be fine with putting it in download_file_factory.h and moving the
factory interface out of DownloadFileManager.

One other option - passing in a NULL DownloadFileFactory into
DownloadFileManager could indicate that the DownloadFileManager should create a
StandardDownloadFileFactory [or whatever the new name is]. That way you don't
need to expose it at all.

[ahendrickson, 2011-12-07 19:55:14]

http://codereview.chromium.org/8770024/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/8770024/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:152: file_manager_ = new
DownloadFileManager(NULL, NULL);
On 2011/12/05 22:25:38, rdsmith wrote:
> You should either specify what happens when you pass in a NULL at the
> constructor declaration, or (preferred) throw together a quick mock here that
> asserts when the factor GetFile is called.

Did both!

Using a NULL to indicate that the default DownloadFileFactory should be used.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.cc:41: :
resource_dispatcher_host_(rdh), download_file_factory_(factory) {
On 2011/12/07 18:58:21, cbentzel wrote:
> Should DCHECK here that both rdh and factory are non-null.

rdh is allowed to be NULL.

If |factory| is NULL, we will use the default factory, which is now defined in
this file.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.cc:72:
download_file_factory_->GetFile(info, request_handle, download_manager));
On 2011/12/07 18:58:21, cbentzel wrote:
> that one unit test passes in a NULL DownloadFileFactory, you probably want a
> fake one.

Done.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
File content/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.h:76: virtual DownloadFile*
GetFile(DownloadCreateInfo* info,
On 2011/12/05 22:25:38, rdsmith wrote:
> nit, suggestion: I'd call this CreateFile.

Done.

http://codereview.chromium.org/8770024/diff/3001/content/browser/download/dow...
content/browser/download/download_file_manager.h:179: DownloadFileFactory*
download_file_factory_;
On 2011/12/07 18:58:21, cbentzel wrote:
> this should be a scoped_ptr if it takes ownership

Done.

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:123: class
StandardDownloadFileFactory
On 2011/12/07 18:58:21, cbentzel wrote:
> On 2011/12/05 22:25:38, rdsmith wrote:
> > I don't think this belongs in ResourceDispatcherHost; it's really part of
the
> > download code.  I'm inclined to think that means a new file, but that seems
> like
> > a real waste of directory space.  I'll ping Chris on whether it's reasonable
> to
> > just declare DownloadFileFactoryImpl in download_file_manager.h when I
respond
> > to this review.
> > 
> > I'd also call it DownloadFileFactoryImpl.
> 
> I agree that the definition doesn't belong in RDH.
> 
> I think it would be fine to put it in download_file_manager.h
> 
> I'd also be fine with putting it in download_file_factory.h and moving the
> factory interface out of DownloadFileManager.
> 
> One other option - passing in a NULL DownloadFileFactory into
> DownloadFileManager could indicate that the DownloadFileManager should create
a
> StandardDownloadFileFactory [or whatever the new name is]. That way you don't
> need to expose it at all.

I like option 3.  Done.

http://codereview.chromium.org/8770024/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:123: class
StandardDownloadFileFactory
On 2011/12/05 22:25:38, rdsmith wrote:
> I don't think this belongs in ResourceDispatcherHost; it's really part of the
> download code.  I'm inclined to think that means a new file, but that seems
like
> a real waste of directory space.  I'll ping Chris on whether it's reasonable
to
> just declare DownloadFileFactoryImpl in download_file_manager.h when I respond
> to this review.
> 
> I'd also call it DownloadFileFactoryImpl.

Renamed, and moved into DownloadFileManager.cc.

Using Chris' NULL parameter option.

[Randy Smith (Not in Mondays), 2011-12-07 20:57:41]

LGTM with comments below (specifically, do circle back with me if there's some
reason for the include file; I want to think about alternatives if so).

http://codereview.chromium.org/8770024/diff/12001/content/browser/download/do...
File content/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/8770024/diff/12001/content/browser/download/do...
content/browser/download/download_file_manager.h:84: // Passing in a NULL for
|factory| will cause the default
nit: "the" to "a", since we're talking about objects rather than classes.

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:30: #include
"content/browser/download/download_file_impl.h"
Why do we need this header inclusion?

[ahendrickson, 2011-12-07 21:11:17]

http://codereview.chromium.org/8770024/diff/12001/content/browser/download/do...
File content/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/8770024/diff/12001/content/browser/download/do...
content/browser/download/download_file_manager.h:84: // Passing in a NULL for
|factory| will cause the default
On 2011/12/07 20:57:41, rdsmith wrote:
> nit: "the" to "a", since we're talking about objects rather than classes.

Done.

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:30: #include
"content/browser/download/download_file_impl.h"
On 2011/12/07 20:57:41, rdsmith wrote:
> Why do we need this header inclusion?

We don't -- I forgot to remove it during a previous cleanup of this file.

[ahendrickson, 2011-12-07 22:06:58]

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/8770024/diff/12001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:30: #include
"content/browser/download/download_file_impl.h"
On 2011/12/07 21:11:17, ahendrickson wrote:
> On 2011/12/07 20:57:41, rdsmith wrote:
> > Why do we need this header inclusion?
> 
> We don't -- I forgot to remove it during a previous cleanup of this file.

(Removed)

[commit-bot: I haz the power, 2011-12-08 02:36:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8770024/9007

[commit-bot: I haz the power, 2011-12-08 02:36:55]

Presubmit check for 8770024-9007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/resource_dispatcher_host.cc

Presubmit checks took 1.2s to calculate.

[ahendrickson, 2011-12-08 16:28:45]

jam, could you take a quick look at resource_dispatcher_host.cc?

It's a very minimal change, due to adding a parameter to the constructor of
DownloadFileManager.

[ahendrickson, 2011-12-08 16:31:35]

darin, could you take a quick look at resource_dispatcher_host.cc?

It's a very minimal change, due to adding a parameter to the constructor of
DownloadFileManager.

[ahendrickson, 2011-12-08 19:14:13]

sky, could you take a quick look at resource_dispatcher_host.cc?

It's a very minimal change, due to adding a parameter to the constructor of
DownloadFileManager.

[ahendrickson, 2011-12-08 19:14:15]

sky, could you take a quick look at resource_dispatcher_host.cc?

It's a very minimal change, due to adding a parameter to the constructor of
DownloadFileManager.

[sky, 2011-12-08 19:14:45]

LGTM

[commit-bot: I haz the power, 2011-12-08 19:29:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8770024/9007

[commit-bot: I haz the power, 2011-12-08 21:04:38]

Change committed as 113658