Add new UMA stats to get a handle on Downloads UI Usage

Add new UMA stats to get a handle on Downloads UI Usage

Download.OpenTime: number of milliseconds between a download completing and when it is opened 

Download.FirstOpenTime: same, but is only counted the first time that a download is opened (even across restarts) 

Download.HistorySize: number of items in the history when a download begins. A user who downloads 5 times per minute, Clears All, then repeats will look exactly like a user who downloads 5 times per week then Clears All and repeats, which is a flat line that goes to 5 then stops. People like me who Clear All after every download will be a dirac delta in the 0 bucket, whereas people who never Clear All will be a flat line all the way out. This should look more like an exponential distribution, with bumps at db sizes when chrome updated. The expected value of this distribution will be the most interesting number.

Download.ShelfSizeOnAutoClose: number of items displayed in the shelf when it autocloses

Download.ShelfSizeOnUserClose: same as ShelfSizeOnAutoClose, but either when the user opens chrome://downloads or clicks the shelf's |x|.

Download.ShelfInProgressSizeOnAutoClose: same as ShelfSizeOnAutoClose, but only counts in-progress items.

Download.ShelfInProgressSizeOnUserClose: same as ShelfSizeOnUserClose, but only counts in-progress items.

Download.ClearAllSize: number of items removed by 'Clear All'

Download.OpensOutstanding: number of completed unopened items when an item is opened.

FirstOpenTime required adding |end_time| and |opened| to the downloads table.
ScheduleAndForget only goes up to 4 args, so I changed UpdateEntry to use DownloadPersistentStoreInfo.

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

[benjhayden, 2011-09-23 21:22:58]

Randy, Asanka, feel free to take a look at these UMA. I'm particularly proud of
HistorySize2.

I think I'd need to add a flag to the DownloadShelf implementations to
distinguish between opening chrome://downloads and clicking the |x|, so LMK if I
should do that.

[Randy Smith (Not in Mondays), 2011-09-26 18:27:25]

I do like HistorySize2.  I have expectations as to what the result will look
like, but I'm very curious about seeing it and think I'll learn something
important from it, so that makes it a good UMA stat :-}.  Maybe (random thought)
we could also have a DownloadOpensOutstanding (number of completed, unopened
downloads when a download is opened) to try and distinguish between inline
processors and batch processors (both of which I could imagine never click clear
all downloads)?

A couple of high level comments:

+ You want the CL description to start with an (ideally) single-line description
that summarizes the entire CL.  Something like "Add new UMA stats to get a
handle on Downloads UI Usage".  I'm not wedded to wording or content, but we
should have that intro sentence.

+ Are you comfortable you know the standard chrome meta-pattern, if there is
one, for doing on-disk database changes?  I'd feel better with this CL if
someone who was familiar with that (I'm not) looked at the upgrade code.  Grab
me if you need help finding such a person.  shess@ would probably do, though I
don't know that he knows that stuff--I just know he knows SQLite.

+ I'm uncomfortable with the creation of a new entry in the delegate.  Is there
any reason not to stick with (what I think of as) the current pattern where you
just call UpdateItemInPersistentStore?  (I know we have
UpdatePathForItemInPersistentStore, but maybe we can get rid of that?)  I'm
being a bit biased in this question, as my mind has already jumped ahead to the
particular castle in the air where the history system is just an observer of the
DownloadManager, so all it'd get would be a DownloadItem pointer.  But no reason
to move further away from there if that's where we want to get to.

I'll wait on resolution of these issues before doing a detailed review.

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
chrome/browser/history/download_database.cc:117: int version = schema_version_;
Does initializing version do anything here?  It looks like it's overwritten on
all paths.  Why not just use schema_version_ directly?

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:258: void
RRemoveDownloademoveDownloads(const base::Time remove_end);
???

[asanka, 2011-09-26 21:54:41]

Looks great!

Do we need to worry about noise in Download.FirstOpenTime due to database
migrations?  Probably not too much.

I agree that HistorySize2 is very interesting.  I wonder if there would be a
steady state relationship between HistorySize2 and ClearAllSize where
HistorySize2[n] = Sum(ClearAllSize[m] for m > n).

For ShelfSize, do we (or could we) have a statistic for how many downloads are
in progress when the shelf is closed?  I'm trying to get a feel for how many
people don't care about monitoring progress.  My guess is these are people who
don't find the shelf to be very useful.

On 2011/09/23 21:22:58, b s h wrote:
> Randy, Asanka, feel free to take a look at these UMA. I'm particularly proud
of
> HistorySize2.
> 
> I think I'd need to add a flag to the DownloadShelf implementations to
> distinguish between opening chrome://downloads and clicking the |x|, so LMK if
I
> should do that.

[benjhayden, 2011-09-27 15:03:58]

Hold the review, still working, just wanted to get these ideas down before they
evaporate.

I just realized that there's a race condition around HistorySize2, so I'm moving
it to after the download was entered in the db and history_downloads_. This will
also prevent recording a ton of bogus zeros for otr downloads.


On 2011/09/26 21:54:41, asanka wrote:
> Looks great!
> 
> Do we need to worry about noise in Download.FirstOpenTime due to database
> migrations?  Probably not too much.

If a user completes a download, upgrades, then opens the download, then
|end_time| will be null so download_stats::RecordOpen won't record it. All of
the downloads that show up in FirstOpenTime will have been completed after the
db upgrade.

> 
> I agree that HistorySize2 is very interesting.  I wonder if there would be a
> steady state relationship between HistorySize2 and ClearAllSize where
> HistorySize2[n] = Sum(ClearAllSize[m] for m > n).

Yes, but the "steady state" would be when *everybody* had just Cleared All,
which some people never do.

> 
> For ShelfSize, do we (or could we) have a statistic for how many downloads are
> in progress when the shelf is closed?  I'm trying to get a feel for how many
> people don't care about monitoring progress.  My guess is these are people who
> don't find the shelf to be very useful.

Done.

[benjhayden, 2011-09-27 17:34:38]

Still working, hold review.


On 2011/09/26 18:27:25, rdsmith wrote:
> I do like HistorySize2.  I have expectations as to what the result will look
> like, but I'm very curious about seeing it and think I'll learn something
> important from it, so that makes it a good UMA stat :-}.  Maybe (random
thought)
> we could also have a DownloadOpensOutstanding (number of completed, unopened
> downloads when a download is opened) to try and distinguish between inline
> processors and batch processors (both of which I could imagine never click
clear
> all downloads)?

I thought that FirstOpenTime would address inline vs. batch?
It seems like DownloadOpensOutstanding would be confused by the Clear All usage
spectrum and browser openers vs. OS openers. 

> 
> A couple of high level comments:
> 
> + You want the CL description to start with an (ideally) single-line
description
> that summarizes the entire CL.  Something like "Add new UMA stats to get a
> handle on Downloads UI Usage".  I'm not wedded to wording or content, but we
> should have that intro sentence.

Done.

> 
> + Are you comfortable you know the standard chrome meta-pattern, if there is
> one, for doing on-disk database changes?  I'd feel better with this CL if
> someone who was familiar with that (I'm not) looked at the upgrade code.  Grab
> me if you need help finding such a person.  shess@ would probably do, though I
> don't know that he knows that stuff--I just know he knows SQLite.

It looks like there's a shared major/minor version pair in the meta table that a
few tables use, but I can't convince myself that I can use it without breaking
something. visit_database.cc uses DoesColumnExist, so I'll use that.

> 
> + I'm uncomfortable with the creation of a new entry in the delegate.  Is
there
> any reason not to stick with (what I think of as) the current pattern where
you
> just call UpdateItemInPersistentStore?  (I know we have
> UpdatePathForItemInPersistentStore, but maybe we can get rid of that?)  I'm
> being a bit biased in this question, as my mind has already jumped ahead to
the
> particular castle in the air where the history system is just an observer of
the
> DownloadManager, so all it'd get would be a DownloadItem pointer.  But no
reason
> to move further away from there if that's where we want to get to.

Done.

> 
> I'll wait on resolution of these issues before doing a detailed review.
> 
>
http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
> File chrome/browser/history/download_database.cc (right):
> 
>
http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
> chrome/browser/history/download_database.cc:117: int version =
schema_version_;
> Does initializing version do anything here?  It looks like it's overwritten on
> all paths.  Why not just use schema_version_ directly?
> 
>
http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
> File chrome/browser/history/history_backend.h (right):
> 
>
http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
> chrome/browser/history/history_backend.h:258: void
> RRemoveDownloademoveDownloads(const base::Time remove_end);
> ???

[benjhayden, 2011-09-28 17:35:23]

PTAL

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/downl...
chrome/browser/history/download_database.cc:117: int version = schema_version_;
On 2011/09/26 18:27:26, rdsmith wrote:
> Does initializing version do anything here?  It looks like it's overwritten on
> all paths.  Why not just use schema_version_ directly?

Rewritten to use DoesColumnExist, which is easier and precedented.

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8008021/diff/7026/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:258: void
RRemoveDownloademoveDownloads(const base::Time remove_end);
On 2011/09/26 18:27:26, rdsmith wrote:
> ???

Done.

[Randy Smith (Not in Mondays), 2011-09-28 19:24:30]

On 2011/09/27 17:34:38, b s h wrote:
> Still working, hold review.
> 
> 
> On 2011/09/26 18:27:25, rdsmith wrote:
> > I do like HistorySize2.  I have expectations as to what the result will look
> > like, but I'm very curious about seeing it and think I'll learn something
> > important from it, so that makes it a good UMA stat :-}.  Maybe (random
> thought)
> > we could also have a DownloadOpensOutstanding (number of completed, unopened
> > downloads when a download is opened) to try and distinguish between inline
> > processors and batch processors (both of which I could imagine never click
> clear
> > all downloads)?
> 
> I thought that FirstOpenTime would address inline vs. batch?

Hmmm.  I don't really see that.  Inline vs. batch is about whether you have many
downloads occurring at once, rather than how long you wait between downloading a
file and opening it.  Maybe you got involved in a code review in between?  :-} 
So I'm looking for something that is a signal for how often people have multiple
downloads going at the same time, and how many.  

> It seems like DownloadOpensOutstanding would be confused by the Clear All
usage
> spectrum and browser openers vs. OS openers. 

Well, I guess that depends on what the Clear All usage spectrum *is*, so maybe
it's worthwhile putting this debate off until we have that information.  My
expectation is that there'll be a small subset of users that clear all all the
time, but that most users will basically not touch the control.  Browser openers
vs. OS openers is a good point; I guess what I'm interested in is how many other
elements are showing on the downloads shelf when you open one.  So you'd need to
nuke that count when the shelf goes away.  Or rely on the fact that OS openers
never open things from the shelf, so they won't contribute very much to this
statistic :-}.

[Randy Smith (Not in Mondays), 2011-09-28 19:41:55]

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
chrome/browser/history/history_types.h:745: struct DownloadItemData {
Is there a reason not to use DownloadPersistentStoreInfo?  It was originally
(six months ago :-}) intended to communication with the history DB and to
include everything that the DownloadItem gets from the history DB.

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
content/browser/download/download_manager.h:267: void
MarkDownloadOpened(DownloadItem* download);
I thought this was being removed?

[benjhayden, 2011-09-28 20:29:39]

Working on OpensOutstanding.

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
chrome/browser/history/history_types.h:745: struct DownloadItemData {
On 2011/09/28 19:41:55, rdsmith wrote:
> Is there a reason not to use DownloadPersistentStoreInfo?  It was originally
> (six months ago :-}) intended to communication with the history DB and to
> include everything that the DownloadItem gets from the history DB.

DownloadItemData isn't what the DownloadItem gets from the db, it's what the db
gets from the item as the item changes. The url, referrer, and start_time don't
change, so it seems like it would be misleading to call UpdateEntry with a
struct that contains them. I can imagine being surprised whether UpdateEntry
updates those fields or not.

That's the reason not to use DPSI, but I understand the desire to minimize code,
so LMK if that reason is compelling or if I should use DPSI anyway.

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
content/browser/download/download_manager.h:267: void
MarkDownloadOpened(DownloadItem* download);
On 2011/09/28 19:41:55, rdsmith wrote:
> I thought this was being removed?

I removed DownloadDatabase::MarkDownloadRemoved and
HistoryService::MarkDownloadRemoved, but there isn't a DownloadManager method
that just calls delegate->UpdateEntry(). All the other methods that call
delegate->UpdateEntry() also do a bunch of other stuff, depending on who calls
them. MarkDownloadOpened() will also implement DownloadOpensOutstanding in the
next patch set.

[benjhayden, 2011-09-29 18:59:01]

OpensOutstanding done. PTAL when you get a chance.

[Randy Smith (Not in Mondays), 2011-09-29 21:23:15]

Brett: Could you take a scan through download_database.cc?  I'd like the eye of
someone who has a bit more background on how we've done database accesses in the
past on this code until I'm happy I or Ben are solidly grounded in Chrome DB
patterns.

[Randy Smith (Not in Mondays), 2011-09-30 15:57:21]

Next round.  Believe it or not, I like what you're doing here.  That's why I'm
giving it so much attention :-}.

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
chrome/browser/history/history_types.h:745: struct DownloadItemData {
On 2011/09/28 20:29:39, b s h wrote:
> On 2011/09/28 19:41:55, rdsmith wrote:
> > Is there a reason not to use DownloadPersistentStoreInfo?  It was originally
> > (six months ago :-}) intended to communication with the history DB and to
> > include everything that the DownloadItem gets from the history DB.
> 
> DownloadItemData isn't what the DownloadItem gets from the db, it's what the
db
> gets from the item as the item changes. The url, referrer, and start_time
don't
> change, so it seems like it would be misleading to call UpdateEntry with a
> struct that contains them. I can imagine being surprised whether UpdateEntry
> updates those fields or not.
> 
> That's the reason not to use DPSI, but I understand the desire to minimize
code,
> so LMK if that reason is compelling or if I should use DPSI anyway.

I think I'd rather use DPSI.  We're already using it in two different ways (pass
information down to the DB for creation, pass all rows back from the DB) and the
values that are significant and not are different in those two cases (db id is
ignored in the first case and not in the second).  I'm inclined to suggest that
you put in debug only code that confirms that the non-mutable fields don't
change on update, and comment the structure with what's mutable and what's not. 
But maybe someday we'll want to mutate the fixed fields :-}.

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
content/browser/download/download_manager.h:267: void
MarkDownloadOpened(DownloadItem* download);
On 2011/09/28 20:29:39, b s h wrote:
> On 2011/09/28 19:41:55, rdsmith wrote:
> > I thought this was being removed?
> 
> I removed DownloadDatabase::MarkDownloadRemoved and
> HistoryService::MarkDownloadRemoved, but there isn't a DownloadManager method
> that just calls delegate->UpdateEntry(). All the other methods that call
> delegate->UpdateEntry() also do a bunch of other stuff, depending on who calls
> them. MarkDownloadOpened() will also implement DownloadOpensOutstanding in the
> next patch set.

Hmmm.  Ok.  From a pure functionality approach, I'd rather have a blunt hammer
(when something changes in the DownloadItem, it calls
DownloadManager::DownloadUpdated(), which passes the change along to the history
system.  Or even better, the DM is an observer of the DI, and just does the
automatic pass along if the DI is a persistent DI).  But that makes collecting
the opens outstanding statistic hard.  Still, if you'd be willing, keep an eye
out for ways to make this type of transformation; I think it would simplify the
DI->DM interface and reduce the size of the DM interface.  

(I'm tempted to request a "RecordOpensOustanding" method on the DM that the DI
calls on this transition, and then just switching the actual functionality over
to one of the sketches above, and if you want to that, feel free.  Of you can
leave it like this and keep a lookout for future improvements--your call.)

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

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:31: //   opened         1 if the
download has ever been opened else 0
Not even really at the level of "suggestion", more "floating an idea": What I've
seen in non-chromium code for documenting database columns is having the
comments for the meaning of each field interwoven with the "CREATE TABLE"
statement that creates the DB.  If you wanted to, that could be done by creating
a multi-line const char * which was the CREATE TABLE statement, putting it here,
and putting the comments on each line.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:244:
download_stats::RecordShelfSize([downloadItemControllers_ count]);
I believe that on some platforms, but not others, download shelves will
auto-close when the last unopened download is opened.  How will this affect the
statistics?

(At a minimum you should alter the CL description.  But I'd suggest putting some
thought into what this'll mean for stat interpretation.)

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:253:
download->IsInterrupted();
Is there a reason we're not just counting number in progress?

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:300: ", err=" << err <<
What's the context for this change?

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_manager.cc:822:
download_stats::RecordHistorySize(entries->size());
I'm trying to wrap my brain around how this will be biased by people who
shutdown their browsers/computers every day/hour/minute versus people who leave
them up for a Very Long Time.  I'd sorta expect that difference in users to
render this statistics somewhat meaningless; the browser bouncers will
completely dominate the samples, but may or may not be a particularly
representative sample of the population.  I'm not sure how to handle that; can
we rely purely on HistorySize2?

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:24: //
TODO(ahendrickson) -- Reduce the number of constructors.
I think it's reasonable to remove this--at least, a default constructor and a
full list constructor seem reasonable to me.  Could you check with Andy and then
maybe nuke this line while you're here?

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:35: bool opend);
nit: I dislike deliberately misspelling something to get around namespace
collisions.  download_opened?  been_opened?

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:66: bool opened;
It's pretty obvious, but I'd still include a comment here about what this
variable means.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:191: (1 << 10)/*num_buckets*/);
Why such large numbers for max and num buckets (especially buckets, since that's
memory space)?  I'd pick a number over which the conclusion would be "Ok, they
just don't clear their downloads", and not worry about getting data about the
nuances above that.  I'm also not sure what question we're trying to answer with
1024 buckets; that seems like overkill.  Enlighten me?

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:199: (1 << 10)/*num_buckets*/);
See above.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:219: (1 << 10)/*num_buckets*/);
See above.  I'd imagine we're really only interested in three buckets here: 1)
Clear every time, 2) Clear before too long, 3) Never (or almost never) clear.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:227: (1 << 10)/*num_buckets*/);
See comment immediately above (including the three buckets).  I don't mind more
buckets, but I don't want to gobble a lot of chrome memory without having a
specific question in mind I'm trying to answer.

[brettw, 2011-10-01 16:45:44]

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

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:82: meta_table_.Init(&GetDB(), 0,
0);
I'd rather use the versioning system for updating the table than manually
checking the stuff you're adding. In your case, the compat version would be 0
(since old versions of Chrome can still read this) but the new version would be
1. Then, like some of the other databases, you check for 0, add the columns, and
set the version. Be sure to do this in a transatcion.

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

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
chrome/browser/history/history.cc:544: void
HistoryService::UpdateDownload(history::DownloadItemData data) {
This should be a const ref instead of a copy.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
chrome/browser/history/history_backend.h:249: void
UpdateDownload(DownloadItemData data);
This should be a const ref. The runnable methods know to copy const refs when
sending to the background thread, but you're requiring yet another copy for it
to call your function.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.cc:391: bool
is_transfer_done = download->IsComplete() ||
This code is duplicated a number of times. Should we consider adding
is_transfer_done() to DownloadItem as a helper? Then this whole loop can be
  if (download_views_[i]->download()->is_transfer_done())
    ++num_pending;

[benjhayden, 2011-10-03 20:54:38]

PTAL

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/8008021/diff/8026/chrome/browser/history/histo...
chrome/browser/history/history_types.h:745: struct DownloadItemData {
On 2011/09/30 15:57:21, rdsmith wrote:
> On 2011/09/28 20:29:39, b s h wrote:
> > On 2011/09/28 19:41:55, rdsmith wrote:
> > > Is there a reason not to use DownloadPersistentStoreInfo?  It was
originally
> > > (six months ago :-}) intended to communication with the history DB and to
> > > include everything that the DownloadItem gets from the history DB.
> > 
> > DownloadItemData isn't what the DownloadItem gets from the db, it's what the
> db
> > gets from the item as the item changes. The url, referrer, and start_time
> don't
> > change, so it seems like it would be misleading to call UpdateEntry with a
> > struct that contains them. I can imagine being surprised whether UpdateEntry
> > updates those fields or not.
> > 
> > That's the reason not to use DPSI, but I understand the desire to minimize
> code,
> > so LMK if that reason is compelling or if I should use DPSI anyway.
> 
> I think I'd rather use DPSI.  We're already using it in two different ways
(pass
> information down to the DB for creation, pass all rows back from the DB) and
the
> values that are significant and not are different in those two cases (db id is
> ignored in the first case and not in the second).  I'm inclined to suggest
that
> you put in debug only code that confirms that the non-mutable fields don't
> change on update, and comment the structure with what's mutable and what's
not. 
> But maybe someday we'll want to mutate the fixed fields :-}.

Done.

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
content/browser/download/download_manager.h:267: void
MarkDownloadOpened(DownloadItem* download);
On 2011/09/30 15:57:21, rdsmith wrote:
> On 2011/09/28 20:29:39, b s h wrote:
> > On 2011/09/28 19:41:55, rdsmith wrote:
> > > I thought this was being removed?
> > 
> > I removed DownloadDatabase::MarkDownloadRemoved and
> > HistoryService::MarkDownloadRemoved, but there isn't a DownloadManager
method
> > that just calls delegate->UpdateEntry(). All the other methods that call
> > delegate->UpdateEntry() also do a bunch of other stuff, depending on who
calls
> > them. MarkDownloadOpened() will also implement DownloadOpensOutstanding in
the
> > next patch set.
> 
> Hmmm.  Ok.  From a pure functionality approach, I'd rather have a blunt hammer
> (when something changes in the DownloadItem, it calls
> DownloadManager::DownloadUpdated(), which passes the change along to the
history
> system.  Or even better, the DM is an observer of the DI, and just does the
> automatic pass along if the DI is a persistent DI).  But that makes collecting
> the opens outstanding statistic hard.  Still, if you'd be willing, keep an eye
> out for ways to make this type of transformation; I think it would simplify
the
> DI->DM interface and reduce the size of the DM interface.  

The DI could implement RecordOpensOutstanding() inline using DM->Search(). This
feels just a little dirty.

> 
> (I'm tempted to request a "RecordOpensOustanding" method on the DM that the DI
> calls on this transition, and then just switching the actual functionality
over
> to one of the sketches above, and if you want to that, feel free.  Of you can
> leave it like this and keep a lookout for future improvements--your call.)

DM::MarkDownloadOpened() fits with DM's current style, right? I think I'd rather
handle big refactors like this in batch CLs.

I can think of a few different ways of reorganizing the DM/DI interfaces. Want
to have a meeting at some point? Is there something already scheduled for
tomorrow's team meeting?

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

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:31: //   opened         1 if the
download has ever been opened else 0
On 2011/09/30 15:57:21, rdsmith wrote:
> Not even really at the level of "suggestion", more "floating an idea": What
I've
> seen in non-chromium code for documenting database columns is having the
> comments for the meaning of each field interwoven with the "CREATE TABLE"
> statement that creates the DB.  If you wanted to, that could be done by
creating
> a multi-line const char * which was the CREATE TABLE statement, putting it
here,
> and putting the comments on each line.

Done.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/down...
chrome/browser/history/download_database.cc:82: meta_table_.Init(&GetDB(), 0,
0);
On 2011/10/01 16:45:45, brettw wrote:
> I'd rather use the versioning system for updating the table than manually
> checking the stuff you're adding. In your case, the compat version would be 0
> (since old versions of Chrome can still read this) but the new version would
be
> 1. Then, like some of the other databases, you check for 0, add the columns,
and
> set the version. Be sure to do this in a transatcion.

Are you sure about using 0 and 1? sqlite3 .dump meta says |version|=20 and
|last_compatible_version|=16 as set by history_database.cc. DownloadDatabase is
actually not its own database, it's a table in the History database.

It looks like DownloadDatabase::InitDownloadTable() is already inside a
transaction in HistoryDatabase::Init(). Do I need another
committer.Begin/Commit()?

Is there a reason to avoid DoesColumnExist() besides convention? If so, can I
add that reason to sql/connection.h?

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

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
chrome/browser/history/history.cc:544: void
HistoryService::UpdateDownload(history::DownloadItemData data) {
On 2011/10/01 16:45:45, brettw wrote:
> This should be a const ref instead of a copy.

Done.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/history/hist...
chrome/browser/history/history_backend.h:249: void
UpdateDownload(DownloadItemData data);
On 2011/10/01 16:45:45, brettw wrote:
> This should be a const ref. The runnable methods know to copy const refs when
> sending to the background thread, but you're requiring yet another copy for it
> to call your function.

Done.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:244:
download_stats::RecordShelfSize([downloadItemControllers_ count]);
On 2011/09/30 15:57:21, rdsmith wrote:
> I believe that on some platforms, but not others, download shelves will
> auto-close when the last unopened download is opened.  How will this affect
the
> statistics?
> 
> (At a minimum you should alter the CL description.  But I'd suggest putting
some
> thought into what this'll mean for stat interpretation.)

I went ahead and split the stat.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:253:
download->IsInterrupted();
On 2011/09/30 15:57:21, rdsmith wrote:
> Is there a reason we're not just counting number in progress?

Done.

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.cc:391: bool
is_transfer_done = download->IsComplete() ||
On 2011/10/01 16:45:45, brettw wrote:
> This code is duplicated a number of times. Should we consider adding
> is_transfer_done() to DownloadItem as a helper? Then this whole loop can be
>   if (download_views_[i]->download()->is_transfer_done())
>     ++num_pending;

Done with IsInProgress().

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/web_resource...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/8008021/diff/26001/chrome/browser/web_resource...
chrome/browser/web_resource/promo_resource_service.cc:300: ", err=" << err <<
On 2011/09/30 15:57:21, rdsmith wrote:
> What's the context for this change?

I reached the NOTREACHED, but this file changed dramatically in today's merge
and I'm not reaching the NOTREACHED anymore.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_manager.cc:822:
download_stats::RecordHistorySize(entries->size());
On 2011/09/30 15:57:21, rdsmith wrote:
> I'm trying to wrap my brain around how this will be biased by people who
> shutdown their browsers/computers every day/hour/minute versus people who
leave
> them up for a Very Long Time.  I'd sorta expect that difference in users to
> render this statistics somewhat meaningless; the browser bouncers will
> completely dominate the samples, but may or may not be a particularly
> representative sample of the population.  I'm not sure how to handle that; can
> we rely purely on HistorySize2?

Done.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:24: //
TODO(ahendrickson) -- Reduce the number of constructors.
On 2011/09/30 15:57:21, rdsmith wrote:
> I think it's reasonable to remove this--at least, a default constructor and a
> full list constructor seem reasonable to me.  Could you check with Andy and
then
> maybe nuke this line while you're here?

Andy still wants to fix it.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:35: bool opend);
On 2011/09/30 15:57:21, rdsmith wrote:
> nit: I dislike deliberately misspelling something to get around namespace
> collisions.  download_opened?  been_opened?

Done.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:66: bool opened;
On 2011/09/30 15:57:21, rdsmith wrote:
> It's pretty obvious, but I'd still include a comment here about what this
> variable means.

Done.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:191: (1 << 10)/*num_buckets*/);
On 2011/09/30 15:57:21, rdsmith wrote:
> Why such large numbers for max and num buckets (especially buckets, since
that's
> memory space)?  I'd pick a number over which the conclusion would be "Ok, they
> just don't clear their downloads", and not worry about getting data about the
> nuances above that.  I'm also not sure what question we're trying to answer
with
> 1024 buckets; that seems like overkill.  Enlighten me?

Done.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:199: (1 << 10)/*num_buckets*/);
On 2011/09/30 15:57:21, rdsmith wrote:
> See above.

Done.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:219: (1 << 10)/*num_buckets*/);
On 2011/09/30 15:57:21, rdsmith wrote:
> See above.  I'd imagine we're really only interested in three buckets here: 1)
> Clear every time, 2) Clear before too long, 3) Never (or almost never) clear.

I thought that we were also looking to objectively quantify "before too long",
so I set all the "big" ones to 1024 max, 32 buckets.

http://codereview.chromium.org/8008021/diff/26001/content/browser/download/do...
content/browser/download/download_stats.cc:227: (1 << 10)/*num_buckets*/);
On 2011/09/30 15:57:21, rdsmith wrote:
> See comment immediately above (including the three buckets).  I don't mind
more
> buckets, but I don't want to gobble a lot of chrome memory without having a
> specific question in mind I'm trying to answer.

Done.

[brettw, 2011-10-03 21:10:02]

Okay, I guess it's better to leave the versioning stuff as-is. LGTM from me for
some high-level DB stuff, I'll leave the rest to rdsmith.

[Randy Smith (Not in Mondays), 2011-10-04 18:34:31]

Looking good.  A couple of high level comments:

+ What is your definition of "pending" (in the issue description)?  If it's just
in progress, I'd use that vocabulary--it isn't necessarily better, but it's what
we've been using.  If it doesn't mean that, you should define it (and scan
through the CL for other uses of it; there are variables where it appears to
mean in-progress).

+ I think the comment in the issue description about ScheduleAndForget &
DownloadItemData is now outdated.

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8008021/diff/8026/content/browser/download/dow...
content/browser/download/download_manager.h:267: void
MarkDownloadOpened(DownloadItem* download);
On 2011/10/03 20:54:39, b s h wrote:
> On 2011/09/30 15:57:21, rdsmith wrote:
> > On 2011/09/28 20:29:39, b s h wrote:
> > > On 2011/09/28 19:41:55, rdsmith wrote:
> > > > I thought this was being removed?
> > > 
> > > I removed DownloadDatabase::MarkDownloadRemoved and
> > > HistoryService::MarkDownloadRemoved, but there isn't a DownloadManager
> method
> > > that just calls delegate->UpdateEntry(). All the other methods that call
> > > delegate->UpdateEntry() also do a bunch of other stuff, depending on who
> calls
> > > them. MarkDownloadOpened() will also implement DownloadOpensOutstanding in
> the
> > > next patch set.
> > 
> > Hmmm.  Ok.  From a pure functionality approach, I'd rather have a blunt
hammer
> > (when something changes in the DownloadItem, it calls
> > DownloadManager::DownloadUpdated(), which passes the change along to the
> history
> > system.  Or even better, the DM is an observer of the DI, and just does the
> > automatic pass along if the DI is a persistent DI).  But that makes
collecting
> > the opens outstanding statistic hard.  Still, if you'd be willing, keep an
eye
> > out for ways to make this type of transformation; I think it would simplify
> the
> > DI->DM interface and reduce the size of the DM interface.  
> 
> The DI could implement RecordOpensOutstanding() inline using DM->Search().
This
> feels just a little dirty.

Agreed.

> > (I'm tempted to request a "RecordOpensOustanding" method on the DM that the
DI
> > calls on this transition, and then just switching the actual functionality
> over
> > to one of the sketches above, and if you want to that, feel free.  Of you
can
> > leave it like this and keep a lookout for future improvements--your call.)
> 
> DM::MarkDownloadOpened() fits with DM's current style, right? I think I'd
rather
> handle big refactors like this in batch CLs.
> 
> I can think of a few different ways of reorganizing the DM/DI interfaces. Want
> to have a meeting at some point? Is there something already scheduled for
> tomorrow's team meeting?

Fair enough.  I agree that we should try and keep interfaces internally
self-consistent and change them all at once.  

(OKRs is the primary thing on the agenda for tomorrow's meeting :-}.  Maybe next
week.)

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/history/down...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/history/down...
chrome/browser/history/download_database.h:38: // Does not update |url|,
|start_time|, |total_bytes|, or |db_handle|.
The comment that it doesn't update db_handle is a little misleading, as it
implies that db_handle is a peer of the other values mentioned, and the value of
db_handle is used to figure out what to update.

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.cc:157: auto_closed_ =
true;
This seems weird to me.  Are you sure?  Two concerns:
+ I thought that the Windows shelf closed on last unopened download open; this
function doesn't sound like it's used in that case.
+ If a shelf can close and reopen, that doesn't reset auto_closed_ to false.

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
content/browser/download/download_manager_delegate.h:77: // Does not update
|url|, |start_time|, |total_bytes|, or |db_handle|.
See previous comment on previous instance of this comment.

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:23: struct
CONTENT_EXPORT DownloadPersistentStoreInfo {
I think what would be ideal here is a comment describing the full dataflow for
this class.  Maybe something at the top:
* When used to create a history entry, all fields except for db_handle are input
fields.
* When used to update a history entry, db_handle is used to identify the entry
to update, and fields ... are updated (or all fields except ... are updated).
* When used to query the database for existing items, all fields are returned.

[benjhayden, 2011-10-06 21:25:16]

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/history/down...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/history/down...
chrome/browser/history/download_database.h:38: // Does not update |url|,
|start_time|, |total_bytes|, or |db_handle|.
On 2011/10/04 18:34:31, rdsmith wrote:
> The comment that it doesn't update db_handle is a little misleading, as it
> implies that db_handle is a peer of the other values mentioned, and the value
of
> db_handle is used to figure out what to update.  

Done.

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

http://codereview.chromium.org/8008021/diff/42001/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.cc:157: auto_closed_ =
true;
On 2011/10/04 18:34:31, rdsmith wrote:
> This seems weird to me.  Are you sure?  Two concerns:
> + I thought that the Windows shelf closed on last unopened download open; this
> function doesn't sound like it's used in that case.
> + If a shelf can close and reopen, that doesn't reset auto_closed_ to false.

Done.

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
content/browser/download/download_manager_delegate.h:77: // Does not update
|url|, |start_time|, |total_bytes|, or |db_handle|.
On 2011/10/04 18:34:31, rdsmith wrote:
> See previous comment on previous instance of this comment.

Done.

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/42001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:23: struct
CONTENT_EXPORT DownloadPersistentStoreInfo {
On 2011/10/04 18:34:31, rdsmith wrote:
> I think what would be ideal here is a comment describing the full dataflow for
> this class.  Maybe something at the top:
> * When used to create a history entry, all fields except for db_handle are
input
> fields.
> * When used to update a history entry, db_handle is used to identify the entry
> to update, and fields ... are updated (or all fields except ... are updated).
> * When used to query the database for existing items, all fields are returned.

Done.

[Randy Smith (Not in Mondays), 2011-10-09 23:40:17]

High level comment: Make sure (if you haven't already) to try running some
downloads and looking at about:histograms/download to see if the results are in
line with what you expect.  You might specifically want to look at how the buck
ranges end up looking for each of the histograms, and if you're happy with that.
 You might want to get Asanka's input on that issue specifically, as he's the
primary person that'll be making decisions on this data, so he'll be best able
to judge the ROI of # of buckets vs. information value.  

Beyond that and the nits below (most of which are suggestions anyway), LGTM. 
Thanks!

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

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/history/down...
chrome/browser/history/download_database.cc:33: "opened INTEGER NOT NULL)";  //
1 if the download has ever been opened else 0
nit/suggestion: Line up the comments.

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:250:
[downloadItemControllers_ count], numInProgress, sender == nil);
Would a comment here about why sender == nil implies that we're being
auto-closed be appropriate?

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:28: // the history,
all fields except |referrer_url| are set by the DownloadDatabase
Why isn't referrer_url set/read?

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_stats.cc:198: 128);
Hmmm.  Thinking about it, why 128?  There's a way in which anything above 10-12
is "many", as they can't all be displayed on the download shelf, so (I presume)
user's will tend not to have more than that many in progress at a time.  I could
image we'd want to know when that does happen (though I'll bet you it points to
an OS organizer :-}), but I'm not sure that we really gain from having a fine
granularity between, say, 20 and 128.  It probably doesn't matter--it's not a
lot of memory--but I'd like to be mindful about memory footprint anyway.

An ideal world would be the standard logarithmic histogram (fine detail at the
small end, coarse at the large) where we could guarantee that "fine" meant "each
count gets its own bucket until six or so".  But we might actually get that. 
Maybe try a standard histogram with a max of 128, and see what the bucketing
looks like at the low values and if you think that'll give you the information
you want?

Or just ignore this comment--it isn't something I feel strongly about at this
size.  But my guess is that we've got a lower return of valuable information
from memory footprint investment in these histograms.

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_stats.cc:225: 32/*num_buckets*/);
And hear I'd actually increase the number of buckets.  I think we're more
interested in variation in something like the 1-10 range, and above that we're
thinking either major batch processor or OS organizer, and it'll be a little
hard to judge based on this statistic.  And I'm not sure how fine granularity 32
buckets will give us at the low end.  But again, I don't feel strongly.

[benjhayden, 2011-10-10 16:26:22]

On 2011/10/09 23:40:17, rdsmith wrote:
> High level comment: Make sure (if you haven't already) to try running some
> downloads and looking at about:histograms/download to see if the results are
in
> line with what you expect.  You might specifically want to look at how the
buck
> ranges end up looking for each of the histograms, and if you're happy with
that.
>  You might want to get Asanka's input on that issue specifically, as he's the
> primary person that'll be making decisions on this data, so he'll be best able
> to judge the ROI of # of buckets vs. information value. 

I made sure that they work. I'll play around with them some more and look at the
buckets more closely.

 
> 
> Beyond that and the nits below (most of which are suggestions anyway), LGTM. 
> Thanks!
> 
>
http://codereview.chromium.org/8008021/diff/47022/chrome/browser/history/down...
> File chrome/browser/history/download_database.cc (right):
> 
>
http://codereview.chromium.org/8008021/diff/47022/chrome/browser/history/down...
> chrome/browser/history/download_database.cc:33: "opened INTEGER NOT NULL)"; 
//
> 1 if the download has ever been opened else 0
> nit/suggestion: Line up the comments.
> 
>
http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
> File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):
> 
>
http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
> chrome/browser/ui/cocoa/download/download_shelf_controller.mm:250:
> [downloadItemControllers_ count], numInProgress, sender == nil);
> Would a comment here about why sender == nil implies that we're being
> auto-closed be appropriate?
> 
>
http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
> File content/browser/download/download_persistent_store_info.h (right):
> 
>
http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
> content/browser/download/download_persistent_store_info.h:28: // the history,
> all fields except |referrer_url| are set by the DownloadDatabase
> Why isn't referrer_url set/read?
> 
>
http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
> File content/browser/download/download_stats.cc (right):
> 
>
http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
> content/browser/download/download_stats.cc:198: 128);
> Hmmm.  Thinking about it, why 128?  There's a way in which anything above
10-12
> is "many", as they can't all be displayed on the download shelf, so (I
presume)
> user's will tend not to have more than that many in progress at a time.  I
could
> image we'd want to know when that does happen (though I'll bet you it points
to
> an OS organizer :-}), but I'm not sure that we really gain from having a fine
> granularity between, say, 20 and 128.  It probably doesn't matter--it's not a
> lot of memory--but I'd like to be mindful about memory footprint anyway.
> 
> An ideal world would be the standard logarithmic histogram (fine detail at the
> small end, coarse at the large) where we could guarantee that "fine" meant
"each
> count gets its own bucket until six or so".  But we might actually get that. 
> Maybe try a standard histogram with a max of 128, and see what the bucketing
> looks like at the low values and if you think that'll give you the information
> you want?
> 
> Or just ignore this comment--it isn't something I feel strongly about at this
> size.  But my guess is that we've got a lower return of valuable information
> from memory footprint investment in these histograms.
> 
>
http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
> content/browser/download/download_stats.cc:225: 32/*num_buckets*/);
> And hear I'd actually increase the number of buckets.  I think we're more
> interested in variation in something like the 1-10 range, and above that we're
> thinking either major batch processor or OS organizer, and it'll be a little
> hard to judge based on this statistic.  And I'm not sure how fine granularity
32
> buckets will give us at the low end.  But again, I don't feel strongly.

[benjhayden, 2011-10-10 16:27:11]

PTAL

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

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/history/down...
chrome/browser/history/download_database.cc:33: "opened INTEGER NOT NULL)";  //
1 if the download has ever been opened else 0
On 2011/10/09 23:40:17, rdsmith wrote:
> nit/suggestion: Line up the comments.

Done.

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right):

http://codereview.chromium.org/8008021/diff/47022/chrome/browser/ui/cocoa/dow...
chrome/browser/ui/cocoa/download/download_shelf_controller.mm:250:
[downloadItemControllers_ count], numInProgress, sender == nil);
On 2011/10/09 23:40:17, rdsmith wrote:
> Would a comment here about why sender == nil implies that we're being
> auto-closed be appropriate?

There already was a comment below. I refactored this a little to make the idea
more prominent.

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:28: // the history,
all fields except |referrer_url| are set by the DownloadDatabase
On 2011/10/09 23:40:17, rdsmith wrote:
> Why isn't referrer_url set/read?

I imagine that nobody's needed to persist it yet. I have no idea what it's doing
in this struct. I'd offer to kill it if Andy wasn't about to use it.

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_stats.cc:198: 128);
On 2011/10/09 23:40:17, rdsmith wrote:
> Hmmm.  Thinking about it, why 128?  There's a way in which anything above
10-12
> is "many", as they can't all be displayed on the download shelf, so (I
presume)
> user's will tend not to have more than that many in progress at a time.  I
could
> image we'd want to know when that does happen (though I'll bet you it points
to
> an OS organizer :-}), but I'm not sure that we really gain from having a fine
> granularity between, say, 20 and 128.  It probably doesn't matter--it's not a
> lot of memory--but I'd like to be mindful about memory footprint anyway.
> 
> An ideal world would be the standard logarithmic histogram (fine detail at the
> small end, coarse at the large) where we could guarantee that "fine" meant
"each
> count gets its own bucket until six or so".  But we might actually get that. 
> Maybe try a standard histogram with a max of 128, and see what the bucketing
> looks like at the low values and if you think that'll give you the information
> you want?
> 
> Or just ignore this comment--it isn't something I feel strongly about at this
> size.  But my guess is that we've got a lower return of valuable information
> from memory footprint investment in these histograms.

16?

http://codereview.chromium.org/8008021/diff/47022/content/browser/download/do...
content/browser/download/download_stats.cc:225: 32/*num_buckets*/);
On 2011/10/09 23:40:17, rdsmith wrote:
> And hear I'd actually increase the number of buckets.  I think we're more
> interested in variation in something like the 1-10 range, and above that we're
> thinking either major batch processor or OS organizer, and it'll be a little
> hard to judge based on this statistic.  And I'm not sure how fine granularity
32
> buckets will give us at the low end.  But again, I don't feel strongly.

64?

[Randy Smith (Not in Mondays), 2011-10-14 17:21:00]

Sorry, this fell off my radar screen because I had noted myself as LGTMing it. 
Still LGTM.

[commit-bot: I haz the power, 2011-10-14 20:47:37]

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

[commit-bot: I haz the power, 2011-10-14 22:48:24]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 105591, job name
was 8008021-77040 (previous was lost) (previous was lost) (retry) (previous was
lost).

[commit-bot: I haz the power, 2011-10-17 17:37:39]

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

[commit-bot: I haz the power, 2011-10-17 19:08:09]

Change committed as 105871