Created a DownloadManager interface, for use in unit tests..

Created a DownloadManager interface, for use in unit tests..

Bug=None
Test=None


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

[ahendrickson, 2011-11-04 17:18:31]

Randy, PTAL.

I finally got all the CLANG errors fixed.

[jam, 2011-11-04 17:39:18]

drive by: where's the interface?

the convention is to have the interfaces be named X, and have the implementation
be XImpl

[ahendrickson, 2011-11-04 18:22:32]

Huh.  I got the impression from the coding style doc that it was recommended to
have the interface have the suffix 'Interface'.

I can change it to the other style, if that is the standard way.

On 2011/11/04 17:39:18, John Abd-El-Malek wrote:
> drive by: where's the interface?
> 
> the convention is to have the interfaces be named X, and have the
implementation
> be XImpl

[Randy Smith (Not in Mondays), 2011-11-04 18:27:23]

On 2011/11/04 18:22:32, ahendrickson wrote:
> Huh.  I got the impression from the coding style doc that it was recommended
to
> have the interface have the suffix 'Interface'.
> 
> I can change it to the other style, if that is the standard way.
> 
> On 2011/11/04 17:39:18, John Abd-El-Malek wrote:
> > drive by: where's the interface?
> > 
> > the convention is to have the interfaces be named X, and have the
> implementation
> > be XImpl

I've seen both and am happy to take John's opinion about the most common case. 
My belief and inclination is that the thing without the suffix should be the
thing most commonly used in the code, and the thing with the suffix should be
the thing that's occasionally used in corner cases.  I think we're going to be
passing around the DownloadManager interface and keeping the implementation
mostly unreferenced, so I'm inclined to go with DownloadManager and
DownloadManagerImpl.

[ahendrickson, 2011-11-04 18:29:32]

OK, will do.

On 2011/11/04 18:27:23, rdsmith wrote:
> On 2011/11/04 18:22:32, ahendrickson wrote:
> > Huh.  I got the impression from the coding style doc that it was recommended
> to
> > have the interface have the suffix 'Interface'.
> > 
> > I can change it to the other style, if that is the standard way.
> > 
> > On 2011/11/04 17:39:18, John Abd-El-Malek wrote:
> > > drive by: where's the interface?
> > > 
> > > the convention is to have the interfaces be named X, and have the
> > implementation
> > > be XImpl
> 
> I've seen both and am happy to take John's opinion about the most common case.

> My belief and inclination is that the thing without the suffix should be the
> thing most commonly used in the code, and the thing with the suffix should be
> the thing that's occasionally used in corner cases.  I think we're going to be
> passing around the DownloadManager interface and keeping the implementation
> mostly unreferenced, so I'm inclined to go with DownloadManager and
> DownloadManagerImpl.

[jam, 2011-11-04 22:09:02]

On 2011/11/04 18:27:23, rdsmith wrote:
> On 2011/11/04 18:22:32, ahendrickson wrote:
> > Huh.  I got the impression from the coding style doc that it was recommended
> to
> > have the interface have the suffix 'Interface'.
> > 
> > I can change it to the other style, if that is the standard way.
> > 
> > On 2011/11/04 17:39:18, John Abd-El-Malek wrote:
> > > drive by: where's the interface?
> > > 
> > > the convention is to have the interfaces be named X, and have the
> > implementation
> > > be XImpl
> 
> I've seen both and am happy to take John's opinion about the most common case.

> My belief and inclination is that the thing without the suffix should be the
> thing most commonly used in the code, and the thing with the suffix should be
> the thing that's occasionally used in corner cases.  I think we're going to be
> passing around the DownloadManager interface and keeping the implementation
> mostly unreferenced, so I'm inclined to go with DownloadManager and
> DownloadManagerImpl.

I haven't seen the "Interface" suffix much, but perhaps it's in areas which I
don't look at. Since we'll want to expose DownloadManager from content through
the content API, it's also important to match the rest of the Content API. For
that we've been using the pattern I suggested. If you can also put the interface
in the content namespace, that'd be good.

[ahendrickson, 2011-11-07 21:11:57]

Switched from DownloadManagerInterface/DownloadManager pair to
DownloadManager/DownloadManagerImpl.

[Randy Smith (Not in Mondays), 2011-11-08 21:53:52]

Andy: Basic message is that this approach looks good, and I'm still reviewing
(diving down to the next level of detail).

A question for John: One thing that came up for me in my first read through of
PS4 was a feeling of terror when I noticed the change of
DownloadPrefs::FromDownloadManager to taking a DownloadManagerInterface.  The
terror is because there is now an implicit restriction on any mock we do of
DownloadManager, which is that if DownloadPrefs::FromDownloadManager is ever
called on it, that mock had better have a delegate that is a
ChromeDownloadManagerDelegate.  This was a flaw in the original code (any
DownloadManager that might have DownloadPrefs::FromDownloadManager called on it
had better have a ChromeDownloadManagerDelegate as delegate.  But now we're
adding another layer of indirection.  And there's no good place in the code
(that I could think of) to mark this as "DANGER!  Be careful or you'll shoot
yourself".

That diff no longer exists, but the functionality change is still there.  So
John: Can you think of a good way to get around this, or maybe make it a bit
safer?  I can't, other than getting rid of DownloadPrefs::FromDownloadManager()
and putting other ways of doing the lookup into the code.  (I looked at that
option, and it falls down in the UI, which is chrome level but doesn't have any
real hooks other than to the DownloadItem/DownloadManager).  So I think I have
to live with my terror, at least until we do a more thorough refactor.  But I
thought I'd ask in case I was missing some idea.

http://codereview.chromium.org/8351052/diff/3002/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/3002/content/browser/download/dow...
content/browser/download/download_manager.h:376: // More
DownloadManagerInterface functions.
nit: Combine these with the list above.

[Randy Smith (Not in Mondays), 2011-11-08 22:10:50]

Andy: This looks good.  Clue me on my one question below and I think you're good
to go.

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

http://codereview.chromium.org/8351052/diff/11015/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:60: download_manager_(new
DownloadManagerImpl(
Why this change?

http://codereview.chromium.org/8351052/diff/11015/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/11015/content/browser/download/do...
content/browser/download/download_manager.h:310: // Browser's download manager:
manages all downloads and destination view.
I *think* this comment is more appropriate next to the interface class above. 
It's a tricky call, but I think the object that's managing the downloads is more
associated with the interface (and all the comments on the interface) than with
the implementation class.

[ahendrickson, 2011-11-09 17:09:02]

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

http://codereview.chromium.org/8351052/diff/11015/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:60: download_manager_(new
DownloadManagerImpl(
On 2011/11/08 22:10:51, rdsmith wrote:
> Why this change?

A) This is a test for DownloadManager.
B) The old version of MockDownloadManager was derived from DownloadManager, and
just added one empty function.  It did not override any functions, so its
functionality was identical to DownloadManager.
C) The new MockDownloadManager is very different from DownloadManager.

The upshot is that this change preserves the functionality of the test!

http://codereview.chromium.org/8351052/diff/11015/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/11015/content/browser/download/do...
content/browser/download/download_manager.h:310: // Browser's download manager:
manages all downloads and destination view.
On 2011/11/08 22:10:51, rdsmith wrote:
> I *think* this comment is more appropriate next to the interface class above. 
> It's a tricky call, but I think the object that's managing the downloads is
more
> associated with the interface (and all the comments on the interface) than
with
> the implementation class.

Done.

[jam, 2011-11-09 17:34:00]

http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
content/browser/download/download_manager.h:311: class CONTENT_EXPORT
DownloadManagerImpl
this need to go into a separate file, i.e .download_manager_impl.h/cc

[Randy Smith (Not in Mondays), 2011-11-09 19:06:33]

LGTM.

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

http://codereview.chromium.org/8351052/diff/11015/chrome/browser/download/dow...
chrome/browser/download/download_manager_unittest.cc:60: download_manager_(new
DownloadManagerImpl(
On 2011/11/09 17:09:02, ahendrickson wrote:
> On 2011/11/08 22:10:51, rdsmith wrote:
> > Why this change?
> 
> A) This is a test for DownloadManager.
> B) The old version of MockDownloadManager was derived from DownloadManager,
and
> just added one empty function.  It did not override any functions, so its
> functionality was identical to DownloadManager.
> C) The new MockDownloadManager is very different from DownloadManager.
> 
> The upshot is that this change preserves the functionality of the test!

Sounds good.

[ahendrickson, 2011-11-09 22:00:59]

http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
content/browser/download/download_manager.h:311: class CONTENT_EXPORT
DownloadManagerImpl
On 2011/11/09 17:34:00, John Abd-El-Malek wrote:
> this need to go into a separate file, i.e .download_manager_impl.h/cc

Done.

[ahendrickson, 2011-11-09 23:13:20]

jam, could you PTAL?

On 2011/11/09 22:00:59, ahendrickson wrote:
>
http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
> File content/browser/download/download_manager.h (right):
> 
>
http://codereview.chromium.org/8351052/diff/18001/content/browser/download/do...
> content/browser/download/download_manager.h:311: class CONTENT_EXPORT
> DownloadManagerImpl
> On 2011/11/09 17:34:00, John Abd-El-Malek wrote:
> > this need to go into a separate file, i.e .download_manager_impl.h/cc
> 
> Done.

[jam, 2011-11-10 17:19:09]

lgtm with nits fixed

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:35: //#include "base/callback.h"
nit: remove?

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:285: private:
nit: spacing

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:287: friend class
DownloadManagerTest;
are all these friends still needed?

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager_impl.h:5: // The DownloadManager
object manages the process of downloading, including
nit: this line through 25 should be removed, the comment should only be in the
interface file, and the implementation's header doesn't need to duplicate it

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager_impl.h:114: // More DownloadManager
functions.
nit: all the overridden functions from the same interface should be listed right
after each other

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
File content/browser/download/mock_download_manager.cc (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
content/browser/download/mock_download_manager.cc:29: DownloadVector* result) {
nit: spacing here and below in other functions is off

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
File content/browser/download/mock_download_manager.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
content/browser/download/mock_download_manager.h:94: private:
nit spacing

[commit-bot: I haz the power, 2011-11-17 13:16:10]

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

[commit-bot: I haz the power, 2011-11-17 16:13:02]

Change committed as 110499

[ahendrickson, 2011-11-18 18:47:39]

Sorry this is after commit . . .

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:35: //#include "base/callback.h"
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit: remove?

Done.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:285: private:
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit: spacing

Done.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager.h:287: friend class
DownloadManagerTest;
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> are all these friends still needed?

Was able to get rid of two.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager_impl.h:5: // The DownloadManager
object manages the process of downloading, including
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit: this line through 25 should be removed, the comment should only be in the
> interface file, and the implementation's header doesn't need to duplicate it

Done.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
content/browser/download/download_manager_impl.h:114: // More DownloadManager
functions.
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit: all the overridden functions from the same interface should be listed
right
> after each other

Done.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
File content/browser/download/mock_download_manager.cc (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
content/browser/download/mock_download_manager.cc:29: DownloadVector* result) {
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit: spacing here and below in other functions is off

Done.

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
File content/browser/download/mock_download_manager.h (right):

http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
content/browser/download/mock_download_manager.h:94: private:
On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> nit spacing

Done.

[Randy Smith (Not in Mondays), 2011-11-18 18:53:08]

On 2011/11/18 18:47:39, ahendrickson wrote:
> Sorry this is after commit . . .

Meaning you'll land these changes separately, or just that you responded to the
CL after you landed the changes?

> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> File content/browser/download/download_manager.h (right):
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> content/browser/download/download_manager.h:35: //#include "base/callback.h"
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit: remove?
> 
> Done.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> content/browser/download/download_manager.h:285: private:
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit: spacing
> 
> Done.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> content/browser/download/download_manager.h:287: friend class
> DownloadManagerTest;
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > are all these friends still needed?
> 
> Was able to get rid of two.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> File content/browser/download/download_manager_impl.h (right):
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> content/browser/download/download_manager_impl.h:5: // The DownloadManager
> object manages the process of downloading, including
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit: this line through 25 should be removed, the comment should only be in
the
> > interface file, and the implementation's header doesn't need to duplicate it
> 
> Done.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/do...
> content/browser/download/download_manager_impl.h:114: // More DownloadManager
> functions.
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit: all the overridden functions from the same interface should be listed
> right
> > after each other
> 
> Done.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
> File content/browser/download/mock_download_manager.cc (right):
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
> content/browser/download/mock_download_manager.cc:29: DownloadVector* result)
{
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit: spacing here and below in other functions is off
> 
> Done.
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
> File content/browser/download/mock_download_manager.h (right):
> 
>
http://codereview.chromium.org/8351052/diff/23002/content/browser/download/mo...
> content/browser/download/mock_download_manager.h:94: private:
> On 2011/11/10 17:19:09, John Abd-El-Malek wrote:
> > nit spacing
> 
> Done.