Add some histograms for <a ping> requests.

Add some histograms for <a ping> requests.

So we can see if we're hitting the timeout, how long requests take, etc.

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

[davidben, 2014-01-27 20:40:39]

mmenke: Main review since you reviewed DetachableResourceHandler.
asvitkine: For histograms.xml OWNERS.
jam: content/ OWNERS

samarth, tburkard: FYI

[jam, 2014-01-27 21:35:17]

On 2014/01/27 20:40:39, David Benjamin wrote:
> mmenke: Main review since you reviewed DetachableResourceHandler.
> asvitkine: For histograms.xml OWNERS.
> jam: content/ OWNERS
> 
> samarth, tburkard: FYI

owner lgtm

[mmenke, 2014-01-27 22:51:54]

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:20: // Enum type for <a
ping> result.
Think you should mention histograms, and new results being added to the end. 
I'm paranoid.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:36: }
nit:  "}  // namespace"

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:63: if (response_started_)
{
optional:  Just out of curiousity/paranoia / whatever, maybe log the
successfully completed ones separately from those that just started?  I don't
expect a whole lot with start and not (response_completed_ + status is OK), but
just seems a little odd not to separate them out.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:66: result_type =
UMA_PING_RESULT_TYPE_TIMEDOUT;
Shouldn't we end up in the CANCELED bucket here?  If so, I think it's a bit more
consistent to put this below the response_completed_ test, just before the
canceled one.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:67: } else if
(!response_completed_) {
I think response_completed_ is a confusing name here, since at this point, it
means the request "completed" with an error message, without a response ever
being started.

Maybe just a comment about the weirdness?

[davidben, 2014-01-28 19:52:40]

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:20: // Enum type for <a
ping> result.
On 2014/01/27 22:51:54, mmenke wrote:
> Think you should mention histograms, and new results being added to the end. 
> I'm paranoid.

Done.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:36: }
On 2014/01/27 22:51:54, mmenke wrote:
> nit:  "}  // namespace"

Done.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:63: if (response_started_)
{
On 2014/01/27 22:51:54, mmenke wrote:
> optional:  Just out of curiousity/paranoia / whatever, maybe log the
> successfully completed ones separately from those that just started?  I don't
> expect a whole lot with start and not (response_completed_ + status is OK),
but
> just seems a little odd not to separate them out.

Done.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:66: result_type =
UMA_PING_RESULT_TYPE_TIMEDOUT;
On 2014/01/27 22:51:54, mmenke wrote:
> Shouldn't we end up in the CANCELED bucket here?  If so, I think it's a bit
more
> consistent to put this below the response_completed_ test, just before the
> canceled one.

Done.

https://codereview.chromium.org/148573002/diff/40001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:67: } else if
(!response_completed_) {
On 2014/01/27 22:51:54, mmenke wrote:
> I think response_completed_ is a confusing name here, since at this point, it
> means the request "completed" with an error message, without a response ever
> being started.
> 
> Maybe just a comment about the weirdness?

Added a comment.

[Alexei Svitkine (slow), 2014-01-28 20:01:08]

lgtm

[mmenke, 2014-01-28 20:02:47]

LGTM

https://codereview.chromium.org/148573002/diff/80001/content/browser/loader/d...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/148573002/diff/80001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:75: // The request was
aborted without OnResponseCompleted before any response
optional:  aborted -> deleted? ("Aborted" sounds too much like "ERR_ABORTED",
which would result in CANCELED).

[davidben, 2014-01-28 20:10:27]

https://codereview.chromium.org/148573002/diff/80001/content/browser/loader/d...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/148573002/diff/80001/content/browser/loader/d...
content/browser/loader/detachable_resource_handler.cc:75: // The request was
aborted without OnResponseCompleted before any response
On 2014/01/28 20:02:47, mmenke wrote:
> optional:  aborted -> deleted? ("Aborted" sounds too much like "ERR_ABORTED",
> which would result in CANCELED).

Done.

[commit-bot: I haz the power, 2014-01-28 20:13:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/148573002/100001

[commit-bot: I haz the power, 2014-01-29 02:00:25]

Change committed as 247587