Isolate initiation counts for downloads to their own histograms and improve

Isolate initiation counts for downloads to their own histograms and improve
naming.

BUG=None


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

[Randy Smith (Not in Mondays), 2012-02-04 15:20:37]

Chris, could you take a look at this?  This came out of a larger discussion in
email around a change of John's that touched on histograms; I can send you the
email if you want, but I think it's clear what I'm trying to do here and why.

I want a full review, but specific questions I'm curious about your thoughts on
include:
* Should I be using the same routine name in chrome and content?  It does the
same thing and it's in different namespaces, but I'm still not sure.
* Should UNTHROTTLED_COUNT (the total count of all downloads initiated) be in
the new histogram or the old one?

[cbentzel, 2012-02-04 16:19:02]

high-level question: Should the "source" of a download be passed into
DownloadResourceHandler + Item, so we can record data on a per-source basis.

i.e. look at download times per-source, content types per-source, errors
per-source, bytes-per-source, etc.

[Randy Smith (Not in Mondays), 2012-02-04 16:41:43]

On 2012/02/04 16:19:02, cbentzel wrote:
> high-level question: Should the "source" of a download be passed into
> DownloadResourceHandler + Item, so we can record data on a per-source basis.
> 
> i.e. look at download times per-source, content types per-source, errors
> per-source, bytes-per-source, etc.

It's a good idea, but by itself it slams into the chrome/content split.  Some of
our sources are in content, and some in chrome, and we can't have any
dependencies within content on what's in chrome.  The discussion that provoked
this CL whacked at this pi~nata for a while but didn't get it to split open; as
far as we got was "we could pass opaque sources into content, and we could make
the chrome level enum for those sources dependent on the content sources, but we
don't have a way to specify in content how large that enum should be".

I'll forward you the discussion in separate email.

[cbentzel, 2012-02-06 19:57:29]

This generally looks fine - I'd like to get this data, and my complaints are
mostly nits.

You'll need to integrate with my recent changes, and a few nits. Would be nice
to be consistent about whether recording the source comes before or after the
trigger of the download.

I'd still love it if a source is attached to the DownloadItem - either at the
content layer, or by maintaining a download_id->source map at the Chrome layer.
I'll review the thread you sent me.

http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_throttling_resource_handler.cc (right):

http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_throttling_resource_handler.cc:137:
download_util::RecordDownloadSource(download_util::INITIATED_BY_NAVIGATION);
Should this be done here or at the BufferedResourceHandler side, and then we
could record a hold-back here?

http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
File content/browser/download/download_stats.h (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
content/browser/download/download_stats.h:72: //
SavePackage::IsSaveableContents().
I don't love the name of "SAVE_PACKAGE_FAILURE" - makes it sound like there's a
problem, when really it's just non-HTML content. Not going to push hard against
the name, though.

http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
File content/browser/download/drag_download_file.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
content/browser/download/drag_download_file.cc:142:
download_stats::RecordDownloadSource(
Most of the other cases record prior to doing the download. Perhaps that could
be done here.

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1429:
download_stats::INITIATED_BY_RENDERER_SAVE);
Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here.

[Randy Smith (Not in Mondays), 2012-02-06 20:13:22]

On 2012/02/06 19:57:29, cbentzel wrote:
> This generally looks fine - I'd like to get this data, and my complaints are
> mostly nits.
> 
> You'll need to integrate with my recent changes, and a few nits. Would be nice
> to be consistent about whether recording the source comes before or after the
> trigger of the download.
> 
> I'd still love it if a source is attached to the DownloadItem - either at the
> content layer, or by maintaining a download_id->source map at the Chrome
layer.
> I'll review the thread you sent me.

I'm very much open to this idea, if we can find a way to do it cleanly.  But I'm
going to assume we're not doing it in this CL unless you come up with a good
idea before commit :-}.  

I'll integrate your comments and toss another round of this back at you
(probably tomorrow).


> 
>
http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
> File chrome/browser/download/download_throttling_resource_handler.cc (right):
> 
>
http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
> chrome/browser/download/download_throttling_resource_handler.cc:137:
> download_util::RecordDownloadSource(download_util::INITIATED_BY_NAVIGATION);
> Should this be done here or at the BufferedResourceHandler side, and then we
> could record a hold-back here?
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
> File content/browser/download/download_stats.h (right):
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
> content/browser/download/download_stats.h:72: //
> SavePackage::IsSaveableContents().
> I don't love the name of "SAVE_PACKAGE_FAILURE" - makes it sound like there's
a
> problem, when really it's just non-HTML content. Not going to push hard
against
> the name, though.
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
> File content/browser/download/drag_download_file.cc (right):
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
> content/browser/download/drag_download_file.cc:142:
> download_stats::RecordDownloadSource(
> Most of the other cases record prior to doing the download. Perhaps that could
> be done here.
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
> File content/browser/tab_contents/tab_contents.cc (right):
> 
>
http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
> content/browser/tab_contents/tab_contents.cc:1429:
> download_stats::INITIATED_BY_RENDERER_SAVE);
> Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here.

[cbentzel, 2012-02-06 20:15:07]

On Mon, Feb 6, 2012 at 3:13 PM, <rdsmith@chromium.org> wrote:

> On 2012/02/06 19:57:29, cbentzel wrote:
>
>> This generally looks fine - I'd like to get this data, and my complaints
>> are
>> mostly nits.
>>
>
>  You'll need to integrate with my recent changes, and a few nits. Would be
>> nice
>> to be consistent about whether recording the source comes before or after
>> the
>> trigger of the download.
>>
>
>  I'd still love it if a source is attached to the DownloadItem - either at
>> the
>> content layer, or by maintaining a download_id->source map at the Chrome
>>
> layer.
>
>> I'll review the thread you sent me.
>>
>
> I'm very much open to this idea, if we can find a way to do it cleanly.
>  But I'm
> going to assume we're not doing it in this CL unless you come up with a
> good
> idea before commit :-}.
>
> I'll integrate your comments and toss another round of this back at you
> (probably tomorrow).


Yeah, that seems like a good approach.

>
>
>
>
>
> http://codereview.chromium.**org/9316116/diff/1/chrome/**
>
browser/download/download_**throttling_resource_handler.cc<http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/download_throttling_resource_handler.cc>
>
>> File chrome/browser/download/**download_throttling_resource_**handler.cc
>> (right):
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/chrome/**
>
browser/download/download_**throttling_resource_handler.**cc#newcode137<http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/download_throttling_resource_handler.cc#newcode137>
>
>> chrome/browser/download/**download_throttling_resource_**handler.cc:137:
>> download_util::**RecordDownloadSource(download_**
>> util::INITIATED_BY_NAVIGATION)**;
>> Should this be done here or at the BufferedResourceHandler side, and then
>> we
>> could record a hold-back here?
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/download/download_**stats.h<http://codereview.chromium.org/9316116/diff/1/content/browser/download/download_stats.h>
>
>> File content/browser/download/**download_stats.h (right):
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/download/download_**stats.h#newcode72<http://codereview.chromium.org/9316116/diff/1/content/browser/download/download_stats.h#newcode72>
>
>> content/browser/download/**download_stats.h:72: //
>> SavePackage::**IsSaveableContents().
>> I don't love the name of "SAVE_PACKAGE_FAILURE" - makes it sound like
>> there's
>>
> a
>
>> problem, when really it's just non-HTML content. Not going to push hard
>>
> against
>
>> the name, though.
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/download/drag_**download_file.cc<http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_download_file.cc>
>
>> File content/browser/download/drag_**download_file.cc (right):
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/download/drag_**download_file.cc#newcode142<http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_download_file.cc#newcode142>
>
>> content/browser/download/drag_**download_file.cc:142:
>> download_stats::**RecordDownloadSource(
>> Most of the other cases record prior to doing the download. Perhaps that
>> could
>> be done here.
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/tab_contents/tab_**contents.cc<http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/tab_contents.cc>
>
>> File content/browser/tab_contents/**tab_contents.cc (right):
>>
>
>
> http://codereview.chromium.**org/9316116/diff/1/content/**
>
browser/tab_contents/tab_**contents.cc#newcode1429<http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/tab_contents.cc#newcode1429>
>
>> content/browser/tab_contents/**tab_contents.cc:1429:
>> download_stats::INITIATED_BY_**RENDERER_SAVE);
>> Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here.
>>
>
>
>
>
http://codereview.chromium.**org/9316116/<http://codereview.chromium.org/9316...
>

[Randy Smith (Not in Mondays), 2012-02-07 18:20:17]

Jim: I'll be asking you to review the histograms.xml version of this change, so
I wanted to give you a chance to look at the change before then in case you had
any high level "Wait, you can't do that!"s.  Feel free to leave it to Chris and
I if you're not concerned about those.

Chris: Let me know what you think of my responses.

http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_throttling_resource_handler.cc (right):

http://codereview.chromium.org/9316116/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_throttling_resource_handler.cc:137:
download_util::RecordDownloadSource(download_util::INITIATED_BY_NAVIGATION);
On 2012/02/06 19:57:29, cbentzel wrote:
> Should this be done here or at the BufferedResourceHandler side, and then we
> could record a hold-back here?

Depends how we're thinking of the stats.  I'm more inclined to have everything? 
most things?  in the initiation histogram be actually initiated downloads (i.e.
summing to UNTHROTTLED_COUNT) rather than have to do subtractions to see what
actually ended up being downloaded.  Having said that, I think it would be good
to also count the number of things blocked by DownloadThrottlingResourceHandler,
and I'll add UMA to that effect.  I think I'll leave it in the initiation
histograms enum/UMA, though if you think it should go back into counts, let me
know.

http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
File content/browser/download/download_stats.h (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/download/downlo...
content/browser/download/download_stats.h:72: //
SavePackage::IsSaveableContents().
On 2012/02/06 19:57:29, cbentzel wrote:
> I don't love the name of "SAVE_PACKAGE_FAILURE" - makes it sound like there's
a
> problem, when really it's just non-HTML content. Not going to push hard
against
> the name, though.

Nah, you're right, and more to the point, I think I can change the name without
messing up any histograms (as long as I leave the value the same).  

Done.

http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
File content/browser/download/drag_download_file.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/download/drag_d...
content/browser/download/drag_download_file.cc:142:
download_stats::RecordDownloadSource(
On 2012/02/06 19:57:29, cbentzel wrote:
> Most of the other cases record prior to doing the download. Perhaps that could
> be done here.

Done.

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1429:
download_stats::INITIATED_BY_RENDERER_SAVE);
On 2012/02/06 19:57:29, cbentzel wrote:
> Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here. 

Yeah; from the browser process perspective the distinction is
ViewHostMsg_{DownloadUrl, SaveURLAs}.  I'm open to different names for the enum
values if you have them, but I don't feel certain enough in my own heart that
the only place we could ever get that IPC is from pepper to call it out to
pepper.  But if you do, I'll change it.

[Randy Smith (Not in Mondays), 2012-02-07 18:23:44]

(Sorry; quick followup patch set to regularize the relationship of recording to
initiation at all sites.)

[cbentzel, 2012-02-07 18:29:32]

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1429:
download_stats::INITIATED_BY_RENDERER_SAVE);
On 2012/02/07 18:20:18, rdsmith wrote:
> On 2012/02/06 19:57:29, cbentzel wrote:
> > Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here. 
> 
> Yeah; from the browser process perspective the distinction is
> ViewHostMsg_{DownloadUrl, SaveURLAs}.  I'm open to different names for the
enum
> values if you have them, but I don't feel certain enough in my own heart that
> the only place we could ever get that IPC is from pepper to call it out to
> pepper.  But if you do, I'll change it.
> 

Right now it's only from Pepper. If you merge/rebase you'll see that I've
commented this function to indicate that as well.

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
File chrome/browser/download/download_throttling_resource_handler.cc (right):

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
chrome/browser/download/download_throttling_resource_handler.cc:146:
download_util::RecordDownloadSource(download_util::BLOCKED_BY_THROTTLING);
I don't like having this as a separate source, as the source is ultimately the
same (navigation). Prefer to have a separate histogram for the throttling
behavior.

[jar (doing other things), 2012-02-07 19:18:50]

FWIW: The histogram changes look fine... though I didn't look into the logic
driving the calls and values.

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
chrome/browser/download/download_util.cc:633: "Download.SourcesChrome", source,
DOWNLOAD_INITIATION_SOURCES_LAST_ENTRY);
FYI: This sort of transition is good/common. You should mark the old name as
"obsolete" in the XML file (don't delete it) when the time comes.

[Randy Smith (Not in Mondays), 2012-02-07 19:28:13]

Comments incorporated.  Gonna merge up to TOT next; there's a nasty CL (for
these purposes) in-between and I'll want comments on the merge before I submit.

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9316116/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1429:
download_stats::INITIATED_BY_RENDERER_SAVE);
On 2012/02/07 18:29:32, cbentzel wrote:
> On 2012/02/07 18:20:18, rdsmith wrote:
> > On 2012/02/06 19:57:29, cbentzel wrote:
> > > Perhaps INITIATED_BY_PEPPER_SAVE? I guess we don't differentiate here. 
> > 
> > Yeah; from the browser process perspective the distinction is
> > ViewHostMsg_{DownloadUrl, SaveURLAs}.  I'm open to different names for the
> enum
> > values if you have them, but I don't feel certain enough in my own heart
that
> > the only place we could ever get that IPC is from pepper to call it out to
> > pepper.  But if you do, I'll change it.
> > 
> 
> Right now it's only from Pepper. If you merge/rebase you'll see that I've
> commented this function to indicate that as well.

Done.

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
File chrome/browser/download/download_throttling_resource_handler.cc (right):

http://codereview.chromium.org/9316116/diff/14001/chrome/browser/download/dow...
chrome/browser/download/download_throttling_resource_handler.cc:146:
download_util::RecordDownloadSource(download_util::BLOCKED_BY_THROTTLING);
On 2012/02/07 18:29:32, cbentzel wrote:
> I don't like having this as a separate source, as the source is ultimately the
> same (navigation). Prefer to have a separate histogram for the throttling
> behavior. 

Moved into a "Counts" histogram at the chrome layer parallel to the same
histogram in content.

[Randy Smith (Not in Mondays), 2012-02-07 20:24:21]

Merge to TOT done.  Chris, PTAL.

[cbentzel, 2012-02-07 20:27:33]

LGTM

https://chromiumcodereview.appspot.com/9316116/diff/11019/chrome/browser/down...
File chrome/browser/download/download_util.h (right):

https://chromiumcodereview.appspot.com/9316116/diff/11019/chrome/browser/down...
chrome/browser/download/download_util.h:186: enum
ChromeDownloadInitiationSources {
Nit: why not ChromeDownloadSources? Or name RecordDownloadInitiationSource for
the function. Would be nice to be consistent.

[Randy Smith (Not in Mondays), 2012-02-09 21:48:01]

https://chromiumcodereview.appspot.com/9316116/diff/11019/chrome/browser/down...
File chrome/browser/download/download_util.h (right):

https://chromiumcodereview.appspot.com/9316116/diff/11019/chrome/browser/down...
chrome/browser/download/download_util.h:186: enum
ChromeDownloadInitiationSources {
On 2012/02/07 20:27:33, cbentzel wrote:
> Nit: why not ChromeDownloadSources? Or name RecordDownloadInitiationSource for
> the function. Would be nice to be consistent.

Tried to regularize naming; let me know if you have any opinions.

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

Darin: Can you take a look at download_resource_throttle.cc?  It's a simple
change, but I had to take the UMA out from the if below (because then if the
request was deferred the UMA would never happen) and I wanted to make sure there
weren't other subtleties in the throttling model that I was missing.

[darin (slow to review), 2012-02-10 23:47:27]

https://chromiumcodereview.appspot.com/9316116/diff/34005/chrome/browser/down...
File chrome/browser/download/download_resource_throttle.cc (right):

https://chromiumcodereview.appspot.com/9316116/diff/34005/chrome/browser/down...
chrome/browser/download/download_resource_throttle.cc:45:
download_util::RecordDownloadSource(
nit: indentation

You are counting on the fact that ContinueDownload only gets called once and
that DownloadResourceThrottle is only used for downloads initiated by
navigation, right?  Those are pretty obvious pre-conditions for this code, so
maybe it is fine to just assume that.  Otherwise, maybe it is worth a comment or
even an assertion, in case someone extends this class or uses it in another
context later.  I suppose the INITIATED_BY_NAVIGATION flag could be passed to
the DownloadResourceThrottle constructor if need be.

[Randy Smith (Not in Mondays), 2012-02-12 19:17:41]

On 2012/02/10 23:47:27, darin wrote:
>
https://chromiumcodereview.appspot.com/9316116/diff/34005/chrome/browser/down...
> File chrome/browser/download/download_resource_throttle.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9316116/diff/34005/chrome/browser/down...
> chrome/browser/download/download_resource_throttle.cc:45:
> download_util::RecordDownloadSource(
> nit: indentation
> 
> You are counting on the fact that ContinueDownload only gets called once and
> that DownloadResourceThrottle is only used for downloads initiated by
> navigation, right?  

And that ContinueDownload will always be called (or is called exactly once, if
you prefer); that there aren't any other paths forward in this code.  But that
looks pretty obvious too (except for request cancellation, which I think I'm ok
with not leaving one of these UMA traces if it happens).

> Those are pretty obvious pre-conditions for this code, so
> maybe it is fine to just assume that.  Otherwise, maybe it is worth a comment
or
> even an assertion, in case someone extends this class or uses it in another
> context later.  

I feel more comfortable with the comment, so I put it in.  (Not yet uploaded;
hold review for my next message.)

> I suppose the INITIATED_BY_NAVIGATION flag could be passed to
> the DownloadResourceThrottle constructor if need be.

[Randy Smith (Not in Mondays), 2012-02-12 19:19:00]

PTAL.

http://codereview.chromium.org/9316116/diff/34005/chrome/browser/download/dow...
File chrome/browser/download/download_resource_throttle.cc (right):

http://codereview.chromium.org/9316116/diff/34005/chrome/browser/download/dow...
chrome/browser/download/download_resource_throttle.cc:45:
download_util::RecordDownloadSource(
On 2012/02/10 23:47:28, darin wrote:
> nit: indentation

Done.

[Randy Smith (Not in Mondays), 2012-02-14 19:41:14]

Jim: I put in the hack we talked about; could you re-review?  This link has just
and only the change that I think you need to review:

http://codereview.chromium.org/9316116/diff2/28007:19002/chrome/browser/downl...

[jar (doing other things), 2012-02-14 19:46:42]

The comment and adjustment to the histogram enum LGTM.

[darin (slow to review), 2012-02-14 19:48:06]

LGTM for download_resource_throttle.cc changes.

[commit-bot: I haz the power, 2012-02-15 19:00:46]

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

[commit-bot: I haz the power, 2012-02-15 21:56:50]

Change committed as 122159