DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

BUG=101214
BUG=106490
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113007

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

[Randy Smith (Not in Mondays), 2011-11-28 20:15:35]

This allows much cleaner, more self-contained unit tests for download item (as
shown in download_item_unittest.cc); not other "real" classes need to be
instantiated in order to test the download item.  There's a fair amount of
interface refactoring here.

Chris: Could you review this for crunchy architectural goodness?  (All files)
Andy: Could you review this for integration with the current downloads code? 
(All files)

[cbentzel, 2011-11-29 18:31:44]

May not get to it soon since memory sheriff is busy.

Note that you have a typo (intereface) in the CL description.

[cbentzel, 2011-11-29 21:30:43]

I didn't look in detail - was more focused on the overall classes.

I like that this removes direct dependencies of DownloadManager on DownloadItem.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:272: GetUserVerifiedFilePath());
Nit: this could probably fit on previous line.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:501:
delegate_->AssertStateConsistent(this);
It seems strange to have the DownloadItem ask the delegate to see if invariants
are upheld. Is there any way to move this checking into the delegate (aka
DownloadManagerImpl) and remove it from the Delegate interface?

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:631:
delegate_->BrowserContext());
Should DownloadItem::BrowserContext be used here instead?

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:41: class Delegate {
There's no documentation about lifetime expectations of the delegate relative to
the DownloadItemImpl.

It looks like it's expected to outlive the DownloadItemImpl since it's passed
into the constructor, retained as a raw pointer, and no set overrides. Would be
good to mention that though.

Should you do this, or should you have a way for the delegate to unregister?

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:50: virtual bool
ShouldOpenDownload(DownloadItem* download) = 0;
It's a little strange that the DownloadItemImpl::Delegate passes a DownloadItem*
in instead of a DownloadItemImpl* to many of the arguments. Does that indicate
that the delegate belongs in DownloadItem instead?

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_manager_impl.h:30: public
DownloadItemImpl::Delegate,
I think this is additional argument that the Delegate should be nested in
DownloadItem rather than DownloadItemImpl. It might be nice eventually to have
the DownloadManagerImpl work with mock DownloadItem's, as well as for
DownloadItemImpl to work with mock delegates.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_manager_impl.h:101: // present in both
interfaces.)
Ooh. Should that change? I suppose both will go to the same implementation.

[ahendrickson, 2011-11-29 21:40:15]

LGTM, with nits.

http://codereview.chromium.org/8697006/diff/8017/chrome/browser/download/down...
File chrome/browser/download/download_prefs.cc (right):

http://codereview.chromium.org/8697006/diff/8017/chrome/browser/download/down...
chrome/browser/download/download_prefs.cc:114: ChromeDownloadManagerDelegate*
delegate =
This part should call FromDownloadManager().

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:41: class Delegate {
Should this be part of DownloadItem instead of DownloadItemImpl?

[Randy Smith (Not in Mondays), 2011-11-30 22:44:04]

Incorporated comments; PTAL.

http://codereview.chromium.org/8697006/diff/8017/chrome/browser/download/down...
File chrome/browser/download/download_prefs.cc (right):

http://codereview.chromium.org/8697006/diff/8017/chrome/browser/download/down...
chrome/browser/download/download_prefs.cc:114: ChromeDownloadManagerDelegate*
delegate =
On 2011/11/29 21:40:15, ahendrickson wrote:
> This part should call FromDownloadManager().

Ok, but long term I want to get DownloadManager out of this pathway.  But then,
long-term I'd like to nuke this pathway, so that may be moot :-}.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:272: GetUserVerifiedFilePath());
On 2011/11/29 21:30:44, cbentzel wrote:
> Nit: this could probably fit on previous line.

Done.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:501:
delegate_->AssertStateConsistent(this);
On 2011/11/29 21:30:44, cbentzel wrote:
> It seems strange to have the DownloadItem ask the delegate to see if
invariants
> are upheld. Is there any way to move this checking into the delegate (aka
> DownloadManagerImpl) and remove it from the Delegate interface?

I tried, but it turned out that we're still getting crashes that are being
detected in the DownloadItem and might not be detected if we executed the
download item logic and did the assertion in DownloadManagerImpl.  In response
to your comment, I did a bit of a more thorough examination and found that:

* 15.0.874.121 has 5083 crashes in AssertStateConsistent, all of which are
coming from download manager.
* The max count of crashes post-15 is in 17.0.942.0, which has 36 crashes in
AssertStateConsistent, 19 of which are from downloads manager, and 17 from
download_item.cc.
* The pathway from download_item.cc is the Remove() pathway.  

So I think this CL needs to keep AssertStateConsistent, and that I should do
another swing at figuring out that crash in the near future :-J.

, but I didn't feel comfortable doing this while we were still having crashes in
the field that come from assertions inside of the DownloadItemImpl.  I can
certainly pull it out; it just involves removing all AssertStateConsistent calls
that are in the DownloadItem, and we're still occasionally tripping on some of
those :-J.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.cc:631:
delegate_->BrowserContext());
On 2011/11/29 21:30:44, cbentzel wrote:
> Should DownloadItem::BrowserContext be used here instead?

In an ideal world, DownloadItem wouldn't have a BrowserContext :-J.  But that's
a lot of plumbing (I looked).  I'll change it, but I hope I can yank it out
later.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:41: class Delegate {
On 2011/11/29 21:30:44, cbentzel wrote:
> There's no documentation about lifetime expectations of the delegate relative
to
> the DownloadItemImpl.
> 
> It looks like it's expected to outlive the DownloadItemImpl since it's passed
> into the constructor, retained as a raw pointer, and no set overrides. Would
be
> good to mention that though.
> 
> Should you do this, or should you have a way for the delegate to unregister?

My intent was to specify that the delegate must outlive the item; I'll put that
in.  I'm going to put the documentation at the constructor, which IMO is the
interface that the contract applies to.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:41: class Delegate {
On 2011/11/29 21:40:15, ahendrickson wrote:
> Should this be part of DownloadItem instead of DownloadItemImpl?

Nope.  It's irrelevant to the DownloadItem interface.  Specifically, if someone
wants to construct something that exports the DownloadItem interface (e.g. a
mock), there's no need for a delegate.  The currently implementation of
DownloadItem (i.e. DownloadItemImpl) is what's dependent on the delegate, so the
delegate definition belongs in the impl.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_item_impl.h:50: virtual bool
ShouldOpenDownload(DownloadItem* download) = 0;
On 2011/11/29 21:30:44, cbentzel wrote:
> It's a little strange that the DownloadItemImpl::Delegate passes a
DownloadItem*
> in instead of a DownloadItemImpl* to many of the arguments. Does that indicate
> that the delegate belongs in DownloadItem instead?

As I indicated to Andy above, I don't think so.  I agree that passing a
DownloadItem is weird, but I think (as we discussed offline) that where I'm
ending up is: the Delegate should be on the DownloadItemImpl since it's relevant
to the implementation of DownloadItemImpl, not to its interface (which is
DownloadItem), but I will leave the arguments to the delegate to DownloadItem's
so that classes that inherit from this interface may be used with other classes
(e.g. mocks) inherited from DownloadItem.  I'll put in a comment to that effect.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_manager_impl.h:30: public
DownloadItemImpl::Delegate,
On 2011/11/29 21:30:44, cbentzel wrote:
> I think this is additional argument that the Delegate should be nested in
> DownloadItem rather than DownloadItemImpl. It might be nice eventually to have
> the DownloadManagerImpl work with mock DownloadItem's, as well as for
> DownloadItemImpl to work with mock delegates.

As per our offline conversation and my comments elsewhere, leaving it on
DownloadItemImpl.

http://codereview.chromium.org/8697006/diff/8017/content/browser/download/dow...
content/browser/download/download_manager_impl.h:101: // present in both
interfaces.)
On 2011/11/29 21:30:44, cbentzel wrote:
> Ooh. Should that change? I suppose both will go to the same implementation. 

I agree with you it's not ideal, but I couldn't see a way to change
BrowserContext().  I *do* see a way to change MaybeCompleteDownload (shift the
interface over to being on the DownloadItemImpl only, so that the
DownloadManager doesn't need to export the thing).  I'll try to do that.  

BrowserContext() is needed in the delegate interface so that the DownloadItem
can figure out what BrowserContext it's on (which it needs as a substitute for
figuring out what DownloadManager it's on, which I no longer want it to know
about).  BrowserContext() is needed in the DownloadManager interface for some
operations in the DownloadsDOMHandler around file icons and translating back and
forth between incognito and regular profiles.  I'm sure there are better ways to
handle all those things (or at least the incognito/regular translation ...
stuff) but they aren't part of this CL.

[cbentzel, 2011-12-02 18:00:52]

LGTM for what this CL is.

I think there are a number of issues which should be addressed eventually but OK
if this helps testing.

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

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item.h:24: #include
"content/browser/download/download_id.h"
Why does this need to be included instead of forward declared as it had?

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

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:39: // Interface to delegate to
which DownloadItemImpl requests operations
"Interface to delegate to which" is confusing.

Perhaps this whole paragraph is unneeded since Delegate is a well-known term
throughout the Chrome code base. You could just adjust the next paragraph to be 

"Delegat is defined in DownloadItemImpl...."

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:54: virtual bool
ShouldOpenFileBasedOnExtension(const FilePath& path) = 0;
Probably for another CL - but seems like this should just pass the extension in
rather than the full path.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:63: virtual void
CheckForFileRemoval(DownloadItem* download_item) = 0;
Is it expected that this will call back into OnDownloadFileRemoved
asynchronously? Should that be documented?

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:63: virtual void
CheckForFileRemoval(DownloadItem* download_item) = 0;
Could this move to a const DownloadItem* argument? I looked at
DownloadManagerImpl's implementation and even though it takes a non-const it
looks like it could take a const easily.

It looks like const DownloadItem* may be reasonable for other members here. I
understand if you want to table that to another CL, as it may require a lot of
plumbing changes.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:73: // DownloadItem.
Should these move to DownloadItem::Observer?

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:85: // outlive the
DownloadItemImpl.
Nit: outlives? 

Should you put some debug checks in to make sure this is actually upheld?

For example:

class Delegate {
 public:
  Delegate() : count_(0) {}
  void Attach() { ++count_; }
  void Detach() { DCHECK(count_ > 0); --count_; }
  virtual ~Delegate() { DCHECK(count_ == 0); } 
 private:
  int count_;  
};

class DownloadItemImpl {
 public:
  DownloadItemImpl(Delegate* delegate) {
    delegate_ = delegate;
    delegate_->Attach();
  }
  virtual ~DownloadItemImpl() {
    delegate_->Detach();
  }
}

If the test coverage is good this may not be worth it because the memory bots
should catch the use-after-free problems. But may be worth considering.

[Randy Smith (Not in Mondays), 2011-12-02 20:04:12]

Chris: One question below about how I implemented Attach/Detach (and enough
extra code around Attach/Detach that another quick glance is probably
appropriate anyway.)

Your suggestions were all excellent; thank you.  Throughout this CL, I've been
walking a line between just doing the interface refactor and doing code
cleanup--I've done some cleanup, but postponed more into the refactoring we're
hopefully going to be doing next quarter.  I'm afraid I'm putting several of
your suggestions in that category; if there are any that I'm putting off that
you want me to jump on more quickly than "in a box for evaluation", please let
me know.

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

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item.h:24: #include
"content/browser/download/download_id.h"
On 2011/12/02 18:00:52, cbentzel wrote:
> Why does this need to be included instead of forward declared as it had? 

Well, I would have thought a by-value return of the type would require a full
declaration so that the calling code could know how much space to allocate for
the return value.  But I agree with you that the change I'm making here doesn't
look like it should produce a new need for including the file, so even though I
don't understand how the forward decl works: Done.

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

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:39: // Interface to delegate to
which DownloadItemImpl requests operations
On 2011/12/02 18:00:52, cbentzel wrote:
> "Interface to delegate to which" is confusing.
> 
> Perhaps this whole paragraph is unneeded since Delegate is a well-known term
> throughout the Chrome code base. You could just adjust the next paragraph to
be 
> 
> "Delegat is defined in DownloadItemImpl...."
> 

Done.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:54: virtual bool
ShouldOpenFileBasedOnExtension(const FilePath& path) = 0;
On 2011/12/02 18:00:52, cbentzel wrote:
> Probably for another CL - but seems like this should just pass the extension
in
> rather than the full path. 

Noted for another CL (this signature already existed and I'm just extending it
into a new area).

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:63: virtual void
CheckForFileRemoval(DownloadItem* download_item) = 0;
On 2011/12/02 18:00:52, cbentzel wrote:
> Is it expected that this will call back into OnDownloadFileRemoved
> asynchronously? Should that be documented? 

Done.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:63: virtual void
CheckForFileRemoval(DownloadItem* download_item) = 0;
On 2011/12/02 18:00:52, cbentzel wrote:
> Could this move to a const DownloadItem* argument? I looked at
> DownloadManagerImpl's implementation and even though it takes a non-const it
> looks like it could take a const easily.
> 
> It looks like const DownloadItem* may be reasonable for other members here. I
> understand if you want to table that to another CL, as it may require a lot of
> plumbing changes.

Yeah; I think I want to put this one of into another CL; doing a non-const to
const transformation often has a *lot* of dominos falling before it's done.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:73: // DownloadItem.
On 2011/12/02 18:00:52, cbentzel wrote:
> Should these move to DownloadItem::Observer?

Architecturally, definitely.  Practically, I'm wary of doing so just because
that code is right at the center of the timing/race issues that we've run into,
and thus I consider the change higher risk than anything else I've done here. 
But I'd definitely like to see it happen.

http://codereview.chromium.org/8697006/diff/17001/content/browser/download/do...
content/browser/download/download_item_impl.h:85: // outlive the
DownloadItemImpl.
On 2011/12/02 18:00:52, cbentzel wrote:
> Nit: outlives? 

Done.

> Should you put some debug checks in to make sure this is actually upheld?

I thought about this for a while, and flip-flopped since I didn't see a way to
just keep it in the delegate class, but eventually did it.  Do you think it's ok
to have the extra variable in the class in release, or should I throw in a whole
lot of (ugly :-}) if !defined(NDEBUG)s?

> 
> For example:
> 
> class Delegate {
>  public:
>   Delegate() : count_(0) {}
>   void Attach() { ++count_; }
>   void Detach() { DCHECK(count_ > 0); --count_; }
>   virtual ~Delegate() { DCHECK(count_ == 0); } 
>  private:
>   int count_;  
> };
> 
> class DownloadItemImpl {
>  public:
>   DownloadItemImpl(Delegate* delegate) {
>     delegate_ = delegate;
>     delegate_->Attach();
>   }
>   virtual ~DownloadItemImpl() {
>     delegate_->Detach();
>   }
> }
> 
> If the test coverage is good this may not be worth it because the memory bots
> should catch the use-after-free problems. But may be worth considering.

[cbentzel, 2011-12-02 20:12:56]

LGTM

I agree that many of my suggestions could be tabled for later. As long as you
are keeping a document of suggested changes this is OK.

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

http://codereview.chromium.org/8697006/diff/24001/content/browser/download/do...
content/browser/download/download_item_impl.h:52: void Attach();
I wonder if we want to keep these around for the long-term or just for now to
see if any new problems.

http://codereview.chromium.org/8697006/diff/24001/content/browser/download/do...
content/browser/download/download_item_impl.h:84: private:
Nit: add an extra newline between AssertStateConsistent and private.

[Randy Smith (Not in Mondays), 2011-12-02 20:26:29]

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

http://codereview.chromium.org/8697006/diff/24001/content/browser/download/do...
content/browser/download/download_item_impl.h:52: void Attach();
On 2011/12/02 20:12:56, cbentzel wrote:
> I wonder if we want to keep these around for the long-term or just for now to
> see if any new problems.

Just as much on the fence as I was about putting them in :-}.  We'll see what
happens; if they don't show anything up in the next couple of weeks, they're
probably not worth it, and I'll probably take them out.

http://codereview.chromium.org/8697006/diff/24001/content/browser/download/do...
content/browser/download/download_item_impl.h:84: private:
On 2011/12/02 20:12:56, cbentzel wrote:
> Nit: add an extra newline between AssertStateConsistent and private. 

Done.

[commit-bot: I haz the power, 2011-12-02 20:27:01]

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

[commit-bot: I haz the power, 2011-12-02 20:27:11]

Presubmit check for 8697006-21002 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:
content/public/browser/download_manager_delegate.h

Presubmit checks took 3.6s to calculate.

[jam, 2011-12-02 20:40:23]

content/public lgtm

[commit-bot: I haz the power, 2011-12-02 20:41:04]

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

[commit-bot: I haz the power, 2011-12-02 22:52:53]

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

[commit-bot: I haz the power, 2011-12-05 17:17:43]

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

[commit-bot: I haz the power, 2011-12-05 20:29:56]

Change committed as 113007

[Cris Neckar, 2011-12-06 00:30:21]

On 2011/12/05 20:29:56, I haz the power (commit-bot) wrote:
> Change committed as 113007

This broke the component build. I filed a bug at http://crbug.com/106490

[Randy Smith (Not in Mondays), 2011-12-06 14:45:44]

On 2011/12/06 00:30:21, Cris Neckar wrote:
> On 2011/12/05 20:29:56, I haz the power (commit-bot) wrote:
> > Change committed as 113007
> 
> This broke the component build. I filed a bug at http://crbug.com/106490

Chris: Thanks for trying to get a fix in, but it looks like my changed got
yanked out of the pool.  I'll resubmit with your fix.  

I wanted to confirm: The build that was broken was the windows debug shared
library build?  I want to make sure I've only got one problem to fix, and not
two.

[Randy Smith (Not in Mondays), 2011-12-06 14:50:01]

On 2011/12/06 14:45:44, rdsmith wrote:
> On 2011/12/06 00:30:21, Cris Neckar wrote:
> > On 2011/12/05 20:29:56, I haz the power (commit-bot) wrote:
> > > Change committed as 113007
> > 
> > This broke the component build. I filed a bug at http://crbug.com/106490
> 
> Chris: Thanks for trying to get a fix in, but it looks like my changed got
> yanked out of the pool.  I'll resubmit with your fix.  
> 
> I wanted to confirm: The build that was broken was the windows debug shared
> library build?  I want to make sure I've only got one problem to fix, and not
> two.

One other question: Is there a way to build on linux that confirms that the
component build works?  I need to incorporate testing this into my workflow
somehow, and that would be the easiest way.

[commit-bot: I haz the power, 2011-12-06 18:21:57]

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

[commit-bot: I haz the power, 2011-12-06 23:28:14]

Change committed as 113277