[Downloads] Ask DownloadHistory if a download was from history.

[Downloads] Ask DownloadHistory if a download was from history.

Observers of downloads currently assume that any download that is
created in the IN_PROGRESS state must be a new download, while all
others were restored from history. Pending changes to how downloads are
created in response to failed requests will mean that this assumption
will no longer hold.

This CL adds a WasRestoredFromHistory() method to DownloadHistory which
determines whether a given download was restored from history.

BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270749

[Randy Smith (Not in Mondays), 2014-04-10 18:03:23]

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_history.h (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_history.h:72: // persisted state may not have
been updated yet a download that were restored
nit: "yet *for* a download"?

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_history.h:93: // this specific DownloadHistory
instance.
Hmmm.  Not relevant for this CL, but I'm inclined to think that we should decide
whether we support multiple DownloadHistory instances (and not unless there's a
use case) and enforce that in the code.  It feels like the kind of thing that
could bit rot very easily (if we allow multiple instances but don't test them).

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.cc:139: if
(!new_download_filter_.Run(item))
There's something basic I'm not understanding here.  This code behaves the same
(modulo the change in this CL) as the old code, but why is that necessary?  If
we're restoring a download from the history, don't we want it to show up in the
UI as well?

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:51: DownloadHistory*
download_history);
I dislike cluttering up class interfaces for testing purposes (which is what I
think this is; is that right?).  Given that the interface needs to "know" about
DownloadHistory, I'd think it would make more sense to just pass a
DownloadHistory argument to the constructor and skip the extra mechanism.  Did
you explore how hard it would be to mock DownloadHistory to allow tests to
control this bit?

I'm willing to go with your preference on this--the test code will be more
complicated and messy with my suggested change, and arguably all the interface
wants is a way to tell whether or not the incoming download is one it needs to
pay attention to.  But the interface would be simpler without the filter, and if
we take the "all the interface wants is a way to tell whether or not the
incoming download is one it needs to pay attention to" we should replace the
DownloadManager with some kind of download item source that has the filter
embedded in it.  Which seems like a lot of work for just this use case.

[asanka, 2014-04-17 21:16:09]

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_history.h (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_history.h:72: // persisted state may not have
been updated yet a download that were restored
On 2014/04/10 18:03:23, rdsmith wrote:
> nit: "yet *for* a download"?

Done.

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_history.h:93: // this specific DownloadHistory
instance.
On 2014/04/10 18:03:23, rdsmith wrote:
> Hmmm.  Not relevant for this CL, but I'm inclined to think that we should
decide
> whether we support multiple DownloadHistory instances (and not unless there's
a
> use case) and enforce that in the code.  It feels like the kind of thing that
> could bit rot very easily (if we allow multiple instances but don't test
them).

Yeah. Currently there's nothing that prevents multiple instances of
DownloadHistory from being associated with a single DownloadManager. There's
only one in practice.

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.cc:139: if
(!new_download_filter_.Run(item))
On 2014/04/10 18:03:23, rdsmith wrote:
> There's something basic I'm not understanding here.  This code behaves the
same
> (modulo the change in this CL) as the old code, but why is that necessary?  If
> we're restoring a download from the history, don't we want it to show up in
the
> UI as well?

This code was refactored somewhat, but the question is still valid.

Perhaps this class should be renamed, but its purpose is to pick out downloads
that the user should be actively displayed to the user, and then figure out how
to display such downloads. There's already a mechanism for checking for all
downloads that download history is using.

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (right):

https://codereview.chromium.org/230103002/diff/20001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:51: DownloadHistory*
download_history);
On 2014/04/10 18:03:23, rdsmith wrote:
> I dislike cluttering up class interfaces for testing purposes (which is what I
> think this is; is that right?).  Given that the interface needs to "know"
about
> DownloadHistory, I'd think it would make more sense to just pass a
> DownloadHistory argument to the constructor and skip the extra mechanism.  Did
> you explore how hard it would be to mock DownloadHistory to allow tests to
> control this bit?
> 
> I'm willing to go with your preference on this--the test code will be more
> complicated and messy with my suggested change, and arguably all the interface
> wants is a way to tell whether or not the incoming download is one it needs to
> pay attention to.  But the interface would be simpler without the filter, and
if
> we take the "all the interface wants is a way to tell whether or not the
> incoming download is one it needs to pay attention to" we should replace the
> DownloadManager with some kind of download item source that has the filter
> embedded in it.  Which seems like a lot of work for just this use case.
> 

I refactored this. PTAL.

[Randy Smith (Not in Mondays), 2014-04-21 18:21:22]

This LGTM (with the notes below), but in reviewing the test I find myself
uncomfortable with the UI dispatch architecture we've got.  It feels as if, in
trying to provide the various different UIs (shelf, Android, downloads.html, I
think) with the notifications they need, we're just turing DownloadUIController
and DownloadItemModel into a grab bag.  Maybe it's worthwhile taking a step back
and figuring out if there's a better abstraction?  Maybe a separate observer/ui
controller for each UI that just worries about what it needs?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_history_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:538:
ExpectWillQueryDownloads(infos.Pass());
This may be due to my own cluelessness (I kept mistaking the version of
ExpectWillQueryDownloads on the history adapter for the one on the
DownloadHistoryTest that's actually called by this function) but could you
clarify that this function actually does something rather than setup an
expectation?  By changing the name or putting in a comment or something similar.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:555:
CallOnDownloadCreated(0);
It was harder to understand these tests because of the lack of parallelism
between them.  Maybe add parallel comments so that it's clear what portions of
the two tests are parallel, and the reader gets a clue why they're different?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_item_model.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_item_model.cc:578: // resumed, then ideally the
UI should be notified.
FWIW, I sorta think this has to be true before we consider resumption working.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.cc:71: 
nit: Did you intentionally put the newline in?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:10: #include
"base/callback_forward.h"
Is this include still needed?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:15: class DownloadHistory;
Is this forward decl still needed?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:85: // DownloadItem
is being destroyed.
This is the second time this functionality has been needed (FakeDownloadItem in
download_path_reservation_tracker_unittest.cc).  Worthwhile pulling it into
MockDownloadItem?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:210:
EXPECT_CALL(*manager_, AddObserver(_))
At this point, I trust your expertise with gmock more than mine, but shouldn't
there be a verification that the last expect actually happened before it's
replaced?

[asanka, 2014-04-23 05:51:09]

Noted the concern about UI observers. I do have a plan for cleaning that up, but
I'll do that in a separate CL.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_history_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:538:
ExpectWillQueryDownloads(infos.Pass());
On 2014/04/21 18:21:22, rdsmith wrote:
> This may be due to my own cluelessness (I kept mistaking the version of
> ExpectWillQueryDownloads on the history adapter for the one on the
> DownloadHistoryTest that's actually called by this function) but could you
> clarify that this function actually does something rather than setup an
> expectation?  By changing the name or putting in a comment or something
similar.

I changed the name.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:555:
CallOnDownloadCreated(0);
On 2014/04/21 18:21:22, rdsmith wrote:
> It was harder to understand these tests because of the lack of parallelism
> between them.  Maybe add parallel comments so that it's clear what portions of
> the two tests are parallel, and the reader gets a clue why they're different?

Hmm. Yeah. I added some comments that should explain what's going on. Does it
look better now?

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_item_model.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_item_model.cc:578: // resumed, then ideally the
UI should be notified.
On 2014/04/21 18:21:22, rdsmith wrote:
> FWIW, I sorta think this has to be true before we consider resumption working.

Noted.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.cc:71: 
On 2014/04/21 18:21:22, rdsmith wrote:
> nit: Did you intentionally put the newline in?

Nope. Removed.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:10: #include
"base/callback_forward.h"
On 2014/04/21 18:21:22, rdsmith wrote:
> Is this include still needed?

Removed.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:15: class DownloadHistory;
On 2014/04/21 18:21:22, rdsmith wrote:
> Is this forward decl still needed?

Removed.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:85: // DownloadItem
is being destroyed.
On 2014/04/21 18:21:22, rdsmith wrote:
> This is the second time this functionality has been needed (FakeDownloadItem
in
> download_path_reservation_tracker_unittest.cc).  Worthwhile pulling it into
> MockDownloadItem?

Good catch. I added it to MockDownloadItem in a separate CL (different owners
etc..) This CL now depends on that CL.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:210:
EXPECT_CALL(*manager_, AddObserver(_))
On 2014/04/21 18:21:22, rdsmith wrote:
> At this point, I trust your expertise with gmock more than mine, but shouldn't
> there be a verification that the last expect actually happened before it's
> replaced?

I changed the expectation to explicitly require matching the current value of
manager_observer_. That should be the equivalent of what you are asking since it
would only match if the AddObserver was already seen.

[Randy Smith (Not in Mondays), 2014-04-23 19:18:57]

Only one concern left.

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:210:
EXPECT_CALL(*manager_, AddObserver(_))
On 2014/04/23 05:51:09, asanka wrote:
> On 2014/04/21 18:21:22, rdsmith wrote:
> > At this point, I trust your expertise with gmock more than mine, but
shouldn't
> > there be a verification that the last expect actually happened before it's
> > replaced?
> 
> I changed the expectation to explicitly require matching the current value of
> manager_observer_. That should be the equivalent of what you are asking since
it
> would only match if the AddObserver was already seen.

Sorry, still confused.  There are two EXPECT_CALL(*manager_,
AddObserver(_)).WillOnce() calls in this function.  What prevents the first from
not being tripped (i.e. tripped zero times) before being overridden by the
second?

[asanka, 2014-05-07 23:19:53]

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:210:
EXPECT_CALL(*manager_, AddObserver(_))
On 2014/04/23 19:18:57, rdsmith wrote:
> On 2014/04/23 05:51:09, asanka wrote:
> > On 2014/04/21 18:21:22, rdsmith wrote:
> > > At this point, I trust your expertise with gmock more than mine, but
> shouldn't
> > > there be a verification that the last expect actually happened before it's
> > > replaced?
> > 
> > I changed the expectation to explicitly require matching the current value
of
> > manager_observer_. That should be the equivalent of what you are asking
since
> it
> > would only match if the AddObserver was already seen.
> 
> Sorry, still confused.  There are two EXPECT_CALL(*manager_,
> AddObserver(_)).WillOnce() calls in this function.  What prevents the first
from
> not being tripped (i.e. tripped zero times) before being overridden by the
> second?

There's an ASSERT_TRUE(download_history_manager_observer_) that should only pass
if the first AddObserver() has already performed its action. I can add
checkpoints along the way, but I think the current setup sufficiently verifies
that the first expectation is met before we get  here.

[Randy Smith (Not in Mondays), 2014-05-08 17:21:04]

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/230103002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller_unittest.cc:210:
EXPECT_CALL(*manager_, AddObserver(_))
On 2014/05/07 23:19:53, asanka wrote:
> On 2014/04/23 19:18:57, rdsmith wrote:
> > On 2014/04/23 05:51:09, asanka wrote:
> > > On 2014/04/21 18:21:22, rdsmith wrote:
> > > > At this point, I trust your expertise with gmock more than mine, but
> > shouldn't
> > > > there be a verification that the last expect actually happened before
it's
> > > > replaced?
> > > 
> > > I changed the expectation to explicitly require matching the current value
> of
> > > manager_observer_. That should be the equivalent of what you are asking
> since
> > it
> > > would only match if the AddObserver was already seen.
> > 
> > Sorry, still confused.  There are two EXPECT_CALL(*manager_,
> > AddObserver(_)).WillOnce() calls in this function.  What prevents the first
> from
> > not being tripped (i.e. tripped zero times) before being overridden by the
> > second?
> 
> There's an ASSERT_TRUE(download_history_manager_observer_) that should only
pass
> if the first AddObserver() has already performed its action. I can add
> checkpoints along the way, but I think the current setup sufficiently verifies
> that the first expectation is met before we get  here.

Ah, sorry, missed that.  LGTM.

[asanka, 2014-05-15 14:50:27]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-05-15 14:50:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asanka@chromium.org/230103002/120001

[commit-bot: I haz the power, 2014-05-15 18:35:23]

Change committed as 270749