Use CertificateReportSender error callback to record HPKP UMA metrics

Use CertificateReportSender error callback to record HPKP UMA metrics

This CL does two things:
1. Use the CertificateReportSender error callback to record UMA metrics
when we fail to send an HPKP report.
2. The error callback argument of a URLRequest* wasn't set up quite
right to do this; TransportSecurityState provides the callback, but
TransportSecurityState is in net/http and shouldn't depend on
URLRequest. So instead I changed the callback arguments to be the report
URI and the net error from sending the report.

BUG=596624
Committed: https://crrev.com/f2979127c4951011a74f00fab8fdff94d87952de
Cr-Commit-Position: refs/heads/master@{#385198}

[estark, 2016-04-02 00:06:02]

estark@chromium.org changed reviewers:
+ eroman@chromium.org

[estark, 2016-04-02 00:06:03]

eroman, PTAL?

[eroman, 2016-04-02 01:55:25]

lgtm

https://codereview.chromium.org/1850853004/diff/20001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1850853004/diff/20001/net/http/transport_secu...
net/http/transport_security_state.h:209: const base::Callback<void(GURL, int)>&
error_callback) = 0;
why not a const GURL?

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
File net/url_request/certificate_report_sender.h (right):

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender.h:33: typedef
base::Callback<void(GURL, int)> ErrorCallback;
(1) Can use "using" instead
(2) Why not const GURL&

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender.h:49: // |error_callback| is called
with a pointer to the URLRequest as an
Update comment

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
File net/url_request/certificate_report_sender_unittest.cc (right):

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender_unittest.cc:51: // called, it sets
|called| to true.
Maybe rephrase this comment to just saying it sets *called to true.

https://codereview.chromium.org/1850853004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1850853004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27049: +    &quot;pinning&quot;
requirements on a per-domain basis. When pinning
not sure that quote marks are needed around pinning.

[estark, 2016-04-05 00:57:29]

estark@chromium.org changed reviewers:
+ mpearson@chromium.org

[estark, 2016-04-05 00:57:30]

Thanks eroman!

mpearson: can you please review histograms.xml?

https://codereview.chromium.org/1850853004/diff/20001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1850853004/diff/20001/net/http/transport_secu...
net/http/transport_security_state.h:209: const base::Callback<void(GURL, int)>&
error_callback) = 0;
On 2016/04/02 01:55:25, eroman wrote:
> why not a const GURL?

Done.

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
File net/url_request/certificate_report_sender.h (right):

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender.h:33: typedef
base::Callback<void(GURL, int)> ErrorCallback;
On 2016/04/02 01:55:25, eroman wrote:
> (1) Can use "using" instead
> (2) Why not const GURL&

Done.

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender.h:49: // |error_callback| is called
with a pointer to the URLRequest as an
On 2016/04/02 01:55:25, eroman wrote:
> Update comment

Done.

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
File net/url_request/certificate_report_sender_unittest.cc (right):

https://codereview.chromium.org/1850853004/diff/20001/net/url_request/certifi...
net/url_request/certificate_report_sender_unittest.cc:51: // called, it sets
|called| to true.
On 2016/04/02 01:55:25, eroman wrote:
> Maybe rephrase this comment to just saying it sets *called to true.

Done.

https://codereview.chromium.org/1850853004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1850853004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27049: +    &quot;pinning&quot;
requirements on a per-domain basis. When pinning
On 2016/04/02 01:55:25, eroman wrote:
> not sure that quote marks are needed around pinning.

Done.

[Mark P, 2016-04-05 16:58:21]

histograms.xml lgtm

Thanks for the particularly clear histogram description.

--mark

[estark, 2016-04-05 16:59:01]

The CQ bit was checked by estark@chromium.org

[estark, 2016-04-05 16:59:01]

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

[commit-bot: I haz the power, 2016-04-05 16:59:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1850853004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1850853004/60001

[commit-bot: I haz the power, 2016-04-05 17:04:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-05 17:06:16]

Description was changed from

==========
Use CertificateReportSender error callback to record HPKP UMA metrics

This CL does two things:
1. Use the CertificateReportSender error callback to record UMA metrics
when we fail to send an HPKP report.
2. The error callback argument of a URLRequest* wasn't set up quite
right to do this; TransportSecurityState provides the callback, but
TransportSecurityState is in net/http and shouldn't depend on
URLRequest. So instead I changed the callback arguments to be the report
URI and the net error from sending the report.

BUG=596624
==========

to

==========
Use CertificateReportSender error callback to record HPKP UMA metrics

This CL does two things:
1. Use the CertificateReportSender error callback to record UMA metrics
when we fail to send an HPKP report.
2. The error callback argument of a URLRequest* wasn't set up quite
right to do this; TransportSecurityState provides the callback, but
TransportSecurityState is in net/http and shouldn't depend on
URLRequest. So instead I changed the callback arguments to be the report
URI and the net error from sending the report.

BUG=596624

Committed: https://crrev.com/f2979127c4951011a74f00fab8fdff94d87952de
Cr-Commit-Position: refs/heads/master@{#385198}
==========

[commit-bot: I haz the power, 2016-04-05 17:06:17]

Patchset 4 (id:??) landed as
https://crrev.com/f2979127c4951011a74f00fab8fdff94d87952de
Cr-Commit-Position: refs/heads/master@{#385198}