Add metrics to track HTTP/0.9 usage for main frames and subresources

Add metrics to track HTTP/0.9 usage for main frames and subresources

This will tell us how many more error pages we'll be showing if we
remove support for it.

BUG=624462
Committed: https://crrev.com/8210acc6749feb3d372cd1aa0456eabe029a03af
Cr-Commit-Position: refs/heads/master@{#404673}

[mmenke, 2016-06-30 20:10:22]

mmenke@chromium.org changed reviewers:
+ bnc@chromium.org, holte@chromium.org

[mmenke, 2016-06-30 20:10:23]

bnc:  Please review everything.
holte:  Please review histogram changes.

I'd love to get this reviewed this week, since we'll need it in M53, regardless,
but I don't think it's the end of the world if I have to merge it a couple weeks
down the road.

[Steven Holte, 2016-06-30 21:11:08]

histograms lgtm

[Bence, 2016-06-30 21:20:11]

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:712: if
(request_->response_info().network_accessed) {
Why is Net.HttpResponseInfo.ConnectionInfo only recorded if
response_info.network_accessed, but
Net.HttpResponseInfo.ConnectionInfo.MainFrame and
Net.HttpResponseInfo.ConnectionInfo.SubResource are recorded regardless of
response_info.network_accessed?

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:713:
UMA_HISTOGRAM_ENUMERATION("Net.HttpResponseInfo.ConnectionInfo",
Since you are changing enum ConnectionInfo, I propose that you obsolete
histogram Net.HttpResponseInfo.ConnectionInfo and either create a new histogram
Net.HttpResponseInfo.ConnectionInfo2, or simply delete lines 712--716.

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:721:
UMA_HISTOGRAM_ENUMERATION("Net.HttpResponseInfo.ConnectionInfo.SubResource",
I think you got the two branches backwards.

https://codereview.chromium.org/2119623002/diff/40001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/2119623002/diff/40001/net/http/http_response_...
net/http/http_response_info.h:37: CONNECTION_INFO_HTTP1 = 1,  // HTTP/1.0 or
HTTP/1.1.
Since you are touching this part of the code anyway, I think it might be
worthwhile to split this further into separate HTTP/1.0 and HTTP/1.1 values.

[mmenke, 2016-06-30 22:18:06]

mmenke@chromium.org changed reviewers:
+ juliatuttle@chromium.org

[mmenke, 2016-06-30 22:18:07]

[+juliatuttle] Please review domain_reliability.

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:712: if
(request_->response_info().network_accessed) {
On 2016/06/30 21:20:11, Bence wrote:
> Why is Net.HttpResponseInfo.ConnectionInfo only recorded if
> response_info.network_accessed, but
> Net.HttpResponseInfo.ConnectionInfo.MainFrame and
> Net.HttpResponseInfo.ConnectionInfo.SubResource are recorded regardless of
> response_info.network_accessed?

I've gone back and forth on this numerous times:  Do we want all requests,
HTTP/HTTPS requests, or requests that went over the network.  I really want to
get at how much disabling HTTP/0.9 breaks, but it's not really clear what's the
most accurate count of that.

I'll go ahead and remove the old histogram, per below comment, and put the new
ones in this branch.

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:713:
UMA_HISTOGRAM_ENUMERATION("Net.HttpResponseInfo.ConnectionInfo",
On 2016/06/30 21:20:11, Bence wrote:
> Since you are changing enum ConnectionInfo, I propose that you obsolete
> histogram Net.HttpResponseInfo.ConnectionInfo and either create a new
histogram
> Net.HttpResponseInfo.ConnectionInfo2, or simply delete lines 712--716.

Removed and obsoleted.

https://codereview.chromium.org/2119623002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:721:
UMA_HISTOGRAM_ENUMERATION("Net.HttpResponseInfo.ConnectionInfo.SubResource",
On 2016/06/30 21:20:11, Bence wrote:
> I think you got the two branches backwards.

Thanks for catching that!  Done.

https://codereview.chromium.org/2119623002/diff/40001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/2119623002/diff/40001/net/http/http_response_...
net/http/http_response_info.h:37: CONNECTION_INFO_HTTP1 = 1,  // HTTP/1.0 or
HTTP/1.1.
On 2016/06/30 21:20:11, Bence wrote:
> Since you are touching this part of the code anyway, I think it might be
> worthwhile to split this further into separate HTTP/1.0 and HTTP/1.1 values.

Done.  They're similar enough to each other, that I wasn't sure it was worth the
effort, but it doesn't hurt.  Did turn up a case where we're now randomly
returning one of the three.

https://codereview.chromium.org/2119623002/diff/80001/net/http/http_basic_str...
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/2119623002/diff/80001/net/http/http_basic_str...
net/http/http_basic_stream.cc:129: details->connection_info =
HttpResponseInfo::CONNECTION_INFO_HTTP1_1;
This is really weird, and should be fixed.  Unfortunately, we have no access to
the HttpResponseInfo here.  We do own the HttpStreamParser, but it doesn't
actually even own the structure, so by accessing it here, we'd be adding
lifetime requirements...And the one consumer of this data just wants to know if
this is HTTP/0.9 or HTTP/1.x.

[mmenke, 2016-06-30 22:19:19]

https://codereview.chromium.org/2119623002/diff/80001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/2119623002/diff/80001/net/http/http_response_...
net/http/http_response_info.h:45: CONNECTION_INFO_HTTP1_0 = 9,
I'm keeping the weird order here, so obsolete histograms are kinda sort correct.

[Bence, 2016-06-30 22:44:59]

Thank you.  net/ LGTM.  Note that I'm not a content owner either.

[mmenke, 2016-06-30 23:54:27]

On 2016/06/30 22:44:59, Bence wrote:
> Thank you.  net/ LGTM.  Note that I'm not a content owner either.

I am the content/browser/loader owner, though, so you don't need to be. :)

[mmenke, 2016-06-30 23:54:57]

On 2016/06/30 23:54:27, mmenke wrote:
> On 2016/06/30 22:44:59, Bence wrote:
> > Thank you.  net/ LGTM.  Note that I'm not a content owner either.
> 
> I am the content/browser/loader owner, though, so you don't need to be. :)

An owner, rather.  Forgot I added Randy.

[Bence, 2016-07-01 00:32:06]

On 2016/06/30 23:54:57, mmenke wrote:
> On 2016/06/30 23:54:27, mmenke wrote:
> > On 2016/06/30 22:44:59, Bence wrote:
> > > Thank you.  net/ LGTM.  Note that I'm not a content owner either.
> > 
> > I am the content/browser/loader owner, though, so you don't need to be. :)
> 
> An owner, rather.  Forgot I added Randy.

Oh, thank makes things easy.  Sweet.

[Julia Tuttle, 2016-07-01 15:48:03]

domain_reliability lgtm.

[mmenke, 2016-07-11 15:18:22]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-07-11 15:18:22]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2119623002/#ps80001
(title: "Response to comments")

[commit-bot: I haz the power, 2016-07-11 15:18:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 16:35:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-11 16:35:20]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 16:37:20]

Description was changed from

==========
Add metrics to track HTTP/0.9 usage for main frames and subresources

This will tell us how many more error pages we'll be showing if we
remove support for it.

BUG=624462
==========

to

==========
Add metrics to track HTTP/0.9 usage for main frames and subresources

This will tell us how many more error pages we'll be showing if we
remove support for it.

BUG=624462

Committed: https://crrev.com/8210acc6749feb3d372cd1aa0456eabe029a03af
Cr-Commit-Position: refs/heads/master@{#404673}
==========

[commit-bot: I haz the power, 2016-07-11 16:37:21]

Patchset 5 (id:??) landed as
https://crrev.com/8210acc6749feb3d372cd1aa0456eabe029a03af
Cr-Commit-Position: refs/heads/master@{#404673}