Make DownloadHistory observe manager, items

Make DownloadHistory observe manager, items

Rip out half of DownloadManagerDelegate.
Make DownloadManager create persisted DownloadItems one at a time and return them to DownloadHistory.
Move DownloadPersistentStoreInfo from content to chrome.
Kill DownloadDatabase::CheckThread(). (Leftover from 85408.)
Change DownloadDatabase::RemoveDownloads() to take an explicit set of db_handles.
Merge DownloadDatabase::UpdateDownload[Path]().

After this CL, I'll send out another one to remove the usage of CancelableRequest from the downloads-specific HistoryService APIs.

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

[benjhayden, 2012-09-10 19:08:15]

PTAL
Moved from http://codereview.chromium.org/10665049/ in order to get the
automated CC line, which is only generated when the first comment in a CL is
from the author?

[Randy Smith (Not in Mondays), 2012-09-11 17:40:08]

On 2012/09/10 19:08:15, benjhayden_chromium wrote:
> PTAL
> Moved from http://codereview.chromium.org/10665049/ in order to get the
> automated CC line, which is only generated when the first comment in a CL is
> from the author?

Now that I look, this may well have been my bad.  That initial comment was
intended to be just to you (you hadn't really publicized it for review yet) and
so I may have decided to send it just to you, and not updated the cc line
afterwards.  I probably figured it would auto-regenerate :-{.

[Randy Smith (Not in Mondays), 2012-09-11 18:36:52]

Next round of comments.  This is getting exciting :-).

One high level question ('cause I'm lazy, though I'm eventually going to want to
track this through thoroughly): Currently (pre-this-CL) there's a race between
canceling and persistence completion for save package download items.  Was there
any fanciness necessary to deal with that race?

Haven't yet reviewed tests, but we're getting close with the main code.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:598: if
(download_history_.get()) {
I believe this changes behavior; do you agree?  (Key question.)

If it does change behavior, I'm inclined to think it's not a behavior change we
want.  As I understand it, incognito mode is allowed to "see" everything about
non-incognito mode, it simply shouldn't leave any footprints.  So if we've
visited this website previously in non-incognito mode, we should be able to find
that out.

If it doesn't change behavior, than I'm sure someone smarter than me had a
reason why we shouldn't see previous visits in non-incognito mode :-}.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:80: // This allows
OnDownloadUpdated() to see what changed in a DownloadItem if
nit: DownloadHistory::OnDownloadUpdated().

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:171: manager_->AddObserver(this);
What about DownloadItems that were created on the DownloadManager before
DownloadHistory was constructed?

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:209: // If this DCHECK fails, then
you probably added an Observer that
nit, suggestion: Blank line before comment?  (I.e. I think it makes it more
readable, but it's a personal preference.)

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:214: //
DownloadHistory::OnDownloadCreated.
I like this DCHECK and comment, but if you're relying on consumers not doing
this, you should document the interface and tell them that they shouldn't do it.
 (I.e. put a note next to the OnDownloadCreated() definition.)

(You could also make your code resilient against this by postponing handling all
OnDownloadCreated() calls until this loop is completed, then doing a
set-difference on the ids you get here and the ids you got from
OnDownloadCreated().  Or you could put in a TODO() to do that, given that we
don't currently need it.  I don't personally like any of the options :-J, so
it's up to you, I'm just laying them out.)

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:297: DCHECK(db_handle >
history::DownloadDatabase::kUninitializedHandle);
nit: !=

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.h:30: HistoryService* history);
Comment that constructor must guarantee that HistoryService outlives
DownloadHistory?

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:282: if (num_downloads_not_deleted
>= 0) {
I'm good with these stats--I think I was misreading >= as > last time :-{.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
chrome/browser/history/history.cc:606: void HistoryService::QueryDownloads(
nit: This and next line look like they could fit on one line?

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1209: new
std::vector<DownloadPersistentStoreInfo>());
Random thought: Probably worthwhile making sure you do a valgrind trybot run
before landing.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (left):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:419:
DCHECK(file->IsPersisted());
Any idea why this check was here in the first place?

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/ba...
content/browser/download/base_file.cc:268: if (detached_) return
net::ERR_INVALID_HANDLE;
You didn't respond on the other CL, so I'll ask again: Why this change?  It
seems like a very big behavioral change that goes against how I'd personally
like the class to behave.  Why?

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_item_impl.cc:944:
net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
I dislike the name of this event, given that we're pulling the history concept
out of core downloads.  How would you feel about changing it?

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:574:
OnDownloadTargetDetermined(
I don't follow where this came from; it doesn't look like the old code path
called OnDownloadTargetDetermined() on SP downloads.  Enlighten me?

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:642: if
(download->GetTargetFilePath().DirName() !=
Could you comment this test?  Something like "If the intermediate filename
rename hasn't occurred, don't complete the download." maybe with an explanation
as to why there are two tests for that?

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:966: // TODO(benjhayden) this
should be a method on DI
I'm confused by this comment.  It is a method on the DI(I);
OnDownloadRenamedToIntermediateName() calls this method.  If you mean that
MaybeCompleteDownload() should be pulled down into DII, completely agreed, and I
think we're almost there.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:974: // TODO(benjhayden) this
should be a method on DI
Same comment as above, with the twist that given this refactoring, you could
actually remove this method from the DIID & DMI.  Want to?  (I'm ok either way,
but it's more deleted code.)

http://codereview.chromium.org/10915180/diff/8001/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/public/test/downloa...
content/public/test/download_test_observer.cc:139: DownloadSet::const_iterator
finished_it(finished_downloads_.find(item));
How could item ever be in finished_downloads_ when OnDownloadCreated() is
called?  Won't that be the first time that anyone anywhere hears about the item?

[benjhayden, 2012-09-13 15:18:16]

PTAL, but ignore SavePackage until I figure out what to do. Maybe we can talk
about SavePackage IRL this afternoon.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:598: if
(download_history_.get()) {
On 2012/09/11 18:36:53, rdsmith wrote:
> I believe this changes behavior; do you agree?  (Key question.)

This does not change behavior.
Before this CL, DownloadHistory::CheckVisitedReferrerBefore() immediately passes
false to the callback (CheckVisitedReferrerBeforeDone()) if HistoryService is
unavailable.

> 
> If it does change behavior, I'm inclined to think it's not a behavior change
we
> want.  As I understand it, incognito mode is allowed to "see" everything about
> non-incognito mode, it simply shouldn't leave any footprints.  So if we've
> visited this website previously in non-incognito mode, we should be able to
find
> that out.
> 
> If it doesn't change behavior, than I'm sure someone smarter than me had a
> reason why we shouldn't see previous visits in non-incognito mode :-}.

I don't understand.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:80: // This allows
OnDownloadUpdated() to see what changed in a DownloadItem if
On 2012/09/11 18:36:53, rdsmith wrote:
> nit: DownloadHistory::OnDownloadUpdated().

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:171: manager_->AddObserver(this);
On 2012/09/11 18:36:53, rdsmith wrote:
> What about DownloadItems that were created on the DownloadManager before
> DownloadHistory was constructed?

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:209: // If this DCHECK fails, then
you probably added an Observer that
On 2012/09/11 18:36:53, rdsmith wrote:
> nit, suggestion: Blank line before comment?  (I.e. I think it makes it more
> readable, but it's a personal preference.)

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:214: //
DownloadHistory::OnDownloadCreated.
On 2012/09/11 18:36:53, rdsmith wrote:
> I like this DCHECK and comment, but if you're relying on consumers not doing
> this, you should document the interface and tell them that they shouldn't do
it.
>  (I.e. put a note next to the OnDownloadCreated() definition.)
> 
> (You could also make your code resilient against this by postponing handling
all
> OnDownloadCreated() calls until this loop is completed, then doing a
> set-difference on the ids you get here and the ids you got from
> OnDownloadCreated().  Or you could put in a TODO() to do that, given that we
> don't currently need it.  I don't personally like any of the options :-J, so
> it's up to you, I'm just laying them out.)

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:297: DCHECK(db_handle >
history::DownloadDatabase::kUninitializedHandle);
On 2012/09/11 18:36:53, rdsmith wrote:
> nit: !=

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/dow...
chrome/browser/download/download_history.h:30: HistoryService* history);
On 2012/09/11 18:36:53, rdsmith wrote:
> Comment that constructor must guarantee that HistoryService outlives
> DownloadHistory?

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:282: if (num_downloads_not_deleted
>= 0) {
On 2012/09/11 18:36:53, rdsmith wrote:
> I'm good with these stats--I think I was misreading >= as > last time :-{.

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
chrome/browser/history/history.cc:606: void HistoryService::QueryDownloads(
On 2012/09/11 18:36:53, rdsmith wrote:
> nit: This and next line look like they could fit on one line?

Done.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1209: new
std::vector<DownloadPersistentStoreInfo>());
On 2012/09/11 18:36:53, rdsmith wrote:
> Random thought: Probably worthwhile making sure you do a valgrind trybot run
> before landing.

TODOne

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (left):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:419:
DCHECK(file->IsPersisted());
On 2012/09/11 18:36:53, rdsmith wrote:
> Any idea why this check was here in the first place?

When a user clicks "Remove" on chrome://downloads, they expect it to disappear.
If this item wasn't persisted, then Remove()ing it might not disappear it. We'd
want to catch that.
Now, though, DownloadManager removes it from its downloads_ map regardless of
what DownloadDatabase says, so it will disappear.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/ba...
content/browser/download/base_file.cc:268: if (detached_) return
net::ERR_INVALID_HANDLE;
On 2012/09/11 18:36:53, rdsmith wrote:
> You didn't respond on the other CL, so I'll ask again: Why this change?  It
> seems like a very big behavioral change that goes against how I'd personally
> like the class to behave.  Why?

I had found a race that meant that there might be an AppendDataToFile call after
Detach. I forget where the race was, so I'll try backing out this change and see
if it crops back up.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_item_impl.cc:944:
net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
On 2012/09/11 18:36:53, rdsmith wrote:
> I dislike the name of this event, given that we're pulling the history concept
> out of core downloads.  How would you feel about changing it?

Done.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:642: if
(download->GetTargetFilePath().DirName() !=
On 2012/09/11 18:36:53, rdsmith wrote:
> Could you comment this test?  Something like "If the intermediate filename
> rename hasn't occurred, don't complete the download." maybe with an
explanation
> as to why there are two tests for that?

Done.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:966: // TODO(benjhayden) this
should be a method on DI
On 2012/09/11 18:36:53, rdsmith wrote:
> I'm confused by this comment.  It is a method on the DI(I);
> OnDownloadRenamedToIntermediateName() calls this method.  If you mean that
> MaybeCompleteDownload() should be pulled down into DII, completely agreed, and
I
> think we're almost there.

Clarified the comment.

http://codereview.chromium.org/10915180/diff/8001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:974: // TODO(benjhayden) this
should be a method on DI
On 2012/09/11 18:36:53, rdsmith wrote:
> Same comment as above, with the twist that given this refactoring, you could
> actually remove this method from the DIID & DMI.  Want to?  (I'm ok either
way,
> but it's more deleted code.)

Done.

http://codereview.chromium.org/10915180/diff/8001/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/8001/content/public/test/downloa...
content/public/test/download_test_observer.cc:139: DownloadSet::const_iterator
finished_it(finished_downloads_.find(item));
On 2012/09/11 18:36:53, rdsmith wrote:
> How could item ever be in finished_downloads_ when OnDownloadCreated() is
> called?  Won't that be the first time that anyone anywhere hears about the
item?

OnDownloadUpdated() is called before this code. That method may call
DownloadInFinalState(), which adds the item to finished_downloads_.
Yes, this is gross. I'll probably rewrite DTO and its accomplices to use DUO and
HDIN before this CL is committed.

[Randy Smith (Not in Mondays), 2012-09-13 19:53:19]

Looks good except for save package and the comments below.

I'd recommend pulling Brett in for the history changes at this point; I doubt
you're going to modify that much before the review with Brett, so no point in
not parallelizing that and the save package work.

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/8001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:598: if
(download_history_.get()) {
On 2012/09/13 15:18:16, benjhayden_chromium wrote:
> On 2012/09/11 18:36:53, rdsmith wrote:
> > I believe this changes behavior; do you agree?  (Key question.)
> 
> This does not change behavior.
> Before this CL, DownloadHistory::CheckVisitedReferrerBefore() immediately
passes
> false to the callback (CheckVisitedReferrerBeforeDone()) if HistoryService is
> unavailable.
> 
> > 
> > If it does change behavior, I'm inclined to think it's not a behavior change
> we
> > want.  As I understand it, incognito mode is allowed to "see" everything
about
> > non-incognito mode, it simply shouldn't leave any footprints.  So if we've
> > visited this website previously in non-incognito mode, we should be able to
> find
> > that out.
> > 
> > If it doesn't change behavior, than I'm sure someone smarter than me had a
> > reason why we shouldn't see previous visits in non-incognito mode :-}.
> 
> I don't understand.

As discussed offline: The original code does appear to redirect to the original
(non-incognito) profile for this query if we're in incognito mode, and we should
too.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:25: LOG(ERROR) <<
left.path.value() << " != " << right.path.value();
Wow, this is going to produce a lot of output, and at a fairly high log level
too.  This might be ok if it only happens when tests fail, but if you call
InfoEqual sometimes expecting the infos not to be equal (which seems like a
reasonable thing for some future test to do even if no current one does) this is
probably overkill.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:64: : public
HistoryService{
nit, suggestion: I think it's more customary to have these on the same line.

nit: I'm pretty sure that a space before the { is required.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:68: handle_counter_(0) {
nit: Curlies on same line is allowed but not required, and I think it our usual
convention.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:321:
DownloadHistoryTest_Load) {
nit, suggestion: It looks really weird to me to have these on different lines.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:337: {
Why the extra curlies?

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:348:
ExpectUpdateDownload(info);
For what it's worth, I find it confusing to read through a test when in some
cases Expect* sometimes means "in the future I expect to see this" and sometimes
means "I expect I will already have seen this".  I know that we already have
that distinction in the support libraries (EXPECT_EQ versus EXPECT_CALL), but
would you be willing to make the distinction in your routine names for routines
you write?

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:361:
IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
Intro line or two before tests describing what you're testing?  (Applies to all
tests.)

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:365:
ExpectQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
Huh; I wouldn't have thought this would compile (I'd think you'd need the
.Pass() at this level too).

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:467:
DownloadHistoryTest_RemoveWhileAdding) {
Do you test the straight remove path anywhere in here?

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_item_impl.cc:936: if (active) {
There's enough duplicated code here (like, all of it :-}) that I'd probably
setup a temporary variable that was a pointer to class method, set it based on
active, and do the call once.  But I'm good either way--just raising the idea in
case it appeals.

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
So this is an example of the issue we were talking about yesterday.  If we can't
have a simple test as to whether we're at the right place in the state diagram,
I want to re-visit the question of whether we have the right/enough states. 
What I said yesterday is "If we can't just test the target filename, revisit",
but I think I'm going to back off from that and say that this logic is
(effectively) on the inside of the download item, so we can rely on inside
knowledge of what's going on.  In which case, just testing the FullPath for null
should work.  Do you agree?

(I'll note that this suggestion makes me a little ootsy.  The question we
*really* want to ask here is "Where are we in the main download cascade?" and
we're using the full file path nullity as a proxy for that.  I believe that it's
true that the full file path will never be non null when we haven't gotten to
this place in the download cascade, but it's not quite as direct a question as,
well, say, checking the state variable :-J.)

http://codereview.chromium.org/10915180/diff/15001/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/public/test/downlo...
content/public/test/download_test_observer.cc:134: void
DownloadTestObserver::OnDownloadCreated(
It seems wrong to use *both* OnDownloadCreated() and ModelChanged() in any
class; would it be reasonable to nuke ModelChanged() here?

[benjhayden, 2012-09-21 20:45:46]

PTAL

I decided to keep CancelableRequestConsumer in DownloadHistory and not simplify
our corner of HistoryService in order to keep this CL less controversial.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:25: LOG(ERROR) <<
left.path.value() << " != " << right.path.value();
On 2012/09/13 19:53:19, rdsmith wrote:
> Wow, this is going to produce a lot of output, and at a fairly high log level
> too.  This might be ok if it only happens when tests fail, but if you call
> InfoEqual sometimes expecting the infos not to be equal (which seems like a
> reasonable thing for some future test to do even if no current one does) this
is
> probably overkill.

Done.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:64: : public
HistoryService{
On 2012/09/13 19:53:19, rdsmith wrote:
> nit, suggestion: I think it's more customary to have these on the same line.
> 
> nit: I'm pretty sure that a space before the { is required.

Done.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:68: handle_counter_(0) {
On 2012/09/13 19:53:19, rdsmith wrote:
> nit: Curlies on same line is allowed but not required, and I think it our
usual
> convention.

When the constructor spans multiple lines, I like how the close curly indicates
the end of the constructor.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:321:
DownloadHistoryTest_Load) {
On 2012/09/13 19:53:19, rdsmith wrote:
> nit, suggestion: It looks really weird to me to have these on different lines.

Done.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:337: {
On 2012/09/13 19:53:19, rdsmith wrote:
> Why the extra curlies?

To delineate the lifetime of infos.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:348:
ExpectUpdateDownload(info);
On 2012/09/13 19:53:19, rdsmith wrote:
> For what it's worth, I find it confusing to read through a test when in some
> cases Expect* sometimes means "in the future I expect to see this" and
sometimes
> means "I expect I will already have seen this".  I know that we already have
> that distinction in the support libraries (EXPECT_EQ versus EXPECT_CALL), but
> would you be willing to make the distinction in your routine names for
routines
> you write?

Done.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:361:
IN_PROC_BROWSER_TEST_F(DownloadHistoryTest,
On 2012/09/13 19:53:19, rdsmith wrote:
> Intro line or two before tests describing what you're testing?  (Applies to
all
> tests.)

Done.

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:365:
ExpectQueryDownloads(scoped_ptr<InfoVector>(new InfoVector()));
On 2012/09/13 19:53:19, rdsmith wrote:
> Huh; I wouldn't have thought this would compile (I'd think you'd need the
> .Pass() at this level too).

I think this is what compilers call a "temporary".

http://codereview.chromium.org/10915180/diff/15001/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:467:
DownloadHistoryTest_RemoveWhileAdding) {
On 2012/09/13 19:53:19, rdsmith wrote:
> Do you test the straight remove path anywhere in here?

DownloadHistoryTest_Create tests OnDownloadRemoved().

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_item_impl.cc:936: if (active) {
On 2012/09/13 19:53:19, rdsmith wrote:
> There's enough duplicated code here (like, all of it :-}) that I'd probably
> setup a temporary variable that was a pointer to class method, set it based on
> active, and do the call once.  But I'm good either way--just raising the idea
in
> case it appeals.

I named the callback instead. How does it look now?

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
On 2012/09/13 19:53:19, rdsmith wrote:
> So this is an example of the issue we were talking about yesterday.  If we
can't
> have a simple test as to whether we're at the right place in the state
diagram,
> I want to re-visit the question of whether we have the right/enough states. 
> What I said yesterday is "If we can't just test the target filename, revisit",
> but I think I'm going to back off from that and say that this logic is
> (effectively) on the inside of the download item, so we can rely on inside
> knowledge of what's going on.  In which case, just testing the FullPath for
null
> should work.  Do you agree?
> 
> (I'll note that this suggestion makes me a little ootsy.  The question we
> *really* want to ask here is "Where are we in the main download cascade?" and
> we're using the full file path nullity as a proxy for that.  I believe that
it's
> true that the full file path will never be non null when we haven't gotten to
> this place in the download cascade, but it's not quite as direct a question
as,
> well, say, checking the state variable :-J.)

Can I let you fix this when you put DownloadItem in the driver's seat?

http://codereview.chromium.org/10915180/diff/15001/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/public/test/downlo...
content/public/test/download_test_observer.cc:134: void
DownloadTestObserver::OnDownloadCreated(
On 2012/09/13 19:53:19, rdsmith wrote:
> It seems wrong to use *both* OnDownloadCreated() and ModelChanged() in any
> class; would it be reasonable to nuke ModelChanged() here?  

Done.

[Randy Smith (Not in Mondays), 2012-09-24 18:03:24]

I haven't reviewed SavePackage interaction yet.  When you have a chance, could
you give me a PS <-> SVN rev equivalent and I'll patch it into a local sandbox
and trace out the SP flow?

Also, it looks like you've removed a lot of testing.  Generally my heart lifts
at seeing code removal, but less at test removal :-}.  Are you comfortable that
your test coverage (at both unit and browser level) are as good after this CL as
before it?

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_item_impl.cc:936: if (active) {
On 2012/09/21 20:45:46, benjhayden_chromium wrote:
> On 2012/09/13 19:53:19, rdsmith wrote:
> > There's enough duplicated code here (like, all of it :-}) that I'd probably
> > setup a temporary variable that was a pointer to class method, set it based
on
> > active, and do the call once.  But I'm good either way--just raising the
idea
> in
> > case it appeals.
> 
> I named the callback instead. How does it look now?

Looks good.

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
On 2012/09/21 20:45:46, benjhayden_chromium wrote:
> On 2012/09/13 19:53:19, rdsmith wrote:
> > So this is an example of the issue we were talking about yesterday.  If we
> can't
> > have a simple test as to whether we're at the right place in the state
> diagram,
> > I want to re-visit the question of whether we have the right/enough states. 
> > What I said yesterday is "If we can't just test the target filename,
revisit",
> > but I think I'm going to back off from that and say that this logic is
> > (effectively) on the inside of the download item, so we can rely on inside
> > knowledge of what's going on.  In which case, just testing the FullPath for
> null
> > should work.  Do you agree?
> > 
> > (I'll note that this suggestion makes me a little ootsy.  The question we
> > *really* want to ask here is "Where are we in the main download cascade?"
and
> > we're using the full file path nullity as a proxy for that.  I believe that
> it's
> > true that the full file path will never be non null when we haven't gotten
to
> > this place in the download cascade, but it's not quite as direct a question
> as,
> > well, say, checking the state variable :-J.)
> 
> Can I let you fix this when you put DownloadItem in the driver's seat?

Is there a reason why you can't just check to see if GetFullPath() is non-empty?

(Very welcome to move into DI and make a new state instead.)

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:168:
download_history_.reset(new DownloadHistory(download_manager_, hs));
Why here rather than on DownloadService?  CDMD is intended as an embedder
appendage to the DM, and while it's admittedly grown a bit beyond that, I don't
think we need to add to it.  How ugly would the code be if
CheckVisitedReferrerBeforeDone indirected through the DownloadService?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:642: // If there's
no history but the profile is on-record, or if this profile is
Are there any cases in which we're on-record and have no history?  What are
they?  I had thought that we had no history iff we were OTR; if that's true,
this seems like more complex logic than needed.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:645:
CheckVisitedReferrerBeforeDone(download_id, callback, danger_type, false);
nit, suggestion: I have a slight preference for the "if" branch being the
uncommon case, and the least indented code being the common case, but I'm ok
with either way.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:6: // observing a single
DownloadManager and all its DownloadItems.
Suggestion: Modify now that it's not doing so directly?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:23: // with a weak pointer to
DownloadHistory to prevent use-after-free bugs.
Worth at least a note that this is redundant; I'm even inclined to say that we
shouldn't do it while we're still using CancelableRequestConsumer.   Having
multiple redundant coding paths that are being relied is confusing and can lead
to introduction of bugs that aren't caught.
Having said that, this code is more self-contained with the weak pointer
callbacks, so I'm ok with it.  But I would like a comment.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:199: void
DownloadHistory::MaybeAddToHistory(content::DownloadItem* item) {
Maybe add some blank lines in this function to group related functionality?  I
find this large a block of text without breaks daunting.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:287: (previous_info == NULL) ||
Suggestion: Given that you've abstracted out GetPersistentStoreInfo into a
separate function, maybe this booleans worth while abstracting out and putting
near that function so they get modified together?  I'm tempted to suggest you
put both methods in a separate class to make very clear that they're linked, but
two methods and no state is probably not enough to justify creating a class.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:31: // Since there isn't (and
shouldn't be) a general public way to access
Why shouldn't there be?  It feels very weird for a file to have to include
download_history.h (for the observer definition), but not have any access to the
class.  

If the answer is "because we don't want consumers to have to know whether there
*is* a history or not", I can think of a couple of ways to deal with the issue. 
One is to make DH an ABC and have a null implementation.  Another is to make
DownloadHistoryObserver (no colon) a top level class, and expose it from the
CDMD (though I'll admit that feels a bit weird too).  I find myself leaning
towards the null class idea, but mostly away from the "declare in one class,
have as an argument to a complete different class".

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:47: virtual void
OnDownloadsRemoved(const HandleSet& db_handle) {}
The lack of an equivalent to OnDownloadDestroyed/ManagerGoingDown in this
interface means that all users of it *must* be destroyed before the
DownloadHistory itself.  That seems like an unnecessary restriction?  If you
feel like it keeps the interface cleaner, do document it at the
Add/RemoveObserver declarations--it's the kind of thing that's implicit in the
interface, but that might be missed if someone is in a hurry or not thinking
things through (maybe they missed the Balmer peak? :-})

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: // Caller must guarantee that
HistoryService outlives DownloadHistory.
Suggestion: Add a note explicitly stating that we're independent of the
DownloadManager lifetime?

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.h:37: class Observer {
I'm not sure how I feel about the DownloadItem not being included in this
interface.  If there's any subsystem that wants to track some aspect of history
storage in a SupportsUserData::Data, it'd need that.  Why not include it?

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.h:43: // thread.
Does it include the DB handle?  The comment implies that it's sent before the
PostTask is done (which would mean that the answer to my question is "no"); is
that true?  If so, why?  ('Cause it'd be nice to have the DB handled)  If not,
it should be clearer.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:527: }
Any tests to test multiple downloads in flight at the same time?  Probing maybe
specifically at the things where we batch updates?

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_status_updater_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_status_updater_unittest.cc:23: using
testing::_;
Why in this CL?  I don't majorly object, just seems pretty random.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:859: // The
DownloadItem is is added to history when the path is determined. If
Is this comment still accurate?

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:447: NotifyModelChanged();
Why?  What DMI consumer needs this?

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:538:
OnDownloadTargetDetermined(
Did you explain why this call was added?  I didn't see it if so.

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
content/public/test/download_test_observer.cc:282:
!download->GetTargetFilePath().empty();
It's in test code, so I don't have strong feelings, but be aware that this may
cause unexpected flakiness because of change in behavior (the difference between
finishing before we execute the intermediate rename or after).

[asanka, 2012-09-24 18:39:43]

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.h:61: void
AddHistoryObserver(DownloadHistory::Observer* observer);
Nit: Document these?

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.cc:323: if (item->GetState() ==
content::DownloadItem::IN_PROGRESS) {
I think this condition will always be false.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
File chrome/browser/history/download_persistent_store_info.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
chrome/browser/history/download_persistent_store_info.h:31: // The final path
where the download is saved.
This comment isn't accurate. This could be the intermediate path.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
chrome/browser/history/download_persistent_store_info.h:38: // The URL that
referred us.
Nit: Note that it isn't changed?

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:932:
download->UpdateObservers();
Hasn't UpdateObservers() already been called at this point?

[benjhayden, 2012-09-24 20:12:11]

Feel free to either PTAL or wait for me to fix some broken tests.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.h:61: void
AddHistoryObserver(DownloadHistory::Observer* observer);
On 2012/09/24 18:39:43, asanka wrote:
> Nit: Document these?

Done.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.cc:323: if (item->GetState() ==
content::DownloadItem::IN_PROGRESS) {
On 2012/09/24 18:39:43, asanka wrote:
> I think this condition will always be false.

Done.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.h:37: class Observer {
On 2012/09/24 18:03:25, rdsmith wrote:
> I'm not sure how I feel about the DownloadItem not being included in this
> interface.  If there's any subsystem that wants to track some aspect of
history
> storage in a SupportsUserData::Data, it'd need that.  Why not include it?

Done.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history.h:43: // thread.
On 2012/09/24 18:03:25, rdsmith wrote:
> Does it include the DB handle?  The comment implies that it's sent before the
> PostTask is done (which would mean that the answer to my question is "no"); is
> that true?  If so, why?  ('Cause it'd be nice to have the DB handled)  If not,
> it should be clearer.

Done.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_history_unittest.cc:527: }
On 2012/09/24 18:03:25, rdsmith wrote:
> Any tests to test multiple downloads in flight at the same time?  Probing
maybe
> specifically at the things where we batch updates?

Made a note to do that before committing.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
File chrome/browser/download/download_status_updater_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/do...
chrome/browser/download/download_status_updater_unittest.cc:23: using
testing::_;
On 2012/09/24 18:03:25, rdsmith wrote:
> Why in this CL?  I don't majorly object, just seems pretty random.

I was working with gmock in DownloadHistoryTest and noticed that these colons
aren't in [m]any other test files.
And they're ugly.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:859: // The
DownloadItem is is added to history when the path is determined. If
On 2012/09/24 18:03:25, rdsmith wrote:
> Is this comment still accurate?

Nope. Deleted.
chrome.downloads.onCreated is fired in OnDownloadCreated, before the filename is
determined.
chrome.downloads.onChanged is fired when the filename is determined.
Extensions can wait for onChanged with a non-empty filename before calling
getFileIcon().
That should probably be documented somewhere. OTOH, it's a transient non-fatal
error that good hackers might be able to figure out on their own. If they can't,
then they'll probably file a bug, at which point I'll look into documenting it
somewhere.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
File chrome/browser/history/download_persistent_store_info.h (right):

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
chrome/browser/history/download_persistent_store_info.h:31: // The final path
where the download is saved.
On 2012/09/24 18:39:43, asanka wrote:
> This comment isn't accurate. This could be the intermediate path.

Done.

http://codereview.chromium.org/10915180/diff/28007/chrome/browser/history/dow...
chrome/browser/history/download_persistent_store_info.h:38: // The URL that
referred us.
On 2012/09/24 18:39:43, asanka wrote:
> Nit: Note that it isn't changed?

Done.

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:447: NotifyModelChanged();
On 2012/09/24 18:03:25, rdsmith wrote:
> Why?  What DMI consumer needs this?

I moved this from AddDownloadItemToHistory().
Don't worry about it, I plan on removing ModelChanged RSN.

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:538:
OnDownloadTargetDetermined(
On 2012/09/24 18:03:25, rdsmith wrote:
> Did you explain why this call was added?  I didn't see it if so.

Sorry, to recap our offline chat, some consumers are now seeing SavePackage
items who weren't before, and they need the target path set as well as the
current path. I'll try to roll this into the DownloadItem c-tor that is called
in the SavePackage case.

http://codereview.chromium.org/10915180/diff/28007/content/browser/download/d...
content/browser/download/download_manager_impl.cc:932:
download->UpdateObservers();
On 2012/09/24 18:39:43, asanka wrote:
> Hasn't UpdateObservers() already been called at this point?

Done.

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
content/public/test/download_test_observer.cc:282:
!download->GetTargetFilePath().empty();
On 2012/09/24 18:03:25, rdsmith wrote:
> It's in test code, so I don't have strong feelings, but be aware that this may
> cause unexpected flakiness because of change in behavior (the difference
between
> finishing before we execute the intermediate rename or after).

Could this CL (DownloadHistory-is-a-Observer) wait for another CL to rewrite all
these things to use DUO?

[Randy Smith (Not in Mondays), 2012-09-25 18:09:20]

SavePackage interaction review.

I'd like it if you could do testing (manual, or ideally, automatic) that tested
the race-that-was in the save package code (persistence finishing before
SavePage vs. the reverse, SavePage itself being cancelled before persistence
finishes, and the download item being cancelled before SavePage finishes). 
There's no reason why this new code shouldn't "just work", but it would be good
to make sure it does; those races at least used to be tricky.

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:530:
download_item->AddObserver(observer);
You got rid of blank lines that I personally feel like improve readability, but
allowing people to evaluate a chunk of related lines at the same time. 
Re-insert?

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:534: new
SavePageData(download_item);
I don't see this being used anywhere in this file anymore.  Delete?

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:538:
OnDownloadTargetDetermined(
So.  This starts a long cascade inside of DownloadItemImpl
(OnTargetDownloadDetermined-> OnDownloadRenamedToIntermediateName ->
DMI::OnDownloadRenamedToIntermediateName -> MaybeCompleteDownload()).  Maybe it
works without you tweaking it (if so, probably because MaybeCompleteDownload is
missing some attribute needed for continuation), or maybe you can get it to
work.  But I'd rather we not engage that cascade at all--it's oriented at
regular downloads, not save page downloads.

I'm not sure how to avoid it, but the way I'd be tempted to try is to modify the
SavePackage DownloadItem constructor so that it naturally sets up the variables
you want setup from the beginning, and don't go anywhere near the Dangerous
Machinery of the regular download item cascade.

[benjhayden, 2012-09-25 20:22:18]

Still working on tests.

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:530:
download_item->AddObserver(observer);
On 2012/09/25 18:09:20, rdsmith wrote:
> You got rid of blank lines that I personally feel like improve readability,
but
> allowing people to evaluate a chunk of related lines at the same time. 
> Re-insert?

Done.

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:534: new
SavePageData(download_item);
On 2012/09/25 18:09:20, rdsmith wrote:
> I don't see this being used anywhere in this file anymore.  Delete?

Done.

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:538:
OnDownloadTargetDetermined(
On 2012/09/25 18:09:20, rdsmith wrote:
> So.  This starts a long cascade inside of DownloadItemImpl
> (OnTargetDownloadDetermined-> OnDownloadRenamedToIntermediateName ->
> DMI::OnDownloadRenamedToIntermediateName -> MaybeCompleteDownload()).  Maybe
it
> works without you tweaking it (if so, probably because MaybeCompleteDownload
is
> missing some attribute needed for continuation), or maybe you can get it to
> work.  But I'd rather we not engage that cascade at all--it's oriented at
> regular downloads, not save page downloads.
> 
> I'm not sure how to avoid it, but the way I'd be tempted to try is to modify
the
> SavePackage DownloadItem constructor so that it naturally sets up the
variables
> you want setup from the beginning, and don't go anywhere near the Dangerous
> Machinery of the regular download item cascade.

Done.

[Randy Smith (Not in Mondays), 2012-09-26 15:56:12]

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
content/browser/download/download_manager_impl.cc:538:
OnDownloadTargetDetermined(
On 2012/09/25 20:22:18, benjhayden_chromium wrote:
> On 2012/09/25 18:09:20, rdsmith wrote:
> > So.  This starts a long cascade inside of DownloadItemImpl
> > (OnTargetDownloadDetermined-> OnDownloadRenamedToIntermediateName ->
> > DMI::OnDownloadRenamedToIntermediateName -> MaybeCompleteDownload()).  Maybe
> it
> > works without you tweaking it (if so, probably because MaybeCompleteDownload
> is
> > missing some attribute needed for continuation), or maybe you can get it to
> > work.  But I'd rather we not engage that cascade at all--it's oriented at
> > regular downloads, not save page downloads.
> > 
> > I'm not sure how to avoid it, but the way I'd be tempted to try is to modify
> the
> > SavePackage DownloadItem constructor so that it naturally sets up the
> variables
> > you want setup from the beginning, and don't go anywhere near the Dangerous
> > Machinery of the regular download item cascade.
> 
> Done.

I don't see any modifications to the SavePackage download item constructor. 
What did you do in place of this?  Or did you decide that it wasn't needed after
all?

[benjhayden, 2012-09-28 13:51:13]

On 2012/09/26 15:56:12, rdsmith wrote:
>
http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10915180/diff/37062/content/browser/download/d...
> content/browser/download/download_manager_impl.cc:538:
> OnDownloadTargetDetermined(
> On 2012/09/25 20:22:18, benjhayden_chromium wrote:
> > On 2012/09/25 18:09:20, rdsmith wrote:
> > > So.  This starts a long cascade inside of DownloadItemImpl
> > > (OnTargetDownloadDetermined-> OnDownloadRenamedToIntermediateName ->
> > > DMI::OnDownloadRenamedToIntermediateName -> MaybeCompleteDownload()). 
Maybe
> > it
> > > works without you tweaking it (if so, probably because
MaybeCompleteDownload
> > is
> > > missing some attribute needed for continuation), or maybe you can get it
to
> > > work.  But I'd rather we not engage that cascade at all--it's oriented at
> > > regular downloads, not save page downloads.
> > > 
> > > I'm not sure how to avoid it, but the way I'd be tempted to try is to
modify
> > the
> > > SavePackage DownloadItem constructor so that it naturally sets up the
> > variables
> > > you want setup from the beginning, and don't go anywhere near the
Dangerous
> > > Machinery of the regular download item cascade.
> > 
> > Done.
> 
> I don't see any modifications to the SavePackage download item constructor. 
> What did you do in place of this?  Or did you decide that it wasn't needed
after
> all?

Sorry, still looking into this.
I had misremembered. The DII ctor for save page already set the target path
correctly. It appears that save_page_browsertest expects DMI to call
ShowDownloadInBrowser for save page, which is one thing that
OnDownloadTargetDetermined did. Still trying to figure out if there's anything
else that needs to happen.

[benjhayden, 2012-09-28 17:00:06]

ShowDownloadInBrowser appears to have been the only thing that
CreateSavePackageItem needed to do.
I think I've ironed out DownloadPersistedObserver in save_page_browsertest.

PTAL and cross your fingers for the trybots.

[asanka, 2012-09-28 19:02:30]

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent(
EndEvent()

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
content/browser/download/download_item_impl.cc:1030: UpdateObservers();
Given that with this change we always call UpdateObservers() after
SetFullPath(), we might as well fold the call into SetFullPath().

[benjhayden, 2012-10-01 14:03:36]

sky, csilv, creis, jochen: PTAL
Still working some kinks out of the changes to */download/*, but your files
should be stable.
Thanks!

[benjhayden, 2012-10-01 14:10:03]

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent(
On 2012/09/28 19:02:30, asanka wrote:
> EndEvent()

Can you elaborate why?
This is Init(), which is only called when downloads are created, so there was no
corresponding BeginEvent. I'd expect it to be incorrect to call EndEvent with no
corresponding BeginEvent.

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
content/browser/download/download_item_impl.cc:1030: UpdateObservers();
On 2012/09/28 19:02:30, asanka wrote:
> Given that with this change we always call UpdateObservers() after
> SetFullPath(), we might as well fold the call into SetFullPath().

Done.

[asanka, 2012-10-01 15:59:36]

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10915180/diff/55002/content/browser/download/d...
content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent(
On 2012/10/01 14:10:03, benjhayden_chromium wrote:
> On 2012/09/28 19:02:30, asanka wrote:
> > EndEvent()
> 
> Can you elaborate why?
> This is Init(), which is only called when downloads are created, so there was
no
> corresponding BeginEvent. I'd expect it to be incorrect to call EndEvent with
no
> corresponding BeginEvent.

Ignore my comment. I misread.

[Randy Smith (Not in Mondays), 2012-10-01 17:19:29]

The save package interface looks good.  I'm holding off on doing other review
until you work out the kinks in download/*; let me know if that's wrong.

[csilv, 2012-10-01 17:29:52]

/webui/ lgtm

[Charlie Reis, 2012-10-01 21:01:01]

content/content_browser.gypi LGTM.

nit: Can you add a BUG= line to the CL description?  There's a lot changing
here, and it'd be good to link it to a bug.

[jochen (gone - plz use gerrit), 2012-10-09 20:58:19]

content/shell lgtm

[csilv, 2012-10-09 21:19:00]

ui/webui/downloads_dom_handler_browsertest.cc  lgtm

[benjhayden, 2012-10-30 20:25:05]

If you've already LG'd, then you do not need to look again.

Randy, Asanka, PTAL.

[Randy Smith (Not in Mondays), 2012-10-30 22:26:21]

On 2012/10/30 20:25:05, benjhayden_chromium wrote:
> If you've already LG'd, then you do not need to look again.
> 
> Randy, Asanka, PTAL.

There are a lot of comments from me (specifically in c#7) that I don't see a
response to, and that makes me a bit worried.  I can go looking for each one,
but let me just ask: Have you incorporated those comments?

[benjhayden, 2012-11-02 17:21:36]

PTAL while I look at the tests.

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
On 2012/09/24 18:03:25, rdsmith wrote:
> On 2012/09/21 20:45:46, benjhayden_chromium wrote:
> > On 2012/09/13 19:53:19, rdsmith wrote:
> > > So this is an example of the issue we were talking about yesterday.  If we
> > can't
> > > have a simple test as to whether we're at the right place in the state
> > diagram,
> > > I want to re-visit the question of whether we have the right/enough
states. 
> > > What I said yesterday is "If we can't just test the target filename,
> revisit",
> > > but I think I'm going to back off from that and say that this logic is
> > > (effectively) on the inside of the download item, so we can rely on inside
> > > knowledge of what's going on.  In which case, just testing the FullPath
for
> > null
> > > should work.  Do you agree?
> > > 
> > > (I'll note that this suggestion makes me a little ootsy.  The question we
> > > *really* want to ask here is "Where are we in the main download cascade?"
> and
> > > we're using the full file path nullity as a proxy for that.  I believe
that
> > it's
> > > true that the full file path will never be non null when we haven't gotten
> to
> > > this place in the download cascade, but it's not quite as direct a
question
> > as,
> > > well, say, checking the state variable :-J.)
> > 
> > Can I let you fix this when you put DownloadItem in the driver's seat?
> 
> Is there a reason why you can't just check to see if GetFullPath() is
non-empty?
> 
> (Very welcome to move into DI and make a new state instead.)

This method moved into DII. I added comments there about why these are the
necessary conditions, but for convenience,
(!GetTargetFilePath().empty()) is checked in ReadyForDownloadCompletionDone()
(target_path_.DirName() == current_path_.DirName()) is checked in NeedsRename(),
called from ReadyForDownloadCompletionDone.

I would not object to replacing these conditions with a new state, but that
seems to me like a job for another CL. Push back if you feel strongly.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:168:
download_history_.reset(new DownloadHistory(download_manager_, hs));
On 2012/09/24 18:03:25, rdsmith wrote:
> Why here rather than on DownloadService?  CDMD is intended as an embedder
> appendage to the DM, and while it's admittedly grown a bit beyond that, I
don't
> think we need to add to it.  How ugly would the code be if
> CheckVisitedReferrerBeforeDone indirected through the DownloadService?

Hm. How would you feel about making CDMD::CheckVisitedReferrerBefore access the
HistoryService directly instead of bouncing through DownloadHistory? CVRB
doesn't really belong with everything else that DH does.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:642: // If there's
no history but the profile is on-record, or if this profile is
On 2012/09/24 18:03:25, rdsmith wrote:
> Are there any cases in which we're on-record and have no history?  What are
> they?  I had thought that we had no history iff we were OTR; if that's true,
> this seems like more complex logic than needed.

My understanding is that there is some rare case where we may be on-record
without a history. Maybe it's only in some tests, maybe there's a race window, I
forget exactly.
Either way, a couple branches and a fall-back case doesn't seem terribly
complicated to me. 
I added a DCHECK, so we'll see if it explodes on trybots.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:645:
CheckVisitedReferrerBeforeDone(download_id, callback, danger_type, false);
On 2012/09/24 18:03:25, rdsmith wrote:
> nit, suggestion: I have a slight preference for the "if" branch being the
> uncommon case, and the least indented code being the common case, but I'm ok
> with either way.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:6: // observing a single
DownloadManager and all its DownloadItems.
On 2012/09/24 18:03:25, rdsmith wrote:
> Suggestion: Modify now that it's not doing so directly?

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:23: // with a weak pointer to
DownloadHistory to prevent use-after-free bugs.
On 2012/09/24 18:03:25, rdsmith wrote:
> Worth at least a note that this is redundant; I'm even inclined to say that we
> shouldn't do it while we're still using CancelableRequestConsumer.   Having
> multiple redundant coding paths that are being relied is confusing and can
lead
> to introduction of bugs that aren't caught.
> Having said that, this code is more self-contained with the weak pointer
> callbacks, so I'm ok with it.  But I would like a comment.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:199: void
DownloadHistory::MaybeAddToHistory(content::DownloadItem* item) {
On 2012/09/24 18:03:25, rdsmith wrote:
> Maybe add some blank lines in this function to group related functionality?  I
> find this large a block of text without breaks daunting.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:287: (previous_info == NULL) ||
On 2012/09/24 18:03:25, rdsmith wrote:
> Suggestion: Given that you've abstracted out GetPersistentStoreInfo into a
> separate function, maybe this booleans worth while abstracting out and putting
> near that function so they get modified together?  I'm tempted to suggest you
> put both methods in a separate class to make very clear that they're linked,
but
> two methods and no state is probably not enough to justify creating a class.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:31: // Since there isn't (and
shouldn't be) a general public way to access
On 2012/09/24 18:03:25, rdsmith wrote:
> Why shouldn't there be?  It feels very weird for a file to have to include
> download_history.h (for the observer definition), but not have any access to
the
> class.  
> 
> If the answer is "because we don't want consumers to have to know whether
there
> *is* a history or not", I can think of a couple of ways to deal with the
issue. 
> One is to make DH an ABC and have a null implementation.  Another is to make
> DownloadHistoryObserver (no colon) a top level class, and expose it from the
> CDMD (though I'll admit that feels a bit weird too).  I find myself leaning
> towards the null class idea, but mostly away from the "declare in one class,
> have as an argument to a complete different class".

Ok, yeah, I'll expose DownloadHistory so that we don't need to bounce
Add/RemoveHistoryObserver.
Before I do that, how do you feel about the suggestion that I made in another
comment about putting DownloadHistory in DownloadService instead of CDMD, and
making CDMD implement CheckVisitedReferrerBefore directly on top of
HistoryService?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:47: virtual void
OnDownloadsRemoved(const HandleSet& db_handle) {}
On 2012/09/24 18:03:25, rdsmith wrote:
> The lack of an equivalent to OnDownloadDestroyed/ManagerGoingDown in this
> interface means that all users of it *must* be destroyed before the
> DownloadHistory itself.  That seems like an unnecessary restriction?  If you
> feel like it keeps the interface cleaner, do document it at the
> Add/RemoveObserver declarations--it's the kind of thing that's implicit in the
> interface, but that might be missed if someone is in a hurry or not thinking
> things through (maybe they missed the Balmer peak? :-})

I'm not sure who you're referring to by 'users'.
ADIN::Observers deal with ManagerGoingDown by checking ADIN::GetManager().
I'm not sure why a DH::O would want to match up OnDownloadsRemoved() calls with
DownloadItems. Wouldn't they just observe the item? Maybe
DH::O::OnDownloadsRemoved isn't all that useful...

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: // Caller must guarantee that
HistoryService outlives DownloadHistory.
On 2012/09/24 18:03:25, rdsmith wrote:
> Suggestion: Add a note explicitly stating that we're independent of the
> DownloadManager lifetime?

What do you mean by 'independent'?
ADIN::Os deal with ManagerGoingDown by checking ADIN::GetManager.

[asanka, 2012-11-02 19:56:57]

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history.cc:269: void
DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {
If CreateDownload() failed, then db_handle == 0. Does that case need special
handling?

[Randy Smith (Not in Mondays), 2012-11-02 23:31:24]

I didn't see an answer to my earlier question as to whether you were satisfied
that you hadn't lost any test coverage in the transformation of this CL?

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
On 2012/11/02 17:21:36, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > On 2012/09/21 20:45:46, benjhayden_chromium wrote:
> > > On 2012/09/13 19:53:19, rdsmith wrote:
> > > > So this is an example of the issue we were talking about yesterday.  If
we
> > > can't
> > > > have a simple test as to whether we're at the right place in the state
> > > diagram,
> > > > I want to re-visit the question of whether we have the right/enough
> states. 
> > > > What I said yesterday is "If we can't just test the target filename,
> > revisit",
> > > > but I think I'm going to back off from that and say that this logic is
> > > > (effectively) on the inside of the download item, so we can rely on
inside
> > > > knowledge of what's going on.  In which case, just testing the FullPath
> for
> > > null
> > > > should work.  Do you agree?
> > > > 
> > > > (I'll note that this suggestion makes me a little ootsy.  The question
we
> > > > *really* want to ask here is "Where are we in the main download
cascade?"
> > and
> > > > we're using the full file path nullity as a proxy for that.  I believe
> that
> > > it's
> > > > true that the full file path will never be non null when we haven't
gotten
> > to
> > > > this place in the download cascade, but it's not quite as direct a
> question
> > > as,
> > > > well, say, checking the state variable :-J.)
> > > 
> > > Can I let you fix this when you put DownloadItem in the driver's seat?
> > 
> > Is there a reason why you can't just check to see if GetFullPath() is
> non-empty?
> > 
> > (Very welcome to move into DI and make a new state instead.)
> 
> This method moved into DII. I added comments there about why these are the
> necessary conditions, but for convenience,
> (!GetTargetFilePath().empty()) is checked in ReadyForDownloadCompletionDone()
> (target_path_.DirName() == current_path_.DirName()) is checked in
NeedsRename(),
> called from ReadyForDownloadCompletionDone.
> 
> I would not object to replacing these conditions with a new state, but that
> seems to me like a job for another CL. Push back if you feel strongly.

I'm ok with putting in a new state in another CL, but I'm still confused about
why we need the check for target_path_.DirName() == current_path_.DirName().  I
believe that if current_path_ is non-NULL, it will have the same directory as
the target path, so we can just make that a DCHECK.  

To my mind, the question we're really asking here is "where are we in the
DownloadItem cascade?".  The "right" way to do that is via a state--ok, not in
this CL.  But having two tests implies that the two conditions race with each
other, as opposed to one always and forever occurring after the other and us
testing on the second one.  Do you know of a reason why we can't replace those
two tests with !target_path.empty()?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:168:
download_history_.reset(new DownloadHistory(download_manager_, hs));
On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > Why here rather than on DownloadService?  CDMD is intended as an embedder
> > appendage to the DM, and while it's admittedly grown a bit beyond that, I
> don't
> > think we need to add to it.  How ugly would the code be if
> > CheckVisitedReferrerBeforeDone indirected through the DownloadService?
> 
> Hm. How would you feel about making CDMD::CheckVisitedReferrerBefore access
the
> HistoryService directly instead of bouncing through DownloadHistory? CVRB
> doesn't really belong with everything else that DH does.

I'd be good with that.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:642: // If there's
no history but the profile is on-record, or if this profile is
On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > Are there any cases in which we're on-record and have no history?  What are
> > they?  I had thought that we had no history iff we were OTR; if that's true,
> > this seems like more complex logic than needed.
> 
> My understanding is that there is some rare case where we may be on-record
> without a history. Maybe it's only in some tests, maybe there's a race window,
I
> forget exactly.
> Either way, a couple branches and a fall-back case doesn't seem terribly
> complicated to me. 
> I added a DCHECK, so we'll see if it explodes on trybots.

Cool, thanks. 

WRT the couple of branches & fall-back case: It's the death by a thousand cuts. 
Code that isn't used, rots, and takes up memory size, and makes it harder for
other people to understand the working code than they need to.  If we can get
rid of this & replace it with a DCHECK, I'd like to.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:31: // Since there isn't (and
shouldn't be) a general public way to access
On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > Why shouldn't there be?  It feels very weird for a file to have to include
> > download_history.h (for the observer definition), but not have any access to
> the
> > class.  
> > 
> > If the answer is "because we don't want consumers to have to know whether
> there
> > *is* a history or not", I can think of a couple of ways to deal with the
> issue. 
> > One is to make DH an ABC and have a null implementation.  Another is to make
> > DownloadHistoryObserver (no colon) a top level class, and expose it from the
> > CDMD (though I'll admit that feels a bit weird too).  I find myself leaning
> > towards the null class idea, but mostly away from the "declare in one class,
> > have as an argument to a complete different class".
> 
> Ok, yeah, I'll expose DownloadHistory so that we don't need to bounce
> Add/RemoveHistoryObserver.
> Before I do that, how do you feel about the suggestion that I made in another
> comment about putting DownloadHistory in DownloadService instead of CDMD, and
> making CDMD implement CheckVisitedReferrerBefore directly on top of
> HistoryService?

I think it's a good idea.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:47: virtual void
OnDownloadsRemoved(const HandleSet& db_handle) {}
On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > The lack of an equivalent to OnDownloadDestroyed/ManagerGoingDown in this
> > interface means that all users of it *must* be destroyed before the
> > DownloadHistory itself.  That seems like an unnecessary restriction?  If you
> > feel like it keeps the interface cleaner, do document it at the
> > Add/RemoveObserver declarations--it's the kind of thing that's implicit in
the
> > interface, but that might be missed if someone is in a hurry or not thinking
> > things through (maybe they missed the Balmer peak? :-})
> 
> I'm not sure who you're referring to by 'users'.
> ADIN::Observers deal with ManagerGoingDown by checking ADIN::GetManager().
> I'm not sure why a DH::O would want to match up OnDownloadsRemoved() calls
with
> DownloadItems. Wouldn't they just observe the item? Maybe
> DH::O::OnDownloadsRemoved isn't all that useful...

I'm confused about how you're saying relates to my comment--possibly we should
talk about this in person.  In parallel to the DM::O and DI::O interfaces (but
not ADIN::O), all users of this interface must call DH::RemoveObserver() before
they self-destruct.  That means they need to keep a pointer to DH around.  But
they don't want to keep a pointer to DH around if DH has been destroyed.  If
they don't get a warning when DH dies, they need to always die before DH does.  
So I'm suggesting that you either add a HistoryGoingDown() method (preferred) or
document that requirement at the Add/RemoveObserver interface (disliked, but you
could talk me into it).  

Does that make more sense than my first note?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: // Caller must guarantee that
HistoryService outlives DownloadHistory.
On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > Suggestion: Add a note explicitly stating that we're independent of the
> > DownloadManager lifetime?
> 
> What do you mean by 'independent'?
> ADIN::Os deal with ManagerGoingDown by checking ADIN::GetManager.

Yep, that's how the implementation deals, but users of DownloadHistory shouldn't
have to know about the implementation, that's sorta the point.  I was suggesting
(your call; it's sorta implied) that while the caller must guarantee that
HistoryService outlives DownloadHistory, the caller doesn't have to worry about
the DownloadManager lifetime in relationship to DownloadHistory.  I think of it
as an obvious question for consumers, since DownloadHistory is acting as a
bridge between HistoryService and DownloadManager.

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
content/public/test/download_test_observer.cc:282:
!download->GetTargetFilePath().empty();
On 2012/09/24 20:12:11, benjhayden_chromium wrote:
> On 2012/09/24 18:03:25, rdsmith wrote:
> > It's in test code, so I don't have strong feelings, but be aware that this
may
> > cause unexpected flakiness because of change in behavior (the difference
> between
> > finishing before we execute the intermediate rename or after).
> 
> Could this CL (DownloadHistory-is-a-Observer) wait for another CL to rewrite
all
> these things to use DUO?

I suppose, if you're volunteering.  But how would that help?  DUO still needs to
do some test on the DownloadItem, and it's the test that I'm calling out to you
as potentially being problematic.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:63: void
RemoveHistoryObserver(DownloadHistory::Observer* observer);
These appear to only be used by tests.  Any reason not to give tests some way to
get at the DownloadHistory object instead?  Or at least make these "ForTesting"?

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history.h:35: //    
RemoveHistoryObserver(this);
I thought you were changing this?

I remain of the opinion that it's better to give observers a direct way to get
at DownloadHistory than to indirect through CDMD this way.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:536: // batched removes.
I'd like this in before landing.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:295: url, full_file_name, 1,
DownloadItem::COMPLETE));
The fact that this is needed, rather than folding this logic into
WaitForSavePackageToFinish, seems like a (minor) flaw in the DownloadHistory
interface.  Is there really no way to probe to see if a particular DownloadItem
has been persisted?  (Worst comes to worst, you observe the history in an object
setup in the SetUp() method, and match up by ids?)

Not a big deal--if you don't want to deal, don't.  But reading this code it
seems like there should be a better way.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:482: // TODO(benjhayden) Should
not be in history.
I'm confused--does this mean that it *is* in the history?  I would think that
this would be part of this CL.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:555: url, full_file_name, -1,
DownloadItem::COMPLETE));
Why the change?

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
chrome/browser/history/download_database.cc:194: "SET full_path=?,
received_bytes=?, state=?, end_time=?, total_bytes=?, "
Why change the persistence of full_path in the same CL as moving the history to
an observer?  They seem like they could (and sorta should) be separate.  (I
don't have strong feelings.)

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
chrome/browser/history/history.h:455: virtual Handle CreateDownload(
Probably worthwhile putting a note in here somewhere about why methods are
virtual.  It's also a little strange that some are and some aren't; you might
want to switch them all over.

[benjhayden, 2012-11-06 20:01:13]

Yes, I just went through all the tests changed in this CL and convinced myself
that the changes do not decrease coverage.

Still working on the TODO at the bottom of download_history_unittest.cc.
Other than that, PTAL unless you've already LG'd.

Thanks!

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10915180/diff/15001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:619: // If the target filename
hasn't been determined, don't complete the download.
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:36, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > On 2012/09/21 20:45:46, benjhayden_chromium wrote:
> > > > On 2012/09/13 19:53:19, rdsmith wrote:
> > > > > So this is an example of the issue we were talking about yesterday. 
If
> we
> > > > can't
> > > > > have a simple test as to whether we're at the right place in the state
> > > > diagram,
> > > > > I want to re-visit the question of whether we have the right/enough
> > states. 
> > > > > What I said yesterday is "If we can't just test the target filename,
> > > revisit",
> > > > > but I think I'm going to back off from that and say that this logic is
> > > > > (effectively) on the inside of the download item, so we can rely on
> inside
> > > > > knowledge of what's going on.  In which case, just testing the
FullPath
> > for
> > > > null
> > > > > should work.  Do you agree?
> > > > > 
> > > > > (I'll note that this suggestion makes me a little ootsy.  The question
> we
> > > > > *really* want to ask here is "Where are we in the main download
> cascade?"
> > > and
> > > > > we're using the full file path nullity as a proxy for that.  I believe
> > that
> > > > it's
> > > > > true that the full file path will never be non null when we haven't
> gotten
> > > to
> > > > > this place in the download cascade, but it's not quite as direct a
> > question
> > > > as,
> > > > > well, say, checking the state variable :-J.)
> > > > 
> > > > Can I let you fix this when you put DownloadItem in the driver's seat?
> > > 
> > > Is there a reason why you can't just check to see if GetFullPath() is
> > non-empty?
> > > 
> > > (Very welcome to move into DI and make a new state instead.)
> > 
> > This method moved into DII. I added comments there about why these are the
> > necessary conditions, but for convenience,
> > (!GetTargetFilePath().empty()) is checked in
ReadyForDownloadCompletionDone()
> > (target_path_.DirName() == current_path_.DirName()) is checked in
> NeedsRename(),
> > called from ReadyForDownloadCompletionDone.
> > 
> > I would not object to replacing these conditions with a new state, but that
> > seems to me like a job for another CL. Push back if you feel strongly.
> 
> I'm ok with putting in a new state in another CL, but I'm still confused about
> why we need the check for target_path_.DirName() == current_path_.DirName(). 
I
> believe that if current_path_ is non-NULL, it will have the same directory as
> the target path, so we can just make that a DCHECK.  
> 
> To my mind, the question we're really asking here is "where are we in the
> DownloadItem cascade?".  The "right" way to do that is via a state--ok, not in
> this CL.  But having two tests implies that the two conditions race with each
> other, as opposed to one always and forever occurring after the other and us
> testing on the second one.  Do you know of a reason why we can't replace those
> two tests with !target_path.empty()?

I seem to remember that there is a rare case where this can happen, but I don't
remember exactly where. I took out this condition and no tests failed, so I'll
leave it out until we find a case where it might happen.

Asanka, do you happen to know when !target_path_.empty() &&
target_path.DirName() != current_path_.DirName() if ever?

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:168:
download_history_.reset(new DownloadHistory(download_manager_, hs));
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > Why here rather than on DownloadService?  CDMD is intended as an embedder
> > > appendage to the DM, and while it's admittedly grown a bit beyond that, I
> > don't
> > > think we need to add to it.  How ugly would the code be if
> > > CheckVisitedReferrerBeforeDone indirected through the DownloadService?
> > 
> > Hm. How would you feel about making CDMD::CheckVisitedReferrerBefore access
> the
> > HistoryService directly instead of bouncing through DownloadHistory? CVRB
> > doesn't really belong with everything else that DH does.
> 
> I'd be good with that.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:642: // If there's
no history but the profile is on-record, or if this profile is
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > Are there any cases in which we're on-record and have no history?  What
are
> > > they?  I had thought that we had no history iff we were OTR; if that's
true,
> > > this seems like more complex logic than needed.
> > 
> > My understanding is that there is some rare case where we may be on-record
> > without a history. Maybe it's only in some tests, maybe there's a race
window,
> I
> > forget exactly.
> > Either way, a couple branches and a fall-back case doesn't seem terribly
> > complicated to me. 
> > I added a DCHECK, so we'll see if it explodes on trybots.
> 
> Cool, thanks. 
> 
> WRT the couple of branches & fall-back case: It's the death by a thousand
cuts. 
> Code that isn't used, rots, and takes up memory size, and makes it harder for
> other people to understand the working code than they need to.  If we can get
> rid of this & replace it with a DCHECK, I'd like to.  

The DCHECK exploded in unit_tests:
ChromeDownloadManagerDelegateTest.StartDownload_Basic.
Added a comment that unit_tests do not have a HistoryService.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:31: // Since there isn't (and
shouldn't be) a general public way to access
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > Why shouldn't there be?  It feels very weird for a file to have to include
> > > download_history.h (for the observer definition), but not have any access
to
> > the
> > > class.  
> > > 
> > > If the answer is "because we don't want consumers to have to know whether
> > there
> > > *is* a history or not", I can think of a couple of ways to deal with the
> > issue. 
> > > One is to make DH an ABC and have a null implementation.  Another is to
make
> > > DownloadHistoryObserver (no colon) a top level class, and expose it from
the
> > > CDMD (though I'll admit that feels a bit weird too).  I find myself
leaning
> > > towards the null class idea, but mostly away from the "declare in one
class,
> > > have as an argument to a complete different class".
> > 
> > Ok, yeah, I'll expose DownloadHistory so that we don't need to bounce
> > Add/RemoveHistoryObserver.
> > Before I do that, how do you feel about the suggestion that I made in
another
> > comment about putting DownloadHistory in DownloadService instead of CDMD,
and
> > making CDMD implement CheckVisitedReferrerBefore directly on top of
> > HistoryService?
> 
> I think it's a good idea.

Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:47: virtual void
OnDownloadsRemoved(const HandleSet& db_handle) {}
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > The lack of an equivalent to OnDownloadDestroyed/ManagerGoingDown in this
> > > interface means that all users of it *must* be destroyed before the
> > > DownloadHistory itself.  That seems like an unnecessary restriction?  If
you
> > > feel like it keeps the interface cleaner, do document it at the
> > > Add/RemoveObserver declarations--it's the kind of thing that's implicit in
> the
> > > interface, but that might be missed if someone is in a hurry or not
thinking
> > > things through (maybe they missed the Balmer peak? :-})
> > 
> > I'm not sure who you're referring to by 'users'.
> > ADIN::Observers deal with ManagerGoingDown by checking ADIN::GetManager().
> > I'm not sure why a DH::O would want to match up OnDownloadsRemoved() calls
> with
> > DownloadItems. Wouldn't they just observe the item? Maybe
> > DH::O::OnDownloadsRemoved isn't all that useful...
> 
> I'm confused about how you're saying relates to my comment--possibly we should
> talk about this in person.  In parallel to the DM::O and DI::O interfaces (but
> not ADIN::O), all users of this interface must call DH::RemoveObserver()
before
> they self-destruct.  That means they need to keep a pointer to DH around.  But
> they don't want to keep a pointer to DH around if DH has been destroyed.  If
> they don't get a warning when DH dies, they need to always die before DH does.
 
> So I'm suggesting that you either add a HistoryGoingDown() method (preferred)
or
> document that requirement at the Add/RemoveObserver interface (disliked, but
you
> could talk me into it).  
> 
> Does that make more sense than my first note?

Ah! Yes, thanks.
Done.

http://codereview.chromium.org/10915180/diff/29001/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: // Caller must guarantee that
HistoryService outlives DownloadHistory.
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/11/02 17:21:37, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > Suggestion: Add a note explicitly stating that we're independent of the
> > > DownloadManager lifetime?
> > 
> > What do you mean by 'independent'?
> > ADIN::Os deal with ManagerGoingDown by checking ADIN::GetManager.
> 
> Yep, that's how the implementation deals, but users of DownloadHistory
shouldn't
> have to know about the implementation, that's sorta the point.  I was
suggesting
> (your call; it's sorta implied) that while the caller must guarantee that
> HistoryService outlives DownloadHistory, the caller doesn't have to worry
about
> the DownloadManager lifetime in relationship to DownloadHistory.  I think of
it
> as an obvious question for consumers, since DownloadHistory is acting as a
> bridge between HistoryService and DownloadManager.

Done.

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10915180/diff/28007/content/public/test/downlo...
content/public/test/download_test_observer.cc:282:
!download->GetTargetFilePath().empty();
On 2012/11/02 23:31:24, rdsmith wrote:
> On 2012/09/24 20:12:11, benjhayden_chromium wrote:
> > On 2012/09/24 18:03:25, rdsmith wrote:
> > > It's in test code, so I don't have strong feelings, but be aware that this
> may
> > > cause unexpected flakiness because of change in behavior (the difference
> > between
> > > finishing before we execute the intermediate rename or after).
> > 
> > Could this CL (DownloadHistory-is-a-Observer) wait for another CL to rewrite
> all
> > these things to use DUO?
> 
> I suppose, if you're volunteering.  But how would that help?  DUO still needs
to
> do some test on the DownloadItem, and it's the test that I'm calling out to
you
> as potentially being problematic.

Sorry, I don't know what I was thinking.
Yes, this is a slight change in behavior. I'll experiment to see if I still need
this.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:63: void
RemoveHistoryObserver(DownloadHistory::Observer* observer);
On 2012/11/02 23:31:25, rdsmith wrote:
> These appear to only be used by tests.  Any reason not to give tests some way
to
> get at the DownloadHistory object instead?  Or at least make these
"ForTesting"?

Exposed DownloadService::GetDownloadHistory(). LMK if you think I should also
make DownloadHistory::Add/RemoveObserver "ForTesting".

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history.cc:269: void
DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {
On 2012/11/02 19:56:57, asanka wrote:
> If CreateDownload() failed, then db_handle == 0. Does that case need special
> handling?

Yes and no.
I made DownloadDatabase::CreateDownload return kUninitializedHandle so that we
only have one special value.
I made DownloadHistory::ItemAdded unset is_adding regardless of db_handle, but
return early right after that, without automatically retrying. The next
OnDownloadUpdated/OnDownloadOpened, it will re-try adding it to the db.
How does that look?

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history.h:35: //    
RemoveHistoryObserver(this);
On 2012/11/02 23:31:25, rdsmith wrote:
> I thought you were changing this?
> 
> I remain of the opinion that it's better to give observers a direct way to get
> at DownloadHistory than to indirect through CDMD this way.

Done.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:295: url, full_file_name, 1,
DownloadItem::COMPLETE));
On 2012/11/02 23:31:25, rdsmith wrote:
> The fact that this is needed, rather than folding this logic into
> WaitForSavePackageToFinish, seems like a (minor) flaw in the DownloadHistory
> interface.  Is there really no way to probe to see if a particular
DownloadItem
> has been persisted?  (Worst comes to worst, you observe the history in an
object
> setup in the SetUp() method, and match up by ids?)
> 
> Not a big deal--if you don't want to deal, don't.  But reading this code it
> seems like there should be a better way.

Done.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:482: // TODO(benjhayden) Should
not be in history.
On 2012/11/02 23:31:25, rdsmith wrote:
> I'm confused--does this mean that it *is* in the history?  I would think that
> this would be part of this CL.

It was a reminder to test that the item was removed from history, which I've
done.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:555: url, full_file_name, -1,
DownloadItem::COMPLETE));
On 2012/11/02 23:31:25, rdsmith wrote:
> Why the change?  

Done.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
chrome/browser/history/download_database.cc:194: "SET full_path=?,
received_bytes=?, state=?, end_time=?, total_bytes=?, "
On 2012/11/02 23:31:25, rdsmith wrote:
> Why change the persistence of full_path in the same CL as moving the history
to
> an observer?  They seem like they could (and sorta should) be separate.  (I
> don't have strong feelings.)

This isn't actually changing what's persisted. This is just merging
UpdateDownload with UpdateDownloadPath to simplify the API.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
chrome/browser/history/history.h:455: virtual Handle CreateDownload(
On 2012/11/02 23:31:25, rdsmith wrote:
> Probably worthwhile putting a note in here somewhere about why methods are
> virtual.  It's also a little strange that some are and some aren't; you might
> want to switch them all over.

Added a note below the section header on L446.
I'd rather not touch others' APIs, especially in this already gargantuan CL.

[benjhayden, 2012-11-07 16:39:55]

Test done.
PTAL at that if you haven't already LG'd.
Thanks!

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:536: // batched removes.
On 2012/11/02 23:31:25, rdsmith wrote:
> I'd like this in before landing.

Done.

[Randy Smith (Not in Mondays), 2012-11-07 21:10:29]

I haven't reviewed the tests yet, but I'm feeling a bit fried, so I'll toss
these comments at you and come back to the tests later.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:63: void
RemoveHistoryObserver(DownloadHistory::Observer* observer);
On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> On 2012/11/02 23:31:25, rdsmith wrote:
> > These appear to only be used by tests.  Any reason not to give tests some
way
> to
> > get at the DownloadHistory object instead?  Or at least make these
> "ForTesting"?
> 
> Exposed DownloadService::GetDownloadHistory(). LMK if you think I should also
> make DownloadHistory::Add/RemoveObserver "ForTesting".

Nope, I think that'll do it--you correctly divined that the weirdness of having
the Add/Remove interfaces on CDMD was what was making me ootsy.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
chrome/browser/history/download_database.cc:194: "SET full_path=?,
received_bytes=?, state=?, end_time=?, total_bytes=?, "
On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> On 2012/11/02 23:31:25, rdsmith wrote:
> > Why change the persistence of full_path in the same CL as moving the history
> to
> > an observer?  They seem like they could (and sorta should) be separate.  (I
> > don't have strong feelings.)
> 
> This isn't actually changing what's persisted. This is just merging
> UpdateDownload with UpdateDownloadPath to simplify the API.

Ah, sorry, my braino.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
chrome/browser/history/history.h:455: virtual Handle CreateDownload(
On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> On 2012/11/02 23:31:25, rdsmith wrote:
> > Probably worthwhile putting a note in here somewhere about why methods are
> > virtual.  It's also a little strange that some are and some aren't; you
might
> > want to switch them all over.
> 
> Added a note below the section header on L446.
> I'd rather not touch others' APIs, especially in this already gargantuan CL.

I'm good with that, but I'd request that you specifically call this issue out to
Brett when you ask for his review--I'd like his conscious ok rather than him
potentially not noticing.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:619:
profile_->GetOriginalProfile(), Profile::EXPLICIT_ACCESS);
As I read the code, you don't need the GetOriginalProfile(); the history service
factory redirects from incognito to regular if EXPLICIT_ACCESS.  And this code
is a little confusing to the reader for non-incognito profiles.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:154: struct
VisitedReferrerBeforeDoneParameters {
Oh, I wish we didn't have to do this :-{.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:160: };
Style guide nit:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

(I consider "typedefs" to include "Type definitions" :-}.)

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/d...
File chrome/browser/download/download_service.h (right):

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/d...
chrome/browser/download/download_service.h:39: DownloadHistory*
GetDownloadHistory();
Specify may be null if OTR.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:246: DownloadItem*,
I'm confused enough about this interface to think it deserves a comment.  What
is the purpose of the filter?  Why does it take pointers to two DownloadItems?

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:247: const
DownloadPersistentStoreInfo&)> DownloadPersistedFilter;
nit: Style guide violation for ordering within a class.  It's test code, so I
don't care a lot, but it should probably still be at the top of the class.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:517: class
DownloadRemovedObserver : public DownloadPersistedObserver {
Why here?  I don't think it's a style guide issue, but my sense of custom has
been that these utility classes belong at the top of files.

[benjhayden, 2012-11-08 18:57:03]

PTAL if you haven't already LG'd.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:63: void
RemoveHistoryObserver(DownloadHistory::Observer* observer);
On 2012/11/07 21:10:29, rdsmith wrote:
> On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> > On 2012/11/02 23:31:25, rdsmith wrote:
> > > These appear to only be used by tests.  Any reason not to give tests some
> way
> > to
> > > get at the DownloadHistory object instead?  Or at least make these
> > "ForTesting"?
> > 
> > Exposed DownloadService::GetDownloadHistory(). LMK if you think I should
also
> > make DownloadHistory::Add/RemoveObserver "ForTesting".
> 
> Nope, I think that'll do it--you correctly divined that the weirdness of
having
> the Add/Remove interfaces on CDMD was what was making me ootsy.

Done.

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/do...
chrome/browser/history/download_database.cc:194: "SET full_path=?,
received_bytes=?, state=?, end_time=?, total_bytes=?, "
On 2012/11/07 21:10:29, rdsmith wrote:
> On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> > On 2012/11/02 23:31:25, rdsmith wrote:
> > > Why change the persistence of full_path in the same CL as moving the
history
> > to
> > > an observer?  They seem like they could (and sorta should) be separate. 
(I
> > > don't have strong feelings.)
> > 
> > This isn't actually changing what's persisted. This is just merging
> > UpdateDownload with UpdateDownloadPath to simplify the API.
> 
> Ah, sorry, my braino.

Done.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:619:
profile_->GetOriginalProfile(), Profile::EXPLICIT_ACCESS);
On 2012/11/07 21:10:29, rdsmith wrote:
> As I read the code, you don't need the GetOriginalProfile(); the history
service
> factory redirects from incognito to regular if EXPLICIT_ACCESS.  And this code
> is a little confusing to the reader for non-incognito profiles.

Done.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:154: struct
VisitedReferrerBeforeDoneParameters {
On 2012/11/07 21:10:29, rdsmith wrote:
> Oh, I wish we didn't have to do this :-{.

Be careful what you wish for.
I changed this back to how it was, and split the interpretation of the results
from the history out into a trampoline static callback
"VisitCountsToVisitedBefore". So now the call to the history service takes a
callback that takes a callback, each of which takes < 7 parameters.

http://codereview.chromium.org/10915180/diff/130059/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.h:160: };
On 2012/11/07 21:10:29, rdsmith wrote:
> Style guide nit:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> (I consider "typedefs" to include "Type definitions" :-}.)

Moot.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/d...
File chrome/browser/download/download_service.h (right):

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/d...
chrome/browser/download/download_service.h:39: DownloadHistory*
GetDownloadHistory();
On 2012/11/07 21:10:29, rdsmith wrote:
> Specify may be null if OTR.

Done.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:246: DownloadItem*,
On 2012/11/07 21:10:29, rdsmith wrote:
> I'm confused enough about this interface to think it deserves a comment.  What
> is the purpose of the filter?  Why does it take pointers to two DownloadItems?

I was going to do something clever, but it isn't necessary. PTAL.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:247: const
DownloadPersistentStoreInfo&)> DownloadPersistedFilter;
On 2012/11/07 21:10:29, rdsmith wrote:
> nit: Style guide violation for ordering within a class.  It's test code, so I
> don't care a lot, but it should probably still be at the top of the class.

Moot.

http://codereview.chromium.org/10915180/diff/146012/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:517: class
DownloadRemovedObserver : public DownloadPersistedObserver {
On 2012/11/07 21:10:29, rdsmith wrote:
> Why here?  I don't think it's a style guide issue, but my sense of custom has
> been that these utility classes belong at the top of files.

Done.

[benjhayden, 2012-11-08 19:11:06]

Scott asked that I use an adapter class between HistoryService and
DownloadHistory instead of mocking HistoryService directly. This won't change
much, so feel free to keep reviewing, but be aware that this small change is
coming.

[benjhayden, 2012-11-08 19:56:36]

On 2012/11/08 19:11:06, benjhayden_chromium wrote:
> Scott asked that I use an adapter class between HistoryService and
> DownloadHistory instead of mocking HistoryService directly. This won't change
> much, so feel free to keep reviewing, but be aware that this small change is
> coming.

DownloadHistory::HistoryAdapter is in PS54.

[sky, 2012-11-08 20:55:17]

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
chrome/browser/history/download_database.cc:257: void
DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) {
If this is going to delete a download at a time, it should do just that: take a
single download. Have the caller (HistoryBackend) handle repeatedly invoking
this method and move UMA/other stuff out of here.

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
File chrome/browser/history/download_persistent_store_info.h (right):

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
chrome/browser/history/download_persistent_store_info.h:5: #ifndef
CHROME_BROWSER_HISTORY_DOWNLOAD_PERSISTENT_STORE_INFO_H_
It looks like you're moving this from content. How come it shows as a new file?

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/h...
File chrome/browser/history/history.h (left):

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/h...
chrome/browser/history/history.h:509: void RemoveDownloadsBetween(base::Time
remove_begin, base::Time remove_end);
Isn't browsing_data_remover using this?

[benjhayden, 2012-11-08 21:40:03]

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/hi...
chrome/browser/history/history.h:455: virtual Handle CreateDownload(
On 2012/11/07 21:10:29, rdsmith wrote:
> On 2012/11/06 20:01:14, benjhayden_chromium wrote:
> > On 2012/11/02 23:31:25, rdsmith wrote:
> > > Probably worthwhile putting a note in here somewhere about why methods are
> > > virtual.  It's also a little strange that some are and some aren't; you
> might
> > > want to switch them all over.
> > 
> > Added a note below the section header on L446.
> > I'd rather not touch others' APIs, especially in this already gargantuan CL.
> 
> I'm good with that, but I'd request that you specifically call this issue out
to
> Brett when you ask for his review--I'd like his conscious ok rather than him
> potentially not noticing.

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
chrome/browser/history/download_database.cc:257: void
DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) {
On 2012/11/08 20:55:17, sky wrote:
> If this is going to delete a download at a time, it should do just that: take
a
> single download. Have the caller (HistoryBackend) handle repeatedly invoking
> this method and move UMA/other stuff out of here.

Can you say more?
It looks to me like HistoryBackend is a thin layer of glue code w.r.t. the
downloads table. It looks like DownloadDatabase was created specifically to
encapsulate all of the downloads table-related logic.
That strategy would require exposing CountDownloads() just for UMA.
I'd be happy to split the DELETE FROM statement out into a separate private
method on DownloadDatabase if you're just looking for a little more code
organization and modularity.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
File chrome/browser/history/download_persistent_store_info.h (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
chrome/browser/history/download_persistent_store_info.h:5: #ifndef
CHROME_BROWSER_HISTORY_DOWNLOAD_PERSISTENT_STORE_INFO_H_
On 2012/11/08 20:55:17, sky wrote:
> It looks like you're moving this from content. How come it shows as a new
file?

I'm sorry, I don't know.
I manually computed the diffs:
http://www/~benjhayden/10915180_download_persistent_store_info.h.diff
http://www/~benjhayden/10915180_download_persistent_store_info.cc.diff

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/hi...
File chrome/browser/history/history.h (left):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/hi...
chrome/browser/history/history.h:509: void RemoveDownloadsBetween(base::Time
remove_begin, base::Time remove_end);
On 2012/11/08 20:55:17, sky wrote:
> Isn't browsing_data_remover using this?

browsing_data_remover is using DownloadManager::RemoveDownloadsBetween, which
uses HistoryService::RemoveDownloads as of this CL.
At some point, I might want to try to simplify the DownloadManager interface
w.r.t. RemoveDownloadsBetween, but not in this CL.

[asanka, 2012-11-08 22:45:53]

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:307:
.WillRepeatedly(Return());
Nit: indent

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:341:
.WillRepeatedly(Return());
These expectations are already set below.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history.cc:291: if (!item) {
DCHECK instead?

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history.cc:392: // DownloadHistoryData::info().
Given that ShouldUpdateHistory() returns true if info() is NULL, isn't
OnDownloadOpened() the same as OnDownloadUpdated() (modulo UMA)? It'll be
simpler to call OnDownloadUpdated().

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:373:
IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
Why are these browser tests?

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:570: // Instead of calling
RemoveDownloads() immediately, it should
Comment ends abruptly.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:690: }
Missing test case for download that's updated while adding. The download should
be updated in history immediately after its added.

[sky, 2012-11-09 16:45:50]

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/d...
chrome/browser/history/download_database.cc:257: void
DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) {
On 2012/11/08 21:40:03, benjhayden_chromium wrote:
> On 2012/11/08 20:55:17, sky wrote:
> > If this is going to delete a download at a time, it should do just that:
take
> a
> > single download. Have the caller (HistoryBackend) handle repeatedly invoking
> > this method and move UMA/other stuff out of here.
> 
> Can you say more?

We have generally kept the DataBase implementations minimal and pushed as much
logic as possible to the caller. For example, URLDatabase does not have a
DeleteURLRows(std::std<>) even though it could be used. Instead we have
DeleteURLRow(URLID) and left it to callers to invoke it multiple times as
necessary.

> It looks to me like HistoryBackend is a thin layer of glue code w.r.t. the
> downloads table. It looks like DownloadDatabase was created specifically to
> encapsulate all of the downloads table-related logic.
> That strategy would require exposing CountDownloads() just for UMA.
> I'd be happy to split the DELETE FROM statement out into a separate private
> method on DownloadDatabase if you're just looking for a little more code
> organization and modularity.

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/h...
File chrome/browser/history/history.h (left):

https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/h...
chrome/browser/history/history.h:509: void RemoveDownloadsBetween(base::Time
remove_begin, base::Time remove_end);
On 2012/11/08 21:40:03, benjhayden_chromium wrote:
> On 2012/11/08 20:55:17, sky wrote:
> > Isn't browsing_data_remover using this?
> 
> browsing_data_remover is using DownloadManager::RemoveDownloadsBetween, which
> uses HistoryService::RemoveDownloads as of this CL.
> At some point, I might want to try to simplify the DownloadManager interface
> w.r.t. RemoveDownloadsBetween, but not in this CL.

My mistake.

[Randy Smith (Not in Mondays), 2012-11-09 21:36:39]

Could you skim over the logging statements you've added in this CL and just
confirm that you believe that they'll be useful for general debugging in the
future rather than being targeted at just this CL?  I'd rather not add random
logging statements unless we think they'll be useful.

This is getting close; I think when we're resolve the below issues I'll be good
with this CL.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:149: void
VisitCountsToVisitedBefore(
*snort*  No problem can't be solved by another layer of indirection :-}.  I do
like this better than the previous (hacky though it is), but please put in a
comment indicating why you're doing it.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_browsertest.cc:1370: if (service &&
service->GetDownloadManagerDelegate())
Why is it relevant whether or not the service has a DownloadManagerDelegate?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history.h:75: static bool
IsPersisted(content::DownloadItem* item);
I'll call out that I'm uncomfortable that this method is static; I think of
statics as worth avoiding unless there's a better way.  The primary argument
I've heard against them is that they can't be mocked if someone wants to write a
test for some other class that uses the specified class.  I'll add a secondary
argument that this method being static reflects an implementation detail, which
is that we're storing the history related DownloadITem information as
ExternalData on the download item, as opposed to in a map inside the
DownloadHistory.  

But I consider both those arguments weak, so I'll make them and leave the choice
up to you.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:31:
CHECK_EQ(left.path.value(), right.path.value());
Why CHECK instead of EXPECT?  That'll give you a crash as opposed to a failed
test.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:47: class
FakeHistoryService : public DownloadHistory::HistoryAdapter {
nit: I think the name should now be FakeHistoryAdapter?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:59: const
HistoryService::DownloadQueryCallback& callback) OVERRIDE {
Suggestion: It would be slightly easier to understand how these functions were
going to be used with DCHECK(CurrentlyOn...)) or comments about threads methods
run on.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:78:
create_download_callback_ = base::Bind(
Suggestion: The implication of just having a single place to store the callback
is that you'll always be waiting until FinishCreateDownload() is called before
the next invocation of CreateDownload().  Is it worthwhile putting in a DCHECK()
so that someone who violates that invariant in a new test realizes quickly that
they have to update this code?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:156: LOG(ERROR) <<
__FUNCTION__ << " " << *oops;
I think of "EXPECT_TRUE(false) << ..." as a better idiom for tests--it signals
that you're outputting test data for failing expectations.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:158:
CHECK(difference.empty());
Why a CHECK?  Isn't this a test expectation, and hence something you want to
signal failure for rather than crash?  (And if you follow my suggestion above,
you don't need the extra signaling.)

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:407:
item_observer()->OnDownloadDestroyed(&item(0));
Just a crash test, or is there some behavior on the history you can test here?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:56: class
DownloadPersistedObserver : public DownloadHistory::Observer {
A sentence or two in a comment about how to use this?  An observer that triggers
(returns?) if any persisted download item causes the passed filter to return
true?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:71: if (service &&
service->GetDownloadManagerDelegate())
Why is the download manager delegate relevant?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:145: if (info.path !=
expected_path) {
Why not EXPECT_EQ?  Seems more in line with how tests are usually written.  You
could have the set of expectations, then one big && return at the end for the
return value.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:279: // is |expected_url|.
This doesn't feel like an accurate description.  "Returns after a download item
has been created and persisted (as determined by the filter).  Returns true if
that download matches |expected_url|"?

Actually, the description needs to specifically call out that a null filter
disables waiting for download item persistence.

Mind you, written out like that, the interface contract doesn't make a lot of
sense, and it might be reasonable to change it.  Maybe there's a more useful
interface contract?  Maybe separating out "finish" and "confirming that download
was stored properly" (running the filter)?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:306:
DownloadPersistedObserver(browser->profile(), filter).WaitForPersisted();
This feels ... wrong to me.  Specifically, the behavior of this routine is now
race-dependent--if the download_item is persisted before this point, filter is
not checked, and if it's not, filter is gating.  Could you write this code so
the results aren't dependent on the winner of a race?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:910:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
I'm confused by this test.  It seems to rely on a quirk of the current
implementation that we're probably going to change at some point.  The target
file path is set during download filename determination, and
CreateSlowTestDownload() returns when it's got a Download to the IN_PROGRESS
state.  Is there any reason these tests should fail if we ever make the download
visible before filename determination?

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:954:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
What are you testing here?

[benjhayden, 2012-11-12 18:44:16]

All the log statements that I added are VLOG(20), so they should only bother
folks like us who set --vmodule=*download*=20.
Most of the log statements that I added are in public DownloadItemImpl methods,
so they should only appear when something significant happens to the download
item. I can take them out if you feel strongly. I tend to favor too much logging
rather than too little since I rely solely on logging for debugging.

PTAL if you have not already LG'd.
Thanks!

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:307:
.WillRepeatedly(Return());
On 2012/11/08 22:45:53, asanka wrote:
> Nit: indent

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:341:
.WillRepeatedly(Return());
On 2012/11/08 22:45:53, asanka wrote:
> These expectations are already set below.

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history.cc:291: if (!item) {
On 2012/11/08 22:45:53, asanka wrote:
> DCHECK instead?

I don't see any mechanism to prevent items from being deleted between
CreateDownload and ItemAdded.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history.cc:392: // DownloadHistoryData::info().
On 2012/11/08 22:45:53, asanka wrote:
> Given that ShouldUpdateHistory() returns true if info() is NULL, isn't
> OnDownloadOpened() the same as OnDownloadUpdated() (modulo UMA)? It'll be
> simpler to call OnDownloadUpdated().

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:373:
IN_PROC_BROWSER_TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
On 2012/11/08 22:45:53, asanka wrote:
> Why are these browser tests?

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:570: // Instead of calling
RemoveDownloads() immediately, it should
On 2012/11/08 22:45:53, asanka wrote:
> Comment ends abruptly.

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:690: }
On 2012/11/08 22:45:53, asanka wrote:
> Missing test case for download that's updated while adding. The download
should
> be updated in history immediately after its added.

Done.

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/do...
chrome/browser/history/download_database.cc:257: void
DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) {
On 2012/11/09 16:45:50, sky wrote:
> On 2012/11/08 21:40:03, benjhayden_chromium wrote:
> > On 2012/11/08 20:55:17, sky wrote:
> > > If this is going to delete a download at a time, it should do just that:
> take
> > a
> > > single download. Have the caller (HistoryBackend) handle repeatedly
invoking
> > > this method and move UMA/other stuff out of here.
> > 
> > Can you say more?
> 
> We have generally kept the DataBase implementations minimal and pushed as much
> logic as possible to the caller. For example, URLDatabase does not have a
> DeleteURLRows(std::std<>) even though it could be used. Instead we have
> DeleteURLRow(URLID) and left it to callers to invoke it multiple times as
> necessary.
> 
> > It looks to me like HistoryBackend is a thin layer of glue code w.r.t. the
> > downloads table. It looks like DownloadDatabase was created specifically to
> > encapsulate all of the downloads table-related logic.
> > That strategy would require exposing CountDownloads() just for UMA.
> > I'd be happy to split the DELETE FROM statement out into a separate private
> > method on DownloadDatabase if you're just looking for a little more code
> > organization and modularity.
> 

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:149: void
VisitCountsToVisitedBefore(
On 2012/11/09 21:36:39, rdsmith wrote:
> *snort*  No problem can't be solved by another layer of indirection :-}.  I do
> like this better than the previous (hacky though it is), but please put in a
> comment indicating why you're doing it.

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_browsertest.cc:1370: if (service &&
service->GetDownloadManagerDelegate())
On 2012/11/09 21:36:39, rdsmith wrote:
> Why is it relevant whether or not the service has a DownloadManagerDelegate?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history.h:75: static bool
IsPersisted(content::DownloadItem* item);
On 2012/11/09 21:36:39, rdsmith wrote:
> I'll call out that I'm uncomfortable that this method is static; I think of
> statics as worth avoiding unless there's a better way.  The primary argument
> I've heard against them is that they can't be mocked if someone wants to write
a
> test for some other class that uses the specified class.  I'll add a secondary
> argument that this method being static reflects an implementation detail,
which
> is that we're storing the history related DownloadITem information as
> ExternalData on the download item, as opposed to in a map inside the
> DownloadHistory.  
> 
> But I consider both those arguments weak, so I'll make them and leave the
choice
> up to you.

There's also no way to mock DownloadHistory out of DownloadService, or mock
HistoryService.
This CL currently has no need to mock IsPersisted or DownloadHistory.
If a future CL needs to mock IsPersisted, then I'd suggest adding
static void SetIsPersistedForTesting(content::DownloadItem* item, bool
persisted);
If IsPersisted needs to move to being an instance method rather than a static
method, then it should be easy enough to migrate callers to call through
DownloadService.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:31:
CHECK_EQ(left.path.value(), right.path.value());
On 2012/11/09 21:36:39, rdsmith wrote:
> Why CHECK instead of EXPECT?  That'll give you a crash as opposed to a failed
> test.

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:47: class
FakeHistoryService : public DownloadHistory::HistoryAdapter {
On 2012/11/09 21:36:39, rdsmith wrote:
> nit: I think the name should now be FakeHistoryAdapter?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:59: const
HistoryService::DownloadQueryCallback& callback) OVERRIDE {
On 2012/11/09 21:36:39, rdsmith wrote:
> Suggestion: It would be slightly easier to understand how these functions were
> going to be used with DCHECK(CurrentlyOn...)) or comments about threads
methods
> run on.

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:78:
create_download_callback_ = base::Bind(
On 2012/11/09 21:36:39, rdsmith wrote:
> Suggestion: The implication of just having a single place to store the
callback
> is that you'll always be waiting until FinishCreateDownload() is called before
> the next invocation of CreateDownload().  Is it worthwhile putting in a
DCHECK()
> so that someone who violates that invariant in a new test realizes quickly
that
> they have to update this code?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:156: LOG(ERROR) <<
__FUNCTION__ << " " << *oops;
On 2012/11/09 21:36:39, rdsmith wrote:
> I think of "EXPECT_TRUE(false) << ..." as a better idiom for tests--it signals
> that you're outputting test data for failing expectations.  

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:158:
CHECK(difference.empty());
On 2012/11/09 21:36:39, rdsmith wrote:
> Why a CHECK?  Isn't this a test expectation, and hence something you want to
> signal failure for rather than crash?  (And if you follow my suggestion above,
> you don't need the extra signaling.)

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:407:
item_observer()->OnDownloadDestroyed(&item(0));
On 2012/11/09 21:36:39, rdsmith wrote:
> Just a crash test, or is there some behavior on the history you can test here?

I forget why I wrote those. They're swallowed by ADIN.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:56: class
DownloadPersistedObserver : public DownloadHistory::Observer {
On 2012/11/09 21:36:39, rdsmith wrote:
> A sentence or two in a comment about how to use this?  An observer that
triggers
> (returns?) if any persisted download item causes the passed filter to return
> true?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:71: if (service &&
service->GetDownloadManagerDelegate())
On 2012/11/09 21:36:39, rdsmith wrote:
> Why is the download manager delegate relevant?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:145: if (info.path !=
expected_path) {
On 2012/11/09 21:36:39, rdsmith wrote:
> Why not EXPECT_EQ?  Seems more in line with how tests are usually written. 
You
> could have the set of expectations, then one big && return at the end for the
> return value.

DownloadStoredProperly may be called multiple times, so returning false doesn't
necessarily mean that the test failed, it might just mean that the test hasn't
passed yet.
Changed these to VLOG(20)s and added a comment.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:279: // is |expected_url|.
On 2012/11/09 21:36:39, rdsmith wrote:
> This doesn't feel like an accurate description.  "Returns after a download
item
> has been created and persisted (as determined by the filter).  Returns true if
> that download matches |expected_url|"?
> 
> Actually, the description needs to specifically call out that a null filter
> disables waiting for download item persistence.
> 
> Mind you, written out like that, the interface contract doesn't make a lot of
> sense, and it might be reasonable to change it.  Maybe there's a more useful
> interface contract?  Maybe separating out "finish" and "confirming that
download
> was stored properly" (running the filter)?  

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:306:
DownloadPersistedObserver(browser->profile(), filter).WaitForPersisted();
On 2012/11/09 21:36:39, rdsmith wrote:
> This feels ... wrong to me.  Specifically, the behavior of this routine is now
> race-dependent--if the download_item is persisted before this point, filter is
> not checked, and if it's not, filter is gating.  Could you write this code so
> the results aren't dependent on the winner of a race?

Done.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:910:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
On 2012/11/09 21:36:39, rdsmith wrote:
> I'm confused by this test.  It seems to rely on a quirk of the current
> implementation that we're probably going to change at some point.  The target
> file path is set during download filename determination, and
> CreateSlowTestDownload() returns when it's got a Download to the IN_PROGRESS
> state.  Is there any reason these tests should fail if we ever make the
download
> visible before filename determination?

Discussed offline.
The item is already available before filename determination.
DownloadsGetFileIconFunction relies on GetTargetFilePath.
DownloadsGetFileIconFunction would fail mysteriously if
DownloadTestObserverInProgress::IsDownloadInFinalState() did not check
GetTargetFilePath.
If this expect fails, then it will clarify why DownloadsGetFileIconFunction also
fails.
Changed to ASSERT.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:954:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
On 2012/11/09 21:36:39, rdsmith wrote:
> What are you testing here?

The same as above, but for a different DownloadItem.

[Randy Smith (Not in Mondays), 2012-11-12 19:24:30]

LGTM.q

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:145: if (info.path !=
expected_path) {
On 2012/11/12 18:44:16, benjhayden_chromium wrote:
> On 2012/11/09 21:36:39, rdsmith wrote:
> > Why not EXPECT_EQ?  Seems more in line with how tests are usually written. 
> You
> > could have the set of expectations, then one big && return at the end for
the
> > return value.
> 
> DownloadStoredProperly may be called multiple times, so returning false
doesn't
> necessarily mean that the test failed, it might just mean that the test hasn't
> passed yet.
> Changed these to VLOG(20)s and added a comment.

Whoops--good point.

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:954:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
On 2012/11/12 18:44:16, benjhayden_chromium wrote:
> On 2012/11/09 21:36:39, rdsmith wrote:
> > What are you testing here?
> 
> The same as above, but for a different DownloadItem.

Should this also be an ASSERT?  (Up to you.)

[benjhayden, 2012-11-12 19:59:53]

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:954:
EXPECT_FALSE(download_item->GetTargetFilePath().empty());
On 2012/11/12 19:24:30, rdsmith wrote:
> On 2012/11/12 18:44:16, benjhayden_chromium wrote:
> > On 2012/11/09 21:36:39, rdsmith wrote:
> > > What are you testing here?
> > 
> > The same as above, but for a different DownloadItem.
> 
> Should this also be an ASSERT?  (Up to you.)

Done.

[asanka, 2012-11-12 22:51:13]

LGTM

[sky, 2012-11-13 20:45:51]

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
chrome/browser/history/download_database.cc:26: const int64
DownloadDatabase::kUninitializedHandle = -1;
nit: for static we generally put "// static" on the previous line.

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
File chrome/browser/history/download_persistent_store_info.h (right):

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
chrome/browser/history/download_persistent_store_info.h:16: struct
DownloadPersistentStoreInfo {
If you're putting this in the history directory it should be in the history
namespace. And since you're doing that I would name it DownloadInfo. You could
also name it DownloadRow, which is consistent with other similar structures.

[benjhayden, 2012-11-13 21:24:13]

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
chrome/browser/history/download_database.cc:26: const int64
DownloadDatabase::kUninitializedHandle = -1;
On 2012/11/13 20:45:52, sky wrote:
> nit: for static we generally put "// static" on the previous line.

Done.

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
File chrome/browser/history/download_persistent_store_info.h (right):

https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/d...
chrome/browser/history/download_persistent_store_info.h:16: struct
DownloadPersistentStoreInfo {
On 2012/11/13 20:45:52, sky wrote:
> If you're putting this in the history directory it should be in the history
> namespace. And since you're doing that I would name it DownloadInfo. You could
> also name it DownloadRow, which is consistent with other similar structures.

Done.

[sky, 2012-11-13 22:05:14]

LGTM

[benjhayden, 2012-11-13 22:18:32]

Thanks everybody!
I'll make a final self-review, run the trybots, and commit tomorrow.

[commit-bot: I haz the power, 2012-11-14 18:01:00]

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

[commit-bot: I haz the power, 2012-11-14 20:56:04]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-19 21:23:44]

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

[commit-bot: I haz the power, 2012-11-19 22:27:34]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-19 22:30:00]

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

[commit-bot: I haz the power, 2012-11-20 00:52:14]

Change committed as 168670

[Randy Smith (Not in Mondays), 2012-12-04 21:48:32]

https://chromiumcodereview.appspot.com/10915180/diff/172001/chrome/browser/do...
File chrome/browser/download/download_history_unittest.cc (right):

https://chromiumcodereview.appspot.com/10915180/diff/172001/chrome/browser/do...
chrome/browser/download/download_history_unittest.cc:351:
EXPECT_CALL(item(index), GetFullPath()).WillRepeatedly(ReturnRef(path));
Apologies for not catching this on the first time through, but I *think* this is
a bug.  Aren't you storing a ref to an automatic variable, which means you're
effectively storing a pointer to a random location on the stack after the
function returns?

[Randy Smith (Not in Mondays), 2012-12-05 13:00:29]

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

https://codereview.chromium.org/10915180/diff/172001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:351:
EXPECT_CALL(item(index), GetFullPath()).WillRepeatedly(ReturnRef(path));
On 2012/12/04 21:48:32, rdsmith wrote:
> Apologies for not catching this on the first time through, but I *think* this
is
> a bug.  Aren't you storing a ref to an automatic variable, which means you're
> effectively storing a pointer to a random location on the stack after the
> function returns?

Whoops, I see; the argument to the function is a reference, and you're relying
on the caller to keep the reference from going out of scope while item(index) is
around.  I'm inclined to think that should take a comment, but I'll put it in as
part of my current CL.  Sorry to bug you.