Add metrics for 'upgrade-insecure-requests'.

Add metrics for 'upgrade-insecure-requests'.

BUG=455674
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192541

[Mike West, 2015-03-25 12:12:10]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2015-03-25 12:12:10]

Mind taking a look, Yoav?

[Mike West, 2015-03-25 12:12:46]

https://codereview.chromium.org/1035683003/diff/1/Source/core/frame/csp/Conte...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1035683003/diff/1/Source/core/frame/csp/Conte...
Source/core/frame/csp/ContentSecurityPolicy.cpp:180: if
(m_insecureRequestsPolicy == SecurityContext::InsecureRequestsUpgrade) {
`>` made sense when reporting was bundled up with this feature. Now that it's
not, it doesn't.

[Yoav Weiss, 2015-03-25 13:38:35]

On 2015/03/25 12:12:46, Mike West wrote:
>
https://codereview.chromium.org/1035683003/diff/1/Source/core/frame/csp/Conte...
> File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):
> 
>
https://codereview.chromium.org/1035683003/diff/1/Source/core/frame/csp/Conte...
> Source/core/frame/csp/ContentSecurityPolicy.cpp:180: if
> (m_insecureRequestsPolicy == SecurityContext::InsecureRequestsUpgrade) {
> `>` made sense when reporting was bundled up with this feature. Now that it's
> not, it doesn't.

LGTM.

We're counting the documents that have the upgrade directive here, right?

Might also be interesting to count requests that actually need it and would
result in mixed content otherwise. (although the main diff between the numbers
would be sites that turn it on, but don't really use it. Maybe not that
interesting...)

[Mike West, 2015-03-25 13:43:34]

On 2015/03/25 at 13:38:35, yoav wrote:
> We're counting the documents that have the upgrade directive here, right?

We're counting the documents that enable the feature, right.

> Might also be interesting to count requests that actually need it and would
result in mixed content otherwise. (although the main diff between the numbers
would be sites that turn it on, but don't really use it. Maybe not that
interesting...)

We have `MixedContentPresent`, which I hope we'll end up driving down with this
feature. I guess you're asking for
`MixedContentWouldHaveBeenPresentButWeUpgradedInstead`? Wouldn't hurt, I guess,
but it's not clear what data that would give us. If you think it's worth adding,
I will.

[Mike West, 2015-03-25 13:48:25]

WDYT of the new metric/names?

[Yoav Weiss, 2015-03-25 13:56:48]

On 2015/03/25 13:48:25, Mike West wrote:
> WDYT of the new metric/names?

Catchy :) Still LGTM.

I think it's good to keep track of both enabling and the actual requests, since
it would help us answer questions like "how much traffic would actually break if
we unship this?"

[Mike West, 2015-03-25 13:58:48]

On 2015/03/25 at 13:56:48, yoav wrote:
> On 2015/03/25 13:48:25, Mike West wrote:
> > WDYT of the new metric/names?
> 
> Catchy :) Still LGTM.
> 
> I think it's good to keep track of both enabling and the actual requests,
since it would help us answer questions like "how much traffic would actually
break if we unship this?"

0%, because we send a feature detection header!

But yeah, let's figure out what the impact is.

[Mike West, 2015-03-25 13:59:02]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-03-25 13:59:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1035683003/20001

[commit-bot: I haz the power, 2015-03-25 15:15:00]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192541