Add a RAPPOR metric to track where users click to allow mixed scripts.

Add a RAPPOR metric to track where users click to allow mixed scripts.

As a side effect, move mixed script shield RAPPOR stats into a new content settings util function, RecordMixedScriptActionWithRAPPOR().

BUG=490462
Committed: https://crrev.com/6be9a489fa3ec17a0173e88027b7feaf53ff8caa
Cr-Commit-Position: refs/heads/master@{#334344}

[lgarron, 2015-05-22 02:56:20]

Patchset #1 (id:1) has been deleted

[lgarron, 2015-05-22 03:10:47]

lgarron@chromium.org changed reviewers:
+ felt@chromium.org, holte@chromium.org

[lgarron, 2015-05-22 03:10:48]

A better place to track this might be where we measure SSL.InsecureContent in
ContentSettingsObserver::allowRunningInsecureContent() [1], but that's in the
renderer, and the RAPPOR service is in the browser.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/co...

[lgarron, 2015-05-22 03:17:18]

Patchset #1 (id:20001) has been deleted

[felt, 2015-05-22 20:59:34]

Why not do this inside RecordMixedScriptAction?

https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1009: //
During an interstitial, the shield applies to the last committed URL.
what is this comment here for? to explain why you're using GetLastCommittedURL
instead of GetVisibleURL?

[lgarron, 2015-05-22 21:04:22]

On 2015/05/22 at 20:59:34, felt wrote:
> Why not do this inside RecordMixedScriptAction?
> 
>
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
(right):
> 
>
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1009: //
During an interstitial, the shield applies to the last committed URL.
> what is this comment here for? to explain why you're using GetLastCommittedURL
instead of GetVisibleURL?

RecordMixedScriptAction() doesn't know the domain, but this is the (only) place
where RecordMixedScriptAction() is called.
Would you prefer to pass the URL to RecordMixedScriptAction() instead?

[felt, 2015-05-22 21:14:25]

On 2015/05/22 21:04:22, lgarron wrote:
> On 2015/05/22 at 20:59:34, felt wrote:
> > Why not do this inside RecordMixedScriptAction?
> > 
> >
>
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1009: //
> During an interstitial, the shield applies to the last committed URL.
> > what is this comment here for? to explain why you're using
GetLastCommittedURL
> instead of GetVisibleURL?
> 
> RecordMixedScriptAction() doesn't know the domain, but this is the (only)
place
> where RecordMixedScriptAction() is called.
> Would you prefer to pass the URL to RecordMixedScriptAction() instead?

It seems like it makes sense to do them in the same place. And passing in the
URL doesn't seem like a problem.

[lgarron, 2015-05-26 19:09:36]

On 2015/05/22 at 21:14:25, felt wrote:
> On 2015/05/22 21:04:22, lgarron wrote:
> > On 2015/05/22 at 20:59:34, felt wrote:
> > > Why not do this inside RecordMixedScriptAction?
> > > 
> > >
> >
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> > > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
> > > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1009:
//
> > During an interstitial, the shield applies to the last committed URL.
> > > what is this comment here for? to explain why you're using
GetLastCommittedURL
> > instead of GetVisibleURL?
> > 
> > RecordMixedScriptAction() doesn't know the domain, but this is the (only)
place
> > where RecordMixedScriptAction() is called.
> > Would you prefer to pass the URL to RecordMixedScriptAction() instead?
> 
> It seems like it makes sense to do them in the same place. And passing in the
URL doesn't seem like a problem.

I've introduced a second function, RecordMixedScriptActionWithRAPPOR(), and
moved the existing MIXED_SCRIPT_ACTION_DISPLAYED_SHIELD metric into it, too.

I didn't reuse RecordMixedScriptAction() because one of the call sites [1]
doesn't have access to the URL, and it felt cleaner to pass in the URL only when
necessary.

[1]
https://chromium.googlesource.com/chromium/src/+/6262a2f534466059a01244152d23...

[lgarron, 2015-06-03 03:19:55]

felt@: post-I/O ping.

https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/1148773005/diff/40001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1009: //
During an interstitial, the shield applies to the last committed URL.
On 2015/05/22 20:59:34, felt wrote:
> what is this comment here for? to explain why you're using GetLastCommittedURL
> instead of GetVisibleURL?

Yeah, exactly. (I would have put the comment below, but up here seemed better.)

[lgarron, 2015-06-11 23:06:03]

felt@: Pretty ping with a cherry on top. :-D

(Assuming you're fine with a separate RecordMixedScriptActionWithRAPPOR()
function, there should be nothing controversial here. It would be nice to merge
this to 44.)

[felt, 2015-06-12 00:18:25]

lgtm % nit

https://codereview.chromium.org/1148773005/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/chrome_content_settings_utils.cc (right):

https://codereview.chromium.org/1148773005/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/chrome_content_settings_utils.cc:20:
RecordMixedScriptAction(action);
A second method seems fine, but can you avoid calling RecordMixedScriptAction
inside of it? It doesn't sound obvious that the WithRAPPOR metric would also do
the UMA. Instead, it's OK to have calls for both right next to each other.

[felt, 2015-06-12 00:18:40]

And very sorry for the delay. >< My bad.

[lgarron, 2015-06-12 00:29:58]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-06-12 00:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1148773005/#ps80001
(title: "Don't call RecordMixedScriptAction() from
RecordMixedScriptActionWithRAPPOR().")

[lgarron, 2015-06-12 00:30:18]

https://codereview.chromium.org/1148773005/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/chrome_content_settings_utils.cc (right):

https://codereview.chromium.org/1148773005/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/chrome_content_settings_utils.cc:20:
RecordMixedScriptAction(action);
On 2015/06/12 at 00:18:25, felt wrote:
> A second method seems fine, but can you avoid calling RecordMixedScriptAction
inside of it? It doesn't sound obvious that the WithRAPPOR metric would also do
the UMA. Instead, it's OK to have calls for both right next to each other.

I chose "with RAPPOR" instead of "using RAPPOR", hoping it would be more clear.
But I don't feel strongly, so I've gone ahead and separated them.

[lgarron, 2015-06-12 00:30:34]

On 2015/06/12 at 00:18:40, felt wrote:
> And very sorry for the delay. >< My bad.

No worries; thanks. :-)

[commit-bot: I haz the power, 2015-06-12 00:31:20]

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

[commit-bot: I haz the power, 2015-06-12 00:39:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-12 00:39:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lgarron, 2015-06-12 00:45:14]

lgarron@chromium.org changed reviewers:
+ bauerb@chromium.org

[lgarron, 2015-06-12 00:45:14]

bauerb: Would you mind reviewing //chrome/browser/content_settings (3 files)?
holte: Would you mind reviewing rappor.xml?

Thanks,
»Lucas

[Steven Holte, 2015-06-13 01:00:13]

lgtm

[Bernhard Bauer, 2015-06-13 10:25:16]

LGTM w/ a nit:

https://codereview.chromium.org/1148773005/diff/80001/chrome/browser/content_...
File chrome/browser/content_settings/chrome_content_settings_utils.h (right):

https://codereview.chromium.org/1148773005/diff/80001/chrome/browser/content_...
chrome/browser/content_settings/chrome_content_settings_utils.h:8: #include
"url/gurl.h"
You can forward-declare GURL without including the header.

[lgarron, 2015-06-13 22:19:51]

Patchset #4 (id:100001) has been deleted

[lgarron, 2015-06-13 22:22:07]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2015-06-13 22:22:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
felt@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1148773005/#ps120001
(title: "Forward-declare GURL.")

[commit-bot: I haz the power, 2015-06-13 22:22:13]

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

[commit-bot: I haz the power, 2015-06-14 00:00:25]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-06-14 00:01:28]

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