Created an interface for DownloadFile, for use in unit tests.

Created an interface for DownloadFile, for use in unit tests.

Bug=None
Test=None


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

[Randy Smith (Not in Mondays), 2011-11-10 02:36:38]

So the amount of friending going on here makes me more than a bit uncomfortable.
 Friending's usually a sign that the abstractions aren't right, and I think
that's true here.  We should try to avoid friending at all, and we should try
quite hard to avoid friending that isn't to test classes for purposes of doing
unit testing of that class.  I'm also a bit confused about some of the
implementation choices.  

Note that I don't think we need the mess in download_manager_unittest.cc with
DownloadFileWithMockStream anymore, because we can design whatever
DownloadFile-fitting class we want to use within that unit test--we now have the
ability to write all of the functions and return errors at the DownloadFile
level.  I haven't traced out any of the dependencies in detail, but I suspect
that fixing that usage will allow you to clean up the inter-class dependencies
quite a bit.

So for the following things, could you see if you can thinking of a way to use
mock classes to get rid of them, or let me know why we need them?

* BaseFile friending DownloadFileImpl
* BaseFile friending BaseFileWithMockStream
* The existence of the ReplaceBaseFile method
* The BaseFile member of DownloadFileImpl being a pointer rather than a
contained class.

Once we've gotten on the same page about the basic class relationships I'll dive
into a lower level review.

[ahendrickson, 2011-11-10 20:19:50]

OK, I refactored the DownloadFileWithMockStream class, and was able to remove a
lot of the friending.

Could you take another look?

On 2011/11/10 02:36:38, rdsmith wrote:
> So the amount of friending going on here makes me more than a bit
uncomfortable.
>  Friending's usually a sign that the abstractions aren't right, and I think
> that's true here.  We should try to avoid friending at all, and we should try
> quite hard to avoid friending that isn't to test classes for purposes of doing
> unit testing of that class.  I'm also a bit confused about some of the
> implementation choices.  
> 
> Note that I don't think we need the mess in download_manager_unittest.cc with
> DownloadFileWithMockStream anymore, because we can design whatever
> DownloadFile-fitting class we want to use within that unit test--we now have
the
> ability to write all of the functions and return errors at the DownloadFile
> level.  I haven't traced out any of the dependencies in detail, but I suspect
> that fixing that usage will allow you to clean up the inter-class dependencies
> quite a bit.
> 
> So for the following things, could you see if you can thinking of a way to use
> mock classes to get rid of them, or let me know why we need them?
> 
> * BaseFile friending DownloadFileImpl
> * BaseFile friending BaseFileWithMockStream
> * The existence of the ReplaceBaseFile method
> * The BaseFile member of DownloadFileImpl being a pointer rather than a
> contained class.
> 
> Once we've gotten on the same page about the basic class relationships I'll
dive
> into a lower level review.

[Randy Smith (Not in Mondays), 2011-11-10 21:58:42]

Thank you very much!  This patch set makes my purist abstract heart much happier
(and I really do think that translates to more maintainable code).

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (left):

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:60: download_manager_(new
MockDownloadManager(
Why this change here?  I thought that was the other CL?

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:166: class
DownloadFileWithErrors : public DownloadFileImpl {
Just noting that in the context of non-test code I'd want to bias away from
implementation inheritance, but in test code I'm cool with it.  (In the abstract
it's a short-cut that can come back to bite you because it splits code that
works together into separate locations, but I think it's a fine shortcut for
testing.)

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file.h:69: // Appends the passed-in |suffix|
to the |path|.
Why the ordering change?  (I don't care, I'm just curious.)

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:21: // These objects live
exclusively on the download thread and handle the writing
nit: Could you change  "download thread" to "file thread" as long as you're
fooling around with these comments.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:24: // cancelled, the DownloadFile
is destroyed.
I'm ambivalent (so if you have a strong feeling, let me know and we can go with
that) but I think that the comment describing the lifetime of a download file
probably should be in the interface file rather than the implementation file. 
It's an audience targeting thing--the people who need to know about the
externally visible behavior of the DownloadFile read the interface file, and I
think this counts as externally visible behavior.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:33: // BaseFile delegated
functions.
nit: The fact that these are delegated to the BaseFile is part of the
implementation, rather than the interface, and hence isn't as important to
remark on in the header.  I think the fact that these functions are inherited
from the DownloadFile ABC is more important.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:46: // DownloadFile
implementation.
The above is part of the DownloadFile implementation as well, right?

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:46: // DownloadFile
implementation.
Shouldn't all functions that inherit from the DownloadFile interface be marked
with OVERRIDE?

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

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_manager.h:136: DownloadManager*
download_manager);
I don't think that this is used in this CL?  I don't think I have objections to
it, but I'd like to see it combined with its use cases to judge that.  Willing
to pull it out and put it in in the first CL that uses it?

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_resource_handler.h:16: #include
"content/browser/download/download_types.h"
I wouldn't think you'd need this include in both .h and .cc file for
download_resource_handler.  Remove it from one of this?  (Preferably the .h if
you can.)

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/mo...
File content/browser/download/mock_download_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/mo...
content/browser/download/mock_download_file.h:20: class MockDownloadFile :
virtual public DownloadFile {
Are we using this anywhere in the CL?  If not, I'd be inclined to not include it
in this CL, but instead include it in the CL in which it's used.  

(I'm specifically wondering if it might make sense to define this class
completely within the test file that uses it, which would allow us to avoid
changing the name of the other class to GMockDownloadFile.  But I can't guess
without seeing it in situ, so I'm inclined to wait on trying to judge for seeing
it in situ.)

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/sa...
File content/browser/download/save_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/sa...
content/browser/download/save_file.h:27: // BaseFile delegated functions.
nit: As I understand chrome custom (and I think there are some arguments
stronger than customer, though I don't remember them) pretty much anything that
isn't in the "simple accessor" category should be in a .cc file.

[ahendrickson, 2011-11-13 00:50:21]

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
File chrome/browser/download/download_manager_unittest.cc (left):

http://codereview.chromium.org/8372034/diff/17001/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:60: download_manager_(new
MockDownloadManager(
On 2011/11/10 21:58:43, rdsmith wrote:
> Why this change here?  I thought that was the other CL?

Ah, yes, this is a result of splitting a CL in two, not that it changes
functionality in any way.

Reverted.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file.h:69: // Appends the passed-in |suffix|
to the |path|.
On 2011/11/10 21:58:43, rdsmith wrote:
> Why the ordering change?  (I don't care, I'm just curious.)

I just wanted the similar functions grouped together.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:21: // These objects live
exclusively on the download thread and handle the writing
On 2011/11/10 21:58:43, rdsmith wrote:
> nit: Could you change  "download thread" to "file thread" as long as you're
> fooling around with these comments.

Done.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:24: // cancelled, the DownloadFile
is destroyed.
On 2011/11/10 21:58:43, rdsmith wrote:
> I'm ambivalent (so if you have a strong feeling, let me know and we can go
with
> that) but I think that the comment describing the lifetime of a download file
> probably should be in the interface file rather than the implementation file. 
> It's an audience targeting thing--the people who need to know about the
> externally visible behavior of the DownloadFile read the interface file, and I
> think this counts as externally visible behavior.

Done.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:33: // BaseFile delegated
functions.
On 2011/11/10 21:58:43, rdsmith wrote:
> nit: The fact that these are delegated to the BaseFile is part of the
> implementation, rather than the interface, and hence isn't as important to
> remark on in the header.  I think the fact that these functions are inherited
> from the DownloadFile ABC is more important.

Done.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:46: // DownloadFile
implementation.
On 2011/11/10 21:58:43, rdsmith wrote:
> The above is part of the DownloadFile implementation as well, right?

Yes

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_impl.h:46: // DownloadFile
implementation.
On 2011/11/10 21:58:43, rdsmith wrote:
> Shouldn't all functions that inherit from the DownloadFile interface be marked
> with OVERRIDE?

Done.

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

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_file_manager.h:136: DownloadManager*
download_manager);
On 2011/11/10 21:58:43, rdsmith wrote:
> I don't think that this is used in this CL?  I don't think I have objections
to
> it, but I'd like to see it combined with its use cases to judge that.  Willing
> to pull it out and put it in in the first CL that uses it?

Done.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/do...
content/browser/download/download_resource_handler.h:16: #include
"content/browser/download/download_types.h"
On 2011/11/10 21:58:43, rdsmith wrote:
> I wouldn't think you'd need this include in both .h and .cc file for
> download_resource_handler.  Remove it from one of this?  (Preferably the .h if
> you can.)

Need it in the header file, as we're declaring a DownloadSaveInfo member
variable.

Done.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/mo...
File content/browser/download/mock_download_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/mo...
content/browser/download/mock_download_file.h:20: class MockDownloadFile :
virtual public DownloadFile {
On 2011/11/10 21:58:43, rdsmith wrote:
> Are we using this anywhere in the CL?  If not, I'd be inclined to not include
it
> in this CL, but instead include it in the CL in which it's used.  
> 
> (I'm specifically wondering if it might make sense to define this class
> completely within the test file that uses it, which would allow us to avoid
> changing the name of the other class to GMockDownloadFile.  But I can't guess
> without seeing it in situ, so I'm inclined to wait on trying to judge for
seeing
> it in situ.)

Using it now, in place of GMockDownloadFile.

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/sa...
File content/browser/download/save_file.h (right):

http://codereview.chromium.org/8372034/diff/17001/content/browser/download/sa...
content/browser/download/save_file.h:27: // BaseFile delegated functions.
On 2011/11/10 21:58:43, rdsmith wrote:
> nit: As I understand chrome custom (and I think there are some arguments
> stronger than customer, though I don't remember them) pretty much anything
that
> isn't in the "simple accessor" category should be in a .cc file.

Done.

[Randy Smith (Not in Mondays), 2011-11-14 21:58:38]

http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
File content/browser/download/mock_download_file.h (right):

http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
content/browser/download/mock_download_file.h:22: class MockDownloadFile :
virtual public DownloadFile {
What are the advantages of creating this class as opposed to using a GMock? 
(I'm not sure, which is why I ask :-}, but if you can get the same functionality
with a gmock, that's probably easier for people who are familiar with gmocks to
understand.)

[ahendrickson, 2011-11-16 19:39:11]

http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
File content/browser/download/mock_download_file.h (right):

http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
content/browser/download/mock_download_file.h:22: class MockDownloadFile :
virtual public DownloadFile {
On 2011/11/14 21:58:39, rdsmith wrote:
> What are the advantages of creating this class as opposed to using a GMock? 
> (I'm not sure, which is why I ask :-}, but if you can get the same
functionality
> with a gmock, that's probably easier for people who are familiar with gmocks
to
> understand.)

It's a lot more flexible.  You can monitor stuff, inject errors, whatever you
want.

At the moment it's only mimicking what GMock did, but that will change.

[Randy Smith (Not in Mondays), 2011-11-16 19:57:31]

On 2011/11/16 19:39:11, ahendrickson wrote:
>
http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
> File content/browser/download/mock_download_file.h (right):
> 
>
http://codereview.chromium.org/8372034/diff/24003/content/browser/download/mo...
> content/browser/download/mock_download_file.h:22: class MockDownloadFile :
> virtual public DownloadFile {
> On 2011/11/14 21:58:39, rdsmith wrote:
> > What are the advantages of creating this class as opposed to using a GMock? 
> > (I'm not sure, which is why I ask :-}, but if you can get the same
> functionality
> > with a gmock, that's probably easier for people who are familiar with gmocks
> to
> > understand.)
> 
> It's a lot more flexible.  You can monitor stuff, inject errors, whatever you
> want.
> 
> At the moment it's only mimicking what GMock did, but that will change.

I believe you can do all of that with GMock.  Of course, that requires climbing
the GMock learning curve, and doing it by hand is probably easier for external
reviewers to understand.  So I'm ok with doing it this way.

[Randy Smith (Not in Mondays), 2011-11-16 19:58:19]

LGTM.

[commit-bot: I haz the power, 2011-11-16 20:04:10]

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

[commit-bot: I haz the power, 2011-11-16 20:04:18]

Presubmit check for 8372034-24003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

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

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

Presubmit checks took 2.3s to calculate.

[ahendrickson, 2011-11-16 20:14:21]

darin: PTAL at content/browser/renderer_host/buffered_resource_handler.cc

I added a header that was necessary for DownloadSaveInfo(), that used to be
included in another header.

[darin (slow to review), 2011-11-16 21:23:02]

On 2011/11/16 20:14:21, ahendrickson wrote:
> darin: PTAL at content/browser/renderer_host/buffered_resource_handler.cc
> 
> I added a header that was necessary for DownloadSaveInfo(), that used to be
> included in another header.

That's totally fine.  LGTM

[commit-bot: I haz the power, 2011-11-16 21:30:17]

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

[commit-bot: I haz the power, 2011-11-16 22:31:43]

Change committed as 110377