Split DownloadItem into an ABC, an Impl, and a Mock.

This split will facilitate testing DownloadQuery and other users of DownloadItems.

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

[benjhayden, 2011-11-08 18:44:49]

PTAL when you get a chance.

[Randy Smith (Not in Mondays), 2011-11-09 17:09:57]

LGTM (modulo virtual destructor and comment about methods inherited from
DownloadItem--if you disagree with me on those, let's do one more round).

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item.h (left):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:15: // receive state updates.
I'm inclined to leave this here.  It's a question of what's the "real" download
item from the perspective of someone reading this file.  I think of .h files as
being targeted at consumers, and thus the interface is the real download item.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:136: virtual ~DownloadItem();
If anyone else is going to own a DownloadItem, don't you need it to have a
virtual destructor?

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:44: 
Why change ordering?  I think of the DownloadState as of more interest to
DownloadItem consumers (not a request, just my perspective).

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item_impl.h:69: virtual void
AddObserver(DownloadItem::Observer* observer) OVERRIDE;
Note that these are all inherited from DownloadItem.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/mock_...
File content/browser/download/mock_download_item.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/mock_...
content/browser/download/mock_download_item.h:15: class MockDownloadItem :
public DownloadItem {
If you're very clear that you're going to be using this in a near-future CL, I'm
ok with it going in, but my rule of thumb is that new code should be put in when
it's first used, and I don't think this CL uses MockDownloadItem.

(Obviously, I'm good with doing refactoring without a specific use case :-}.)

[cbentzel, 2011-11-09 18:28:29]

This overall looks fine in a mechanical way, barring using CamelCase for the
virtuals.

However, are there plans to reduce the size of the DownloadItem interface? Is
that much really needed by all users? Could you segment it into a few
interfaces, all of which DownloadItemImpl derives from?

[Randy Smith (Not in Mondays), 2011-11-09 18:39:15]

On 2011/11/09 18:28:29, cbentzel wrote:
> This overall looks fine in a mechanical way, barring using CamelCase for the
> virtuals.
> 
> However, are there plans to reduce the size of the DownloadItem interface? Is
> that much really needed by all users? Could you segment it into a few
> interfaces, all of which DownloadItemImpl derives from?

I endorse this request in the abstract, and any such simplifications that don't
take much time I'd encourage you to do as the spirit moves you, but at a high
level I think of us as doing testing this quarter and refactoring next, and
doing the interface simplification right really is refactoring.  And complex
refactoring is (somewhat) better done in the context of a larger plan, which is
what I hope to have next quarter.

I'd still love to see it happen, and I don't want to get in (either of your)
way(s) if you see a short-term win--I just want to be clear about priorities.

[Randy Smith (Not in Mondays), 2011-11-09 18:41:54]

As per recent offline email, please make newly virtual getters/setters
CamelCase.

[benjhayden, 2011-11-11 22:31:12]

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item.h (left):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:15: // receive state updates.
On 2011/11/09 17:09:57, rdsmith wrote:
> I'm inclined to leave this here.  It's a question of what's the "real"
download
> item from the perspective of someone reading this file.  I think of .h files
as
> being targeted at consumers, and thus the interface is the real download item.

Done.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:136: virtual ~DownloadItem();
On 2011/11/09 17:09:57, rdsmith wrote:
> If anyone else is going to own a DownloadItem, don't you need it to have a
> virtual destructor?

Done.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item.h:44: 
On 2011/11/09 17:09:57, rdsmith wrote:
> Why change ordering?  I think of the DownloadState as of more interest to
> DownloadItem consumers (not a request, just my perspective).

It wasn't intentional, it happened in one of the merges before I mailed it out.
Done.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/downl...
content/browser/download/download_item_impl.h:69: virtual void
AddObserver(DownloadItem::Observer* observer) OVERRIDE;
On 2011/11/09 17:09:57, rdsmith wrote:
> Note that these are all inherited from DownloadItem.

Done.

http://codereview.chromium.org/8503018/diff/10/content/browser/download/mock_...
File content/browser/download/mock_download_item.h (right):

http://codereview.chromium.org/8503018/diff/10/content/browser/download/mock_...
content/browser/download/mock_download_item.h:15: class MockDownloadItem :
public DownloadItem {
On 2011/11/09 17:09:57, rdsmith wrote:
> If you're very clear that you're going to be using this in a near-future CL,
I'm
> ok with it going in, but my rule of thumb is that new code should be put in
when
> it's first used, and I don't think this CL uses MockDownloadItem.
> 
> (Obviously, I'm good with doing refactoring without a specific use case :-}.)

I'm going to use it in the unittest for DownloadQuery. I was hoping to keep it
from rotting, but I can move it to my DownloadQuery CL if you want. This class
is at least instantiated in download_item_unittest in this CL so it won't rot.

[Randy Smith (Not in Mondays), 2011-11-11 22:47:56]

On 2011/11/11 22:31:12, b s h wrote:
> I'm going to use it in the unittest for DownloadQuery. I was hoping to keep it
> from rotting, but I can move it to my DownloadQuery CL if you want. This class
> is at least instantiated in download_item_unittest in this CL so it won't rot.

I'm ok if you want to keep it here, but I wanted to raise the issue.

[cbentzel, 2011-11-16 12:28:44]

LGTM

It doesn't look like MockDownloadItem is being used in this CL - but it doesn't
harm much to do it here.

http://codereview.chromium.org/8503018/diff/17083/chrome/browser/download/dow...
File chrome/browser/download/download_item_unittest.cc (right):

http://codereview.chromium.org/8503018/diff/17083/chrome/browser/download/dow...
chrome/browser/download/download_item_unittest.cc:256: TEST(MockDownloadItem,
Compiles) {
Do you want to remove this test?

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_item.h:27: namespace base {
Nit: forward declarations in a namespace come after the forward declarations
outside of a namespace. 

See: http://dev.chromium.org/developers/coding-style

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_item_impl.h:290: #endif  //
CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
This should be CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_

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

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_manager.h:62: struct
DownloadPersistentStoreInfo;
Was this needed due to changes in download_item.h?

[benjhayden, 2011-11-18 18:59:34]

Submitting this afternoon if there are no more comments.

http://codereview.chromium.org/8503018/diff/17083/chrome/browser/download/dow...
File chrome/browser/download/download_item_unittest.cc (right):

http://codereview.chromium.org/8503018/diff/17083/chrome/browser/download/dow...
chrome/browser/download/download_item_unittest.cc:256: TEST(MockDownloadItem,
Compiles) {
On 2011/11/16 12:28:44, cbentzel wrote:
> Do you want to remove this test?

No, because it ensures that MockDownloadItem doesn't rot while I work on my
DownloadQuery CL.

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_item.h:27: namespace base {
On 2011/11/16 12:28:44, cbentzel wrote:
> Nit: forward declarations in a namespace come after the forward declarations
> outside of a namespace. 
> 
> See: http://dev.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_item_impl.h:290: #endif  //
CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
On 2011/11/16 12:28:44, cbentzel wrote:
> This should be CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_

Done.

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

http://codereview.chromium.org/8503018/diff/17083/content/browser/download/do...
content/browser/download/download_manager.h:62: struct
DownloadPersistentStoreInfo;
On 2011/11/16 12:28:44, cbentzel wrote:
> Was this needed due to changes in download_item.h?

Huh, I guess I don't need that.

[commit-bot: I haz the power, 2011-11-18 21:02:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8503018/36001

[commit-bot: I haz the power, 2011-11-18 21:02:28]

Can't apply patch for file
chrome/browser/ui/views/download/download_item_view.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/download/download_item_view.cc
Hunk #1 FAILED at 211.
Hunk #2 FAILED at 229.
Hunk #3 FAILED at 337.
Hunk #4 FAILED at 358.
Hunk #5 succeeded at 567 (offset -67 lines).
Hunk #6 succeeded at 900 (offset -67 lines).
Hunk #7 succeeded at 1114 (offset 15 lines).
4 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/download/download_item_view.cc.rej

[commit-bot: I haz the power, 2011-11-18 22:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8503018/40001

[commit-bot: I haz the power, 2011-11-18 23:04:22]

Try job failure for 8503018-40001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-21 18:32:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8503018/43001

[commit-bot: I haz the power, 2011-11-21 19:54:54]

Change committed as 110984