Add UMA for measuring Content-Dispostion header use and abuse.

Add UMA for measuring Content-Dispostion header use and abuse.

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

[asanka, 2012-12-12 00:11:01]

Could you take a look?

rdsmith: downloads and UMA. Specifically whether this is counting correctly. We
only want to get UMA for downloads, excluding Save Package. It seems START_COUNT
also counts Save Package downloads.

abarth: http_content_disposition. Also whether the UMA is sufficient. I opted to
count individual properties instead of what was discussed in the issue.

[Randy Smith (Not in Mondays), 2012-12-12 16:10:35]

content/browser/download/* LGTM with the one nit below.  I agree with you that
this'll only get UMA for downloads (including programmatic downloads) and not
save package.  UNTHROTTLED_COUNT should be in the same camp, which will
theoretically allow us to get the stat as to what fraction of downloads have the
content-disposition header (which I think is somewhat valuable, though not
directly relevant to this issue).

https://codereview.chromium.org/11478034/diff/2001/content/browser/download/d...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/2001/content/browser/download/d...
content/browser/download/download_stats.cc:309: result.has_ext_filename));
Isn't this contradictory to the comment in the enum that nothing else will be
recorded if !is_valid?  You may be sure that the parsing won't produce any other
true booleans if these three booleans are false, but that doesn't seem wise to
rely on at this point in the code.

[asanka, 2012-12-14 01:28:01]

rdsmith: Thanks! The statistics will be a bit more than what we need for the
issue.

rvargas: This is the dependent CL for https://codereview.chromium.org/11471041/.
Would you mind taking a look at this as well?

https://codereview.chromium.org/11478034/diff/2001/content/browser/download/d...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/2001/content/browser/download/d...
content/browser/download/download_stats.cc:309: result.has_ext_filename));
On 2012/12/12 16:10:36, rdsmith wrote:
> Isn't this contradictory to the comment in the enum that nothing else will be
> recorded if !is_valid?  You may be sure that the parsing won't produce any
other
> true booleans if these three booleans are false, but that doesn't seem wise to
> rely on at this point in the code.

Done.

[rvargas (doing something else), 2012-12-14 22:49:54]

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
content/browser/download/download_stats.cc:297: void
RecordDownloadContentDisposition(
Isn't it better to generate the histograms directly from net? That would also be
cleaner.

Maybe I'm missing the reason for getting this only for a download.

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
File net/http/http_content_disposition.h (right):

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
net/http/http_content_disposition.h:43: // quoted-string contained non-ASCII
characters.
nit: I know why you do it this way, but do you mind capitalizing Quoted-string?
And using contains instead of contained

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
File net/http/http_content_disposition_unittest.cc (right):

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
net/http/http_content_disposition_unittest.cc:516: enum ParseResultFlags {
Funny... I was actually thinking that I would have used something like this
instead of 8 bools for the actual interface.

[asanka, 2012-12-15 01:57:32]

Thanks!

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
content/browser/download/download_stats.cc:297: void
RecordDownloadContentDisposition(
On 2012/12/14 22:49:54, rvargas wrote:
> Isn't it better to generate the histograms directly from net? That would also
be
> cleaner.
> 
> Maybe I'm missing the reason for getting this only for a download.

The data gathered here would be used to decide whether or not to deprecate some
non-standard features we support ('name' attribute, non-ascii quoted-strings,
RFC 2047 and percent encoding) and in general to measure the use of the C-D
header.

The question we are asking is: "would removing this feature adversely affect the
handling of downloads?" This is why we are only looking at requests that result
in a download.

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
File net/http/http_content_disposition.h (right):

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
net/http/http_content_disposition.h:43: // quoted-string contained non-ASCII
characters.
On 2012/12/14 22:49:54, rvargas wrote:
> nit: I know why you do it this way, but do you mind capitalizing
Quoted-string?
> And using contains instead of contained

Done.

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
File net/http/http_content_disposition_unittest.cc (right):

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
net/http/http_content_disposition_unittest.cc:516: enum ParseResultFlags {
On 2012/12/14 22:49:54, rvargas wrote:
> Funny... I was actually thinking that I would have used something like this
> instead of 8 bools for the actual interface.

Yeah. I changed the interface to use this. :)

[rvargas (doing something else), 2012-12-15 02:46:57]

lgtm

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
content/browser/download/download_stats.cc:297: void
RecordDownloadContentDisposition(
On 2012/12/15 01:57:32, asanka wrote:
> On 2012/12/14 22:49:54, rvargas wrote:
> > Isn't it better to generate the histograms directly from net? That would
also
> be
> > cleaner.
> > 
> > Maybe I'm missing the reason for getting this only for a download.
> 
> The data gathered here would be used to decide whether or not to deprecate
some
> non-standard features we support ('name' attribute, non-ascii quoted-strings,
> RFC 2047 and percent encoding) and in general to measure the use of the C-D
> header.
> 
> The question we are asking is: "would removing this feature adversely affect
the
> handling of downloads?" This is why we are only looking at requests that
result
> in a download.

But that can be answered by generating the same histograms for every C-D that we
see at the net, no?. I was just thinking that doing it in net would eliminate
the whole need of the struct (now enum... I mean translation to and from it),
because all would be done from within a single cc file. That would still seem
better to me, unless there is something that breaks that I'm not seeing.

But I don't want to bike-shed the review :)

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
File net/http/http_content_disposition_unittest.cc (right):

https://codereview.chromium.org/11478034/diff/10002/net/http/http_content_dis...
net/http/http_content_disposition_unittest.cc:516: enum ParseResultFlags {
On 2012/12/15 01:57:32, asanka wrote:
> On 2012/12/14 22:49:54, rvargas wrote:
> > Funny... I was actually thinking that I would have used something like this
> > instead of 8 bools for the actual interface.
> 
> Yeah. I changed the interface to use this. :)

wow, I was not expecting you to change it.

[asanka, 2012-12-16 18:32:04]

Thanks!

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/11478034/diff/10002/content/browser/download/...
content/browser/download/download_stats.cc:297: void
RecordDownloadContentDisposition(
On 2012/12/15 02:46:57, rvargas wrote:
> On 2012/12/15 01:57:32, asanka wrote:
> > On 2012/12/14 22:49:54, rvargas wrote:
> > > Isn't it better to generate the histograms directly from net? That would
> also
> > be
> > > cleaner.
> > > 
> > > Maybe I'm missing the reason for getting this only for a download.
> > 
> > The data gathered here would be used to decide whether or not to deprecate
> some
> > non-standard features we support ('name' attribute, non-ascii
quoted-strings,
> > RFC 2047 and percent encoding) and in general to measure the use of the C-D
> > header.
> > 
> > The question we are asking is: "would removing this feature adversely affect
> the
> > handling of downloads?" This is why we are only looking at requests that
> result
> > in a download.
> 
> But that can be answered by generating the same histograms for every C-D that
we
> see at the net, no?. I was just thinking that doing it in net would eliminate
> the whole need of the struct (now enum... I mean translation to and from it),
> because all would be done from within a single cc file. That would still seem
> better to me, unless there is something that breaks that I'm not seeing.
> 
> But I don't want to bike-shed the review :)

HttpContentDisposition is instantiated in multiple locations. So the UMA would
have to be controlled from outside the class. I chose to do so after the request
has been recognized as a download. However, since that decision is made outside
of net/, the UMA also moved there.

Given that the C-D header is used primarily to control downloads, generating
histogram data for every C-D seen would give us roughly the same data. But then
if we see an apparently useless header like 'Content-Disposition: filename=foo',
we would have a hard time deciding whether to ignore it (since the
disposition-type is assumed to be 'inline' and the filename is ignored), or log
it (because it might be the result of a 'Save As' or treated as a download due
to the MIME type, in which case the filename is significant).

[commit-bot: I haz the power, 2012-12-16 22:01:59]

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

[commit-bot: I haz the power, 2012-12-17 00:16:55]

Change committed as 173403