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]().
SavePages are now visible in chrome://downloads and the shelf. Even huge pages like cnn.com finish so quickly that the user never has a chance to pause/resume/cancel them (so I'm not entirely sure what would happen if they did), and they are never marked as dangerous. Opening them works fine.

[Randy Smith (Not in Mondays), 2012-08-02 22:47:56]

Actually share notes with you :-}.

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

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:163: // |db_handle|, because
DownloadItem doesn't know its |db_handle|.
Refactor load path into DM so that the DH creates each DI one at a time, and the
DM returns the DI, at which point the db handle can be added directly; no need
for matching.

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:256: data->set_db_handle(db_handle);
Maybe also store the last DownloadPersistentStateInfo in external data as well?

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

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.h:76: typedef std::map<int32,
content::DownloadPersistentStoreInfo> InfoMap;
Maybe put in a UMA to track how often we get an OnDownloadUpdated that we ignore
because there's no change we care about.  If it's small enough, we could get rid
of the maps and unilaterally update the history.

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.h:76: typedef std::map<int32,
content::DownloadPersistentStoreInfo> InfoMap;
Evict persistent infos from the map on completion and matched up with its DB
handle (avoid having space requirements proportional to number of historical
downloads).

[benjhayden, 2012-08-14 21:31:47]

PTAL
I think this beast is finally ready for an initial review.
Please feel free to suggest more behaviors that DownloadHistoryTest could test.

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

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:163: // |db_handle|, because
DownloadItem doesn't know its |db_handle|.
On 2012/08/02 22:47:56, rdsmith wrote:
> Refactor load path into DM so that the DH creates each DI one at a time, and
the
> DM returns the DI, at which point the db handle can be added directly; no need
> for matching.

Done.

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.cc:256: data->set_db_handle(db_handle);
On 2012/08/02 22:47:56, rdsmith wrote:
> Maybe also store the last DownloadPersistentStateInfo in external data as
well?

Done.

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

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.h:76: typedef std::map<int32,
content::DownloadPersistentStoreInfo> InfoMap;
On 2012/08/02 22:47:56, rdsmith wrote:
> Maybe put in a UMA to track how often we get an OnDownloadUpdated that we
ignore
> because there's no change we care about.  If it's small enough, we could get
rid
> of the maps and unilaterally update the history.

Done.

http://codereview.chromium.org/10665049/diff/18001/chrome/browser/download/do...
chrome/browser/download/download_history.h:76: typedef std::map<int32,
content::DownloadPersistentStoreInfo> InfoMap;
On 2012/08/02 22:47:56, rdsmith wrote:
> Evict persistent infos from the map on completion and matched up with its DB
> handle (avoid having space requirements proportional to number of historical
> downloads).

Done.

[Randy Smith (Not in Mondays), 2012-08-15 20:47:43]

High level review.

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:4: 
Explanatory comment at top of file about basic architecture?  Interaction with
DM/DI via observers & UserData, interaction with DB thread via ?, any other high
level information that would help someone reading through this file?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:21: class DownloadHistoryData :
public base::SupportsUserData::Data {
Comment documenting purpose and use?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:23: static const int64
kUninitializedHandle = -1;
Why here?  I would think this would be a DownloadHistory of DownloadDatabase
concept?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:159: manager_->RemoveObserver(this);
DCHECK that the manager has no items on it?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:163: DownloadHistory::InfoVector*
infos) {
Who owns the storage in the vector pointed to by Infos?  Who releases it?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:214: bool removing =
(removing_.find(data->db_handle()) != removing_.end());
Should this be a DCHECK?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:243: // We should have caught
OnDownloadDestroyed().  If the item should have
Does this mean we could have a DCHECK() here?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:326: if (item->GetState() ==
DownloadItem::IN_PROGRESS) {
Can they be opened before they're completed?  I.e. is this test necessary?

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:20: // Wrap HistoryService for easy
mocking and to hide CancelableRequestConsumer.
Document the lifetime guarantees that this class provides.  I think the primary
one that needs to be documented is that no callback passed to this class will be
called after the class is destroyed.  

Secondary question: Would it be a good idea to change this lifetime guarantee
and say that the callbacks have to provide their own protection, e.g. via
WeakPtrs?  I don't have a vision of what that implementation would look like,
but I could imagine it was as simple as doing a WeakPtrFactory on
DownloadHistory and using it, and I suspect someone familiar with chrome
patterns would find it easier code to read.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:21: class
HistoryServiceDownloadAdapter {
Why in .h file rather than a forward decl?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:48: public
base::SupportsWeakPtr<DownloadHistory> {
We've talked about my concerns about SupportsWeakPtr.  What's the argument for
using this, rather than WeakPtrFactory?  Does it need to be part of the public
interface of the class?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: typedef
std::vector<DownloadPersistentStoreInfo> InfoVector;
InfoVector isn't used in the public portion of the interface--why is it a public
typedef?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:52: typedef std::set<int64>
HandleSet;
Ditto HandleSet.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:61: // ownership of |callback|.
Not this CL's problem, but: "Takes ownership of |callback|"?  What does that
mean?  a) callbacks are values, and copyable, and b) it's a const reference.  Do
you have any idea what's going on here?

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:107: // Cannot be merged into
DownloadHistoryData.
Document that the motivation for this is batching removes (dependent on
investigation of possibility of remove -> MaybeAddToHistory).

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:110: // |GetId()|s of items that were
removed while they were being added.
I wince because these comments aren't really understandable (at least by me)
without reading the .cc file.  In some ways that's inevitable, but if there is a
way to give a just .h file understandable description of these items and move
the implementation specific details into the .cc file (e.g. references to
DownloadHistoryData) that would be better.  Failing that, a one line description
of what a DownloadHistoryData is here would help.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:111: std::set<int32>
removed_while_adding_;
Document that the motivation is that we can't remove a download until we get the
add callback giving us the DB handle.

[Randy Smith (Not in Mondays), 2012-08-15 20:49:58]

Two high level thoughts:

-- I'd like to see the description rewritten a bit:
* Have a single line at the top that summarizes what the CL does, followed (next
paragraph) by more detail, but still at a high level.
* Only include bulletted list stuff if it's necessary to understand what the CL
does, and try to tie it into the high level description.
* Only include information about what this CL does, not what you thought about
including and didn't, or what you're going to do next (though belong in the
messages with which you send out requests for comments).

-- Should the mime_util_dbg crash fix be in a separate CL?

[benjhayden, 2012-08-17 18:20:45]

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:4: 
On 2012/08/15 20:47:44, rdsmith wrote:
> Explanatory comment at top of file about basic architecture?  Interaction with
> DM/DI via observers & UserData, interaction with DB thread via ?, any other
high
> level information that would help someone reading through this file?  

I had hoped that the code could speak for itself, but I wrote something that
you're welcome to rewrite or ask me to remove.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:21: class DownloadHistoryData :
public base::SupportsUserData::Data {
On 2012/08/15 20:47:44, rdsmith wrote:
> Comment documenting purpose and use?

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:23: static const int64
kUninitializedHandle = -1;
On 2012/08/15 20:47:44, rdsmith wrote:
> Why here?  I would think this would be a DownloadHistory of DownloadDatabase
> concept?

Moved it out into the anonymous namespace.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:159: manager_->RemoveObserver(this);
On 2012/08/15 20:47:44, rdsmith wrote:
> DCHECK that the manager has no items on it?

Did the observing_items_ set trick instead.
I'll start writing AddObserver(WeakPtr<Observer>) by the time you read this.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:163: DownloadHistory::InfoVector*
infos) {
On 2012/08/15 20:47:44, rdsmith wrote:
> Who owns the storage in the vector pointed to by Infos?  Who releases it?

HistoryService::QueryDownloads() creates a history::DownloadQueryRequest(),
which contains this vector, and passes it to HistoryBackend::QueryDownloads(),
which stuffs it in a scoped_refptr. There's some callback magic in
DownloadQueryRequest's base class, but that's pretty much the whole story.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:214: bool removing =
(removing_.find(data->db_handle()) != removing_.end());
On 2012/08/15 20:47:44, rdsmith wrote:
> Should this be a DCHECK?

I could imagine DownloadItem sending one or two final OnDownloadUpdated calls
between OnDownloadRemoved and OnDownloadDestroyed at some point. It seems like
an implementation detail that that currently can't happen.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:243: // We should have caught
OnDownloadDestroyed().  If the item should have
On 2012/08/15 20:47:44, rdsmith wrote:
> Does this mean we could have a DCHECK() here?

Nope. There's no mechanism that cancels this particular ItemAdded() callback
just because OnDownloadDestroyed() is called.
Thanks for making me take another look at this code. The removed_while_adding_
stuff did not depend on !item, so I moved it up a couple lines.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:326: if (item->GetState() ==
DownloadItem::IN_PROGRESS) {
On 2012/08/15 20:47:44, rdsmith wrote:
> Can they be opened before they're completed?  I.e. is this test necessary?

Wasn't there some chatter about streaming downloads to applications just the
other day?
Either way, feels like an implementation detail.

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:20: // Wrap HistoryService for easy
mocking and to hide CancelableRequestConsumer.
On 2012/08/15 20:47:44, rdsmith wrote:
> Document the lifetime guarantees that this class provides.  I think the
primary
> one that needs to be documented is that no callback passed to this class will
be
> called after the class is destroyed.  

Done.

> 
> Secondary question: Would it be a good idea to change this lifetime guarantee
> and say that the callbacks have to provide their own protection, e.g. via
> WeakPtrs?  I don't have a vision of what that implementation would look like,
> but I could imagine it was as simple as doing a WeakPtrFactory on
> DownloadHistory and using it, and I suspect someone familiar with chrome
> patterns would find it easier code to read.

Added the WeakPtrFactory to DownloadHistory.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:21: class
HistoryServiceDownloadAdapter {
On 2012/08/15 20:47:44, rdsmith wrote:
> Why in .h file rather than a forward decl?

The test needs to be able to override the methods.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:48: public
base::SupportsWeakPtr<DownloadHistory> {
On 2012/08/15 20:47:44, rdsmith wrote:
> We've talked about my concerns about SupportsWeakPtr.  What's the argument for
> using this, rather than WeakPtrFactory?  Does it need to be part of the public
> interface of the class?

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:51: typedef
std::vector<DownloadPersistentStoreInfo> InfoVector;
On 2012/08/15 20:47:44, rdsmith wrote:
> InfoVector isn't used in the public portion of the interface--why is it a
public
> typedef?

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:52: typedef std::set<int64>
HandleSet;
On 2012/08/15 20:47:44, rdsmith wrote:
> Ditto HandleSet.

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:61: // ownership of |callback|.
On 2012/08/15 20:47:44, rdsmith wrote:
> Not this CL's problem, but: "Takes ownership of |callback|"?  What does that
> mean?  a) callbacks are values, and copyable, and b) it's a const reference. 
Do
> you have any idea what's going on here?

I think it meant that it copied the callback to an internal map from
HistoryService::Handle to the callback. But that's an implementation detail, so
I deleted the comment.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:107: // Cannot be merged into
DownloadHistoryData.
On 2012/08/15 20:47:44, rdsmith wrote:
> Document that the motivation for this is batching removes (dependent on
> investigation of possibility of remove -> MaybeAddToHistory).

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:110: // |GetId()|s of items that were
removed while they were being added.
On 2012/08/15 20:47:44, rdsmith wrote:
> I wince because these comments aren't really understandable (at least by me)
> without reading the .cc file.  In some ways that's inevitable, but if there is
a
> way to give a just .h file understandable description of these items and move
> the implementation specific details into the .cc file (e.g. references to
> DownloadHistoryData) that would be better.  Failing that, a one line
description
> of what a DownloadHistoryData is here would help.

Done.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:111: std::set<int32>
removed_while_adding_;
On 2012/08/15 20:47:44, rdsmith wrote:
> Document that the motivation is that we can't remove a download until we get
the
> add callback giving us the DB handle.

Done.

[Randy Smith (Not in Mondays), 2012-08-20 18:00:00]

On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> Did the observing_items_ set trick instead.
> I'll start writing AddObserver(WeakPtr<Observer>) by the time you read this.

Just a note that the right place for that is deep in the guts of chrome
(base/observer_list.h), and there's often a lot of resistance to making those
changes, so you might want to run the idea past someone who's a guardian of base
before you waste time on it.

[Randy Smith (Not in Mondays), 2012-08-20 18:57:04]

Another round of pokes at download_history.*.

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:4: 
On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> On 2012/08/15 20:47:44, rdsmith wrote:
> > Explanatory comment at top of file about basic architecture?  Interaction
with
> > DM/DI via observers & UserData, interaction with DB thread via ?, any other
> high
> > level information that would help someone reading through this file?  
> 
> I had hoped that the code could speak for itself, but I wrote something that
> you're welcome to rewrite or ask me to remove.

L G T M; thanks.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:23: static const int64
kUninitializedHandle = -1;
On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> On 2012/08/15 20:47:44, rdsmith wrote:
> > Why here?  I would think this would be a DownloadHistory of DownloadDatabase
> > concept?
> 
> Moved it out into the anonymous namespace.

I think I was just confused in my earlier comment.  If no-one outside this file
every uses -1 for a db handle to indicate an uninitialized handle, and if the
mechanism for allocating db handles is in this file, it's appropriate for this
to be here (I don't have any strong feeling about current placement versus
inside the helper class; either's fine).  But if other files are structuring the
download handle namespace, I'd think this would belong in those other locations.
 I'll assume for now that all db_handle namespace manipulations are being done
in this file now (so please ignore my original comment and you can put the
constant wherever you'd like) but will mark up other files appropriately if I
see counter-examples.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:243: // We should have caught
OnDownloadDestroyed().  If the item should have
On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> On 2012/08/15 20:47:44, rdsmith wrote:
> > Does this mean we could have a DCHECK() here?
> 
> Nope. There's no mechanism that cancels this particular ItemAdded() callback
> just because OnDownloadDestroyed() is called.

Hmmm.  Then I think the comment's a little confusing; when I read "We should
have caught OnDownloadDestroyed()" I read that as "but we didn't".  Is what
you're trying to say "The item was destroyed before the callback from the DB
signaling successful addition arrived.  We should leave it in the DB unless it
was also removed, in which case it will be in removed_while_adding_."?  Do you
consider that clearer?  If not, I'm happy with something indicating that our
catching OnDownloadDestroyed() is what really happened.

, though I'm having a hard enough time writing a less confusing comment that
maybe I should leave well enough alone.  

> Thanks for making me take another look at this code. The removed_while_adding_
> stuff did not depend on !item, so I moved it up a couple lines.

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.cc:326: if (item->GetState() ==
DownloadItem::IN_PROGRESS) {
On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> On 2012/08/15 20:47:44, rdsmith wrote:
> > Can they be opened before they're completed?  I.e. is this test necessary?
> 
> Wasn't there some chatter about streaming downloads to applications just the
> other day?
> Either way, feels like an implementation detail.

Fair enough.  We're well short of streaming data to applications, though.

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

http://codereview.chromium.org/10665049/diff/18004/chrome/browser/download/do...
chrome/browser/download/download_history.h:61: // ownership of |callback|.
On 2012/08/17 18:20:45, benjhayden_chromium wrote:
> On 2012/08/15 20:47:44, rdsmith wrote:
> > Not this CL's problem, but: "Takes ownership of |callback|"?  What does that
> > mean?  a) callbacks are values, and copyable, and b) it's a const reference.

> Do
> > you have any idea what's going on here?
> 
> I think it meant that it copied the callback to an internal map from
> HistoryService::Handle to the callback. But that's an implementation detail,
so
> I deleted the comment.

Thanks.

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:5: // DownloadHistory observes a
single DownloadManager and all its DownloadItems.
Very good; thanks.  Minor tweak: Can you start with "DownloadHistory manages
persisting DownloadItems to the history service by observing ...".  I think of
starting with the highest level view and drilling down being the best form of
documentation (gives people context) and the purpose of the class is the highest
level view.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:20: // remember to remove the item
when its ItemAdded() callback is called.
Destruction context so that we can reason about relative lifetimes?

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:23: // the same thing.
This probably should be done in another CL, but if we've got another mechanism
than CancelableRequestConsumer to guarantee lifetime issues, can we eliminate
CancelableRequestConsumer?  I think you and I are both more comfortable with the
weak pointer guarantees.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:87: if (history_.get())
Can you think of any contexts in which a DownloadItem could outlive the
DownloadHistory?  The DownloadHistory is tied to the lifetime of the manager,
and by the time the manager is destroyed, all of the DownloadItems on it should
already have been destroyed.

I'm happy with this being a DCHECK, but relative lifetimes strike me as part of
the knowledge this code should have (i.e. not an implementation detail of other
pieces of code) and if those lifetimes say that all the DIs should be destroyed
before the DH is destroyed, let's DCHECK that.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:207:
history_->OnDownloadHistoryDestroyed();
Why isn't this just a scoped_ptr<>?

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:208: for (ItemSet::const_iterator
iter = observing_items_.begin();
Shouldn't this list always be null at this point, because we've already torn
down the DownloadManager?  What's the destruction order?

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:274: if (!loading_ &&
Suggestion: Reverse the sense of the tests and return?

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:343: MaybeAddToHistory(item);
Under what circumstances would we actually want to add a download item to the
history through OnDownloadUpdated()?  It seems like the download would always be
created first, and hence always be tossed at the history through
OnDownloadCreated().  (Same comment for OnDownloadOpened()).

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:399: if (dhd == NULL)
How could this happen?

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.h:23: // go dig for the code before
relying on it.
I understand why you feel this way, but I think this isn't a reasonable set of
instructions--it would make software engineering impossible if you had to grok
the code of all producer classes before relying on them.  If it's part of the
interface that is relied on it should be documented (and ideally unit tested,
but I'm not sure I see a way to unit test this).  If it's not part of the
interface, it shouldn't be relied on.  

I think this makes me a conservative software engineer, but I'm not sure :-}.

Also, I was asking you to document the lifetime guarantees that
HistoryServiceDownloadAdapter provides.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.h:55: // item's profile or manager or
expose this from anywhere.
What is this used for?  

(If the IsTemporary() rewrite, I'd much rather it went into that CL than this
one--plans change regularly and semi-randomly, and with rare exceptions putting
things in anticipatorially (sic) over time results in dead code that rots.  It's
also easier to track purposes in the CL/revision record if it goes in with the
rest of the feature, and/or in association with the bug that describes the
feature.)

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

http://codereview.chromium.org/10665049/diff/9065/content/browser/download/do...
content/browser/download/download_item_impl.cc:129: const int
DownloadItem::kUninitializedHandle = 0;
Shouldn't this be nuked as well?

[benjhayden, 2012-08-24 15:32:15]

Still working on the test failures, but feel free to take another look.

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:5: // DownloadHistory observes a
single DownloadManager and all its DownloadItems.
On 2012/08/20 18:57:04, rdsmith wrote:
> Very good; thanks.  Minor tweak: Can you start with "DownloadHistory manages
> persisting DownloadItems to the history service by observing ...".  I think of
> starting with the highest level view and drilling down being the best form of
> documentation (gives people context) and the purpose of the class is the
highest
> level view.

Done.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:20: // remember to remove the item
when its ItemAdded() callback is called.
On 2012/08/20 18:57:04, rdsmith wrote:
> Destruction context so that we can reason about relative lifetimes?

Done.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:23: // the same thing.
On 2012/08/20 18:57:04, rdsmith wrote:
> This probably should be done in another CL, but if we've got another mechanism
> than CancelableRequestConsumer to guarantee lifetime issues, can we eliminate
> CancelableRequestConsumer?  I think you and I are both more comfortable with
the
> weak pointer guarantees.

It required changing our HistoryService/HistoryBackend methods, so I don't know
how the owner of those classes will feel, but I like it.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:87: if (history_.get())
On 2012/08/20 18:57:04, rdsmith wrote:
> Can you think of any contexts in which a DownloadItem could outlive the
> DownloadHistory?  The DownloadHistory is tied to the lifetime of the manager,
> and by the time the manager is destroyed, all of the DownloadItems on it
should
> already have been destroyed.
> 
> I'm happy with this being a DCHECK, but relative lifetimes strike me as part
of
> the knowledge this code should have (i.e. not an implementation detail of
other
> pieces of code) and if those lifetimes say that all the DIs should be
destroyed
> before the DH is destroyed, let's DCHECK that.

This was part of DownloadHistory::Disable, which was speculative and so removed.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:207:
history_->OnDownloadHistoryDestroyed();
On 2012/08/20 18:57:04, rdsmith wrote:
> Why isn't this just a scoped_ptr<>?

Done.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:208: for (ItemSet::const_iterator
iter = observing_items_.begin();
On 2012/08/20 18:57:04, rdsmith wrote:
> Shouldn't this list always be null at this point, because we've already torn
> down the DownloadManager?  What's the destruction order?

Yes, DownloadManager::Shutdown() deletes the items before calling
delegate->Shutdown(), which is ChromeDownloadManagerDelegate::Shutdown(), which
deletes the DownloadHistory, but this seems like an implementation detail.
Also, this code will evaporate when HyperbolicDownloadItemObserver (or
WeakObserverList, whichever y'all like) is committed.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:274: if (!loading_ &&
On 2012/08/20 18:57:04, rdsmith wrote:
> Suggestion: Reverse the sense of the tests and return?

Done.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:343: MaybeAddToHistory(item);
On 2012/08/20 18:57:04, rdsmith wrote:
> Under what circumstances would we actually want to add a download item to the
> history through OnDownloadUpdated()?  It seems like the download would always
be
> created first, and hence always be tossed at the history through
> OnDownloadCreated().  (Same comment for OnDownloadOpened()).

Transition from Temporary to not Temporary. (In the future, read Temporary as
"disabled".)

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:399: if (dhd == NULL)
On 2012/08/20 18:57:04, rdsmith wrote:
> How could this happen?

Done.

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.h:23: // go dig for the code before
relying on it.
On 2012/08/20 18:57:04, rdsmith wrote:
> I understand why you feel this way, but I think this isn't a reasonable set of
> instructions--it would make software engineering impossible if you had to grok
> the code of all producer classes before relying on them.  If it's part of the
> interface that is relied on it should be documented (and ideally unit tested,
> but I'm not sure I see a way to unit test this).  If it's not part of the
> interface, it shouldn't be relied on.  
> 
> I think this makes me a conservative software engineer, but I'm not sure :-}.
> 
> Also, I was asking you to document the lifetime guarantees that
> HistoryServiceDownloadAdapter provides.  

Done.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.h:55: // item's profile or manager or
expose this from anywhere.
On 2012/08/20 18:57:04, rdsmith wrote:
> What is this used for?  
> 
> (If the IsTemporary() rewrite, I'd much rather it went into that CL than this
> one--plans change regularly and semi-randomly, and with rare exceptions
putting
> things in anticipatorially (sic) over time results in dead code that rots. 
It's
> also easier to track purposes in the CL/revision record if it goes in with the
> rest of the feature, and/or in association with the bug that describes the
> feature.)

Done.

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

http://codereview.chromium.org/10665049/diff/9065/content/browser/download/do...
content/browser/download/download_item_impl.cc:129: const int
DownloadItem::kUninitializedHandle = 0;
On 2012/08/20 18:57:04, rdsmith wrote:
> Shouldn't this be nuked as well?

Done.

[Randy Smith (Not in Mondays), 2012-08-27 18:25:11]

Partial comments; still reviewing, but I'm heading out for the afternoon, so
giving something to chew on.

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:208: for (ItemSet::const_iterator
iter = observing_items_.begin();
On 2012/08/24 15:32:15, benjhayden_chromium wrote:
> On 2012/08/20 18:57:04, rdsmith wrote:
> > Shouldn't this list always be null at this point, because we've already torn
> > down the DownloadManager?  What's the destruction order?
> 
> Yes, DownloadManager::Shutdown() deletes the items before calling
> delegate->Shutdown(), which is ChromeDownloadManagerDelegate::Shutdown(),
which
> deletes the DownloadHistory, but this seems like an implementation detail.

Hmmm.  Ok.  What I hear you saying here is that DownloadHistory implemented as
an observer should be independent of the lifetime of the class it observers.  If
that's a consistent design principle in this code, I'm good with it.  I'm
tempted to ask you to test it, but that's probably overkill :-}.

> Also, this code will evaporate when HyperbolicDownloadItemObserver (or
> WeakObserverList, whichever y'all like) is committed.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:343: MaybeAddToHistory(item);
On 2012/08/24 15:32:15, benjhayden_chromium wrote:
> On 2012/08/20 18:57:04, rdsmith wrote:
> > Under what circumstances would we actually want to add a download item to
the
> > history through OnDownloadUpdated()?  It seems like the download would
always
> be
> > created first, and hence always be tossed at the history through
> > OnDownloadCreated().  (Same comment for OnDownloadOpened()).
> 
> Transition from Temporary to not Temporary. (In the future, read Temporary as
> "disabled".)

Yuck; I had adding state transitions for transition options I don't expect to
ever happen.  But I guess it's not particularly clean at the history level to
specify this as something set at the beginning of the download item and not
changing; since it's in SupportsUserData::Data, it can't be a constructor
argument.  Ok.  

If the code's in here, you should probably have a test for it, otherwise it'll
rot.  (Double yuck--testing code that I don't expect to ever used.)

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

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:77: bool is_persisted() const {
return db_handle_ > kUninitializedHandle; }
As noted elsewhere, I'd sorta like download_database.* to be the authoritative
source of db_handles (actually, I don't care where it is, I just want it to be
in one place, and there's a lot of tricky code about it in download_database.cc
already).  So the implicit knowledge of the namespace in this (that we allocate
values above kUninitializedHandle rather than below) seems wrong to have present
in this file.  Can we do != kUninitializedHandle?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:123: // CancelableRequestConsumer
would normally ensure that callbacks from the
How about "The HistoryService normally uses CancelableRequestConsumer to ..."? 
I got confused because we aren't using CancelableRequestConsumer anymore so I
wasn't sure why we were refering to it.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:125: // DownloadHistory has opted
for less magic, so it uses Ensure*OnUI to
"... less magic by relying on WeakPtrs instead ..."?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:127: // creating WeakPtrs on the
wrong thread.
I don't understand the weak pointer reference in this context?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:233: // DownloadItem while
QueryCallback() is processing the database.
This is actually not strictly speaking true, or at least it won't be in the
future, though I'm not sure what to do about it.  If there's some other observer
on the DownloadManager that hooks OnDownloadCreated, and it calls the
programmatic download interface from OnDownloadCreated, and we change that
interface to create the download item right away and link it up with the request
when the request comes in a la download resumption (this is the "in the future"
part), we could have an extraneous OnDownloadCreated() showing up.    

I suppose we could tell the difference by whether the download item is being in
created in a terminal state (which indicates that it's coming from the history)
or IN_PROGRESS (which is pretty much all other downloads).  Or we could just
take a sledgehammer to it, and make loading_ store the list of downloads created
off to the side, and then do a set-difference against the ones we create here. 
Or we could ignore it for now.  I'm not sure what the right thing is.  

I think what I'll say is: If you want to ignore this for now, put a note next to
"OnDownloadCreated" saying that you're not allowed to call the programmatic
download initiation or CreateDownloadItem from that notification to allow
embedders to reason about the relationship between creating a download item and
receiving the callback.  Maybe with a "TODO()" to fix embedders so they don't
make that assumption :-} :-J.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:317: // have been removed from
history, then t would have also called
nit: t -> it

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

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.h:24: // down before HistoryService is
destroyed.
Hmmm.  I'd either like this entire system to not rely on lifetime relationships
with the download system, or to explicitly rely on the lifetime relationship and
not have any code (other than DCHECKs) to handle violations of the expected
lifetime relationship.  It seems like this is explicitly relying on the lifetime
relationship, and ~DownloadHistory has code to handle the download system not
being destroyed when it goes down.  Pick a direction and regularize?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.h:25: class
HistoryServiceDownloadAdapter {
I'm not sure whether this makes sense or not--this interface is probably much
simpler--but given the lack of lifetime guarantees, couldn't you get rid of this
class entirely and just mock HistoryService in tests?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:108: next_db_handle_(0) {
So this file is the source of db handles, and manages the namespace.  It's got a
special case for a db handle of zero, and it defines the allocation of db
handles by incrementing each time.  There's trickiness around how to assign db
handles before QueryDownloads.  My point is that there's actually a fair amount
of subtlty about the db_handle namespace that's embodied in the code of this
file.  Given that, I don't think it's a good idea to have the unique value
kUninitializedDbHandle defined in download_history.*; it really belongs here. 
Does that make sense?

[Randy Smith (Not in Mondays), 2012-08-27 19:28:19]

Rest of download_history* comments.  Going on to rest of CL :-}.

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

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:23: // the same thing.
On 2012/08/24 15:32:15, benjhayden_chromium wrote:
> On 2012/08/20 18:57:04, rdsmith wrote:
> > This probably should be done in another CL, but if we've got another
mechanism
> > than CancelableRequestConsumer to guarantee lifetime issues, can we
eliminate
> > CancelableRequestConsumer?  I think you and I are both more comfortable with
> the
> > weak pointer guarantees.
> 
> It required changing our HistoryService/HistoryBackend methods, so I don't
know
> how the owner of those classes will feel, but I like it.

I didn't follow what you meant here when I first read it, but now I do, and I
wince a bit.  We're effectively driving a separate functionality stake through
HistoryService in order not to use CancelableRequestConsumer.  But it would be
nice to avoid using CancelableRequestConsumer.  So yes, see what the owner says.
 I'd recommend Brett--I think he wrote that code and knows it best.

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:171:
history_service_->UpdateDownload(info);
So the fact that this does not take a db handle indicates that we duplicate the
db handle between DownloadHistoryData and DownloadPersistentStateInfo.  My
initial reaction was that we could remove it from DHD, but that obviously won't
work because we don't always have DPSI.  But can we remove it from DPSI?  That'd
require a bit of re-plumbing, but the db handle is arguably a control parameter
rather than actually part of the actual persistent storage info.

I don't feel strongly about this, I just think it would be cleaner without the
duplicate info.  If you don't dedup it, could you put a comment next to
db_handle in DHD to explain why it's duplicated?

http://codereview.chromium.org/10665049/diff/9065/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:343: MaybeAddToHistory(item);
On 2012/08/24 15:32:15, benjhayden_chromium wrote:
> On 2012/08/20 18:57:04, rdsmith wrote:
> > Under what circumstances would we actually want to add a download item to
the
> > history through OnDownloadUpdated()?  It seems like the download would
always
> be
> > created first, and hence always be tossed at the history through
> > OnDownloadCreated().  (Same comment for OnDownloadOpened()).
> 
> Transition from Temporary to not Temporary. (In the future, read Temporary as
> "disabled".)

Also: You have the same call to MaybeAddToHistory() in OnDownloadOpened(). 
Shouldn't we always get an OnDownloadUpdated() before an OnDownloadOpened(),
even in the ->!Temporary case?  Heck, the transition to disabled happens through
us--I hope when we get there that we can remove all of these, and just persist
it when we get the call to make it non-disabled.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/his...
chrome/browser/history/history.h:94: // thread that made the request.
I'd modify this comment to say "Each request callback that makes use of the
CancelableRequestConsumer interface is invoked on the thread that made the
request."

[Randy Smith (Not in Mondays), 2012-08-28 13:11:49]

Non-test comments; will review tests when you've finished your work on them.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:259: for
(std::set<DownloadID>::const_iterator it = handles.begin();
I remember that there's some reason why this isn't a problem performance wise,
even when there are a lot of handles, but I don't remember what it is.  Could
you put that reason into a comment here?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:280: if (num_downloads_not_deleted
>= 0) {
I could be wrong, but I'm not certain you want this conditional, and possibly we
could remove the first conditional as well.  

In terms of the first condition, I think of its goal as being to leave out cases
in which the RemoveDownloadsBetween named an interval with no downloads, but I'm
not sure we'll arrive in this code in that case?  And thinking about it, I'm not
sure why we wouldn't want to record that case.  

And in terms of the second conditional, isn't ndnd == 0 (i.e. we just wiped the
DB) one of the most important cases to capture?

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_item_gtk.cc:471: if
(icon_filepath_.empty()) return;
Is this really part of this CL?

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/b...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/b...
content/browser/download/base_file.cc:268: if (detached_) return
net::ERR_INVALID_HANDLE;
Yikes.  This seems like a very big semantic change for BaseFile.  Why?  And why
in this CL?

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
File content/browser/download/download_item_factory.h (right):

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_item_factory.h:40: virtual DownloadItemImpl*
CreatePersistedItem(
We need a better name for this.  Completed item?  "Finished" might be better,
since there can be cancelled/interrupted items in this list.

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

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:483: NotifyModelChanged();
Why are we doing a NotifyModelChanged() at the DMI level?  Nothing at the DMI
level has changed, only things at the DII level.  Or am I missing something?

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:485:
MaybeCompleteDownload(item);
Why do this?  We had a linear execution order before, and now we're racing with
the intermediate file rename.  It's probably ok, but why not leave it linear?

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:901: return
GetDownload(download_id);
How did you assure yourself that this change in behavior was safe?

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

http://codereview.chromium.org/10665049/diff/38003/content/public/test/downlo...
content/public/test/download_test_observer.cc:137:
downloads_observed_.insert(item);
Why the addition?  I don't mind changing over, but I think of
OnDownloadCreated() as being the new method we're replacing ModelChanged() with,
so it's weird to see an observer using OnDownloadCreated() and still using
ModelChanged().

http://codereview.chromium.org/10665049/diff/34011/content/public/test/mock_d...
File content/public/test/mock_download_manager.h (left):

http://codereview.chromium.org/10665049/diff/34011/content/public/test/mock_d...
content/public/test/mock_download_manager.h:67:
MOCK_METHOD2(OnItemAddedToPersistentStore, void(int32 download_id,
Shouldn't this be removed too?

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

http://codereview.chromium.org/10665049/diff/30009/content/browser/download/d...
content/browser/download/download_manager_impl.cc:485:
MaybeCompleteDownload(item);
Why is this here?  If I'm tracing out the new control flow properly, in the
!SavePageData case you'll get to this in DMI::DownloadRenamedToIntermediateName.

[benjhayden, 2012-08-30 13:34:57]

Timing RemoveDownloads() on linux:
time to add 100,000 records: 3847.01 ms
time to remove 100,000 records: 766.267 ms
Deleting 1 record at a time costs about 1/5 what adding a record costs.

If we get complaints about delete taking a while, I can think of ways to
ammortize the time. I'm hesitant to add complexity like that without user
complaints, though.

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

On 2012/08/30 13:34:57, benjhayden_chromium wrote:
> Timing RemoveDownloads() on linux:
> time to add 100,000 records: 3847.01 ms
> time to remove 100,000 records: 766.267 ms
> Deleting 1 record at a time costs about 1/5 what adding a record costs.
> 
> If we get complaints about delete taking a while, I can think of ways to
> ammortize the time. I'm hesitant to add complexity like that without user
> complaints, though.

I'm good with these numbers.  I was mostly looking for a comment at the point
where we're doing the deletes one at a time explaining why we're doing it that
way--probably just saying that in tests, deleting 100,000 records one at a time
took less than a second is fine.

[benjhayden, 2012-09-10 19:02:56]

I think the reason that this CL doesn't have any CC is because the first comment
on it wasn't from me, so please hold review until I move it to another issue
number and mail it out the normal way.

This CL will wait for GetAllDownloads() http://codereview.chromium.org/10913015/
and two up-coming CLs to remove GetDownloadItem(), GetActiveDownloadItem(),
SearchDownloads() and MatchesQuery() to stick.

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

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:77: bool is_persisted() const {
return db_handle_ > kUninitializedHandle; }
On 2012/08/27 18:25:11, rdsmith wrote:
> As noted elsewhere, I'd sorta like download_database.* to be the authoritative
> source of db_handles (actually, I don't care where it is, I just want it to be
> in one place, and there's a lot of tricky code about it in
download_database.cc
> already).  So the implicit knowledge of the namespace in this (that we
allocate
> values above kUninitializedHandle rather than below) seems wrong to have
present
> in this file.  Can we do != kUninitializedHandle?

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:123: // CancelableRequestConsumer
would normally ensure that callbacks from the
On 2012/08/27 18:25:11, rdsmith wrote:
> How about "The HistoryService normally uses CancelableRequestConsumer to ..."?

> I got confused because we aren't using CancelableRequestConsumer anymore so I
> wasn't sure why we were refering to it.

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:125: // DownloadHistory has opted
for less magic, so it uses Ensure*OnUI to
On 2012/08/27 18:25:11, rdsmith wrote:
> "... less magic by relying on WeakPtrs instead ..."?

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:127: // creating WeakPtrs on the
wrong thread.
On 2012/08/27 18:25:11, rdsmith wrote:
> I don't understand the weak pointer reference in this context?

If these functions were methods on DownloadHistory, then they might need to
create WeakPtrs for the posted callback, but you can't create WeakPtrs on a
thread other than the thread that the WeakPtrFactory was created on. So these
functions need to be static.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:233: // DownloadItem while
QueryCallback() is processing the database.
On 2012/08/27 18:25:11, rdsmith wrote:
> This is actually not strictly speaking true, or at least it won't be in the
> future, though I'm not sure what to do about it.  If there's some other
observer
> on the DownloadManager that hooks OnDownloadCreated, and it calls the
> programmatic download interface from OnDownloadCreated, and we change that
> interface to create the download item right away and link it up with the
request
> when the request comes in a la download resumption (this is the "in the
future"
> part), we could have an extraneous OnDownloadCreated() showing up.    
> 
> I suppose we could tell the difference by whether the download item is being
in
> created in a terminal state (which indicates that it's coming from the
history)
> or IN_PROGRESS (which is pretty much all other downloads).  Or we could just
> take a sledgehammer to it, and make loading_ store the list of downloads
created
> off to the side, and then do a set-difference against the ones we create here.

> Or we could ignore it for now.  I'm not sure what the right thing is.  
> 
> I think what I'll say is: If you want to ignore this for now, put a note next
to
> "OnDownloadCreated" saying that you're not allowed to call the programmatic
> download initiation or CreateDownloadItem from that notification to allow
> embedders to reason about the relationship between creating a download item
and
> receiving the callback.  Maybe with a "TODO()" to fix embedders so they don't
> make that assumption :-} :-J.

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.cc:317: // have been removed from
history, then t would have also called
On 2012/08/27 18:25:11, rdsmith wrote:
> nit: t -> it

Done.

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

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.h:24: // down before HistoryService is
destroyed.
On 2012/08/27 18:25:11, rdsmith wrote:
> Hmmm.  I'd either like this entire system to not rely on lifetime
relationships
> with the download system, or to explicitly rely on the lifetime relationship
and
> not have any code (other than DCHECKs) to handle violations of the expected
> lifetime relationship.  It seems like this is explicitly relying on the
lifetime
> relationship, and ~DownloadHistory has code to handle the download system not
> being destroyed when it goes down.  Pick a direction and regularize?

It looks like you're talking about two different lifetime relationships:
HistoryService <> DownloadHistory and DownloadHistory <> DownloadManager.

DownloadHistory relies on HistoryService not disappearing because there's
nothing that it can do to detect HistoryService destruction. I suppose I could
make a virtual method on DownloadHistory that gets the HistoryService for this
profile every time it goes to call the HistoryService, and all the callers could
just bail if the HistoryService was destroyed. I'd be happy to do that, but I
thought you wanted to get away from that pattern.

DownloadHistory doesn't need to rely on outliving DM or DIs because there are
ways to deal with DM/DI destruction. It might as well use these mechanisms in
case our refactoring extends to ChromeDownloadManagerDelegate, and we decide to
do something slightly different with the CDMD<>DH relationship. It's just
defensive programming.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/download/do...
chrome/browser/download/download_history.h:25: class
HistoryServiceDownloadAdapter {
On 2012/08/27 18:25:11, rdsmith wrote:
> I'm not sure whether this makes sense or not--this interface is probably much
> simpler--but given the lack of lifetime guarantees, couldn't you get rid of
this
> class entirely and just mock HistoryService in tests?

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:108: next_db_handle_(0) {
On 2012/08/27 18:25:11, rdsmith wrote:
> So this file is the source of db handles, and manages the namespace.  It's got
a
> special case for a db handle of zero, and it defines the allocation of db
> handles by incrementing each time.  There's trickiness around how to assign db
> handles before QueryDownloads.  My point is that there's actually a fair
amount
> of subtlty about the db_handle namespace that's embodied in the code of this
> file.  Given that, I don't think it's a good idea to have the unique value
> kUninitializedDbHandle defined in download_history.*; it really belongs here. 
> Does that make sense?

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:259: for
(std::set<DownloadID>::const_iterator it = handles.begin();
On 2012/08/28 13:11:49, rdsmith wrote:
> I remember that there's some reason why this isn't a problem performance wise,
> even when there are a lot of handles, but I don't remember what it is.  Could
> you put that reason into a comment here?

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:280: if (num_downloads_not_deleted
>= 0) {
On 2012/08/28 13:11:49, rdsmith wrote:
> I could be wrong, but I'm not certain you want this conditional, and possibly
we
> could remove the first conditional as well.  
> 
> In terms of the first condition, I think of its goal as being to leave out
cases
> in which the RemoveDownloadsBetween named an interval with no downloads, but
I'm
> not sure we'll arrive in this code in that case?  And thinking about it, I'm
not
> sure why we wouldn't want to record that case.  
> 
> And in terms of the second conditional, isn't ndnd == 0 (i.e. we just wiped
the
> DB) one of the most important cases to capture?

That's not what ndnd means.

ndnd = num_downloads_asked_to_delete - num_downloads_actually_deleted.

The conditional throws it out if ndnd is negative, ie. we deleted *more* records
than we meant to delete.
I'll remove the conditional if you're sure that UMA_HISTOGRAM_COUNTS is ok with
negative numbers. I couldn't tell.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/history/his...
chrome/browser/history/history.h:94: // thread that made the request.
On 2012/08/27 19:28:19, rdsmith wrote:
> I'd modify this comment to say "Each request callback that makes use of the
> CancelableRequestConsumer interface is invoked on the thread that made the
> request."

Done.

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_item_gtk.cc (right):

http://codereview.chromium.org/10665049/diff/38003/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_item_gtk.cc:471: if
(icon_filepath_.empty()) return;
On 2012/08/28 13:11:49, rdsmith wrote:
> Is this really part of this CL?

Done.

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
File content/browser/download/download_item_factory.h (right):

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_item_factory.h:40: virtual DownloadItemImpl*
CreatePersistedItem(
On 2012/08/28 13:11:49, rdsmith wrote:
> We need a better name for this.  Completed item?  "Finished" might be better,
> since there can be cancelled/interrupted items in this list.

There can also be IN_PROGRESS entries in the history, which DII rewrites as
CANCELED.

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

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:483: NotifyModelChanged();
On 2012/08/28 13:11:49, rdsmith wrote:
> Why are we doing a NotifyModelChanged() at the DMI level?  Nothing at the DMI
> level has changed, only things at the DII level.  Or am I missing something?

This was moved from AddDownloadItemToHistory().

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:485:
MaybeCompleteDownload(item);
On 2012/08/28 13:11:49, rdsmith wrote:
> Why do this?  We had a linear execution order before, and now we're racing
with
> the intermediate file rename.  It's probably ok, but why not leave it linear?

This was moved from OnSavePageItemAddedToPersistentStore, but you're right it
doesn't look necessary.

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

http://codereview.chromium.org/10665049/diff/38003/content/public/test/downlo...
content/public/test/download_test_observer.cc:137:
downloads_observed_.insert(item);
On 2012/08/28 13:11:49, rdsmith wrote:
> Why the addition?  I don't mind changing over, but I think of
> OnDownloadCreated() as being the new method we're replacing ModelChanged()
with,
> so it's weird to see an observer using OnDownloadCreated() and still using
> ModelChanged().

Done.

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

http://codereview.chromium.org/10665049/diff/30009/content/browser/download/d...
content/browser/download/download_manager_impl.cc:485:
MaybeCompleteDownload(item);
On 2012/08/28 13:11:49, rdsmith wrote:
> Why is this here?  If I'm tracing out the new control flow properly, in the
> !SavePageData case you'll get to this in
DMI::DownloadRenamedToIntermediateName.

I had moved it from OnDownloadItemAddedToPersistentStore, but you're right, it
doesn't appear necessary here.

[Randy Smith (Not in Mondays), 2012-09-11 16:30:34]

Responses to comments on old CL; switching over to new CL now.

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
File content/browser/download/download_item_factory.h (right):

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_item_factory.h:40: virtual DownloadItemImpl*
CreatePersistedItem(
On 2012/09/10 19:02:56, benjhayden_chromium wrote:
> On 2012/08/28 13:11:49, rdsmith wrote:
> > We need a better name for this.  Completed item?  "Finished" might be
better,
> > since there can be cancelled/interrupted items in this list.
> 
> There can also be IN_PROGRESS entries in the history, which DII rewrites as
> CANCELED.

They still end up being finished when they're put into the core downloads system
through this interface.  And I think of the IN_PROGRESS vs. finished distinction
as the key one here--we're not hooking the download item up to a file &
URLRequest, we're just creating it to hang out by itself.  This name makes the
concept of persisting part of the interface.

But I don't care a whole lot; you can keep it like this if you want.

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

http://codereview.chromium.org/10665049/diff/38003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:483: NotifyModelChanged();
On 2012/09/10 19:02:56, benjhayden_chromium wrote:
> On 2012/08/28 13:11:49, rdsmith wrote:
> > Why are we doing a NotifyModelChanged() at the DMI level?  Nothing at the
DMI
> > level has changed, only things at the DII level.  Or am I missing something?
> 
> This was moved from AddDownloadItemToHistory().

Ok.  I think that what this is about then is the fact that we didn't
historically make downloads visible until after they were persisted.  So you
might want to consider removing this as part of one of your other CLs, since
we're changing that behavior.