Log mixed script shield events.

Log mixed script shield events.

Also removes the MixedScript_LoadAnyway_Bubble count (unowned, no longer in histograms.xml).

BUG=413057
Committed: https://crrev.com/79ae9aefbde690aac06d7d37d8ec1d3e43389574
Cr-Commit-Position: refs/heads/master@{#302176}

[lgarron, 2014-10-14 23:56:05]

Patchset #1 (id:1) has been deleted

[lgarron, 2014-10-14 23:59:53]

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

[felt, 2014-10-16 00:49:09]

lgtm!

[lgarron, 2014-10-16 01:03:15]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2014-10-16 01:04:26]

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

[lgarron, 2014-10-16 01:10:24]

The CQ bit was unchecked by lgarron@chromium.org

[lgarron, 2014-10-16 01:10:49]

The CQ bit was unchecked by lgarron@chromium.org

[lgarron, 2014-10-16 01:14:53]

lgarron@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org, sky@chromium.org

[lgarron, 2014-10-16 01:19:51]

Would all 'yall mind reviewing files as follows, please? :-D

jochen:
  - chrome/browser/content_settings/tab_specific_content_settings.cc
  - chrome/browser/content_settings/tab_specific_content_settings.h
  - chrome/browser/ui/content_settings/content_setting_bubble_model.cc
  - Perhaps the other two files, just to make sure?

sky:
  - chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc

isherman:
  - tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2014-10-16 03:24:51]

https://codereview.chromium.org/659573003/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/659573003/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:332:
MIXEDSCRIPT_CONTENT_NUM_EVENTS);
Please define a wrapper function that emits to this histogram, that you'd invoke
as something like "RecordMixedScriptAction(MIXEDSCRIPT_DISPLAYED_SHIELD)".  This
helps to reduce the chances that you've got a small typo in one of the several
similar code paths, and other such subtle bugs.

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

https://codereview.chromium.org/659573003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:39831: +  </int>
Optional nit: I would suggest writing these as something like:

<int value="0" label="Displayed shield"/>
<int value="1" label="Displayed bubble"/>
<int value="2" label="Clicked allow"/>
<int value="3" label="Clicked learn more"/>

IMO, that's easier for a dashboard user to read than ALL_CAPS_LABELS, and most
users won't ever see the on-hover text.

[sky, 2014-10-16 16:05:31]

chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc LGTM

[lgarron, 2014-10-16 22:00:54]

Ilya asked me to refactor the logging.
sky, would you mind sanity checking the new RecordMixedScriptAction call?

jochen:
  - chrome/browser/content_settings/tab_specific_content_settings.cc
  - chrome/browser/content_settings/tab_specific_content_settings.h
  - chrome/browser/ui/content_settings/content_setting_bubble_model.cc
  - Perhaps the other two files, just to make sure?

sky:
  - chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc

isherman:
  - tools/metrics/histograms/histograms.xml

Thanks,
»Lucas

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

https://codereview.chromium.org/659573003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:39831: +  </int>
On 2014/10/16 03:24:51, Ilya Sherman wrote:
> Optional nit: I would suggest writing these as something like:
> 
> <int value="0" label="Displayed shield"/>
> <int value="1" label="Displayed bubble"/>
> <int value="2" label="Clicked allow"/>
> <int value="3" label="Clicked learn more"/>
> 
> IMO, that's easier for a dashboard user to read than ALL_CAPS_LABELS, and most
> users won't ever see the on-hover text.

Sure. I wanted to be thorough, but I suppose someone like me could probably
track down the source of the metric in the source if they needed to, regardless
of how detailed the labels are.

[lgarron, 2014-10-16 22:02:58]

Patchset #2 (id:40001) has been deleted

[Ilya Sherman, 2014-10-16 22:05:13]

LGTM % a nit.  Thanks :)

https://codereview.chromium.org/659573003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/659573003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:67: static void
RecordMixedScriptAction(int action);
nit: Please give the enum a name, and set the variable type to match the enum.

[lgarron, 2014-10-16 22:15:05]

Now with 100% more enum naming.

[Ilya Sherman, 2014-10-16 22:20:36]

https://codereview.chromium.org/659573003/diff/40002/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/659573003/diff/40002/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:64:
MIXEDSCRIPT_NUM_ACTIONS
FYI, the typical style is, if the enum name is FooBar, to have the enum values
be named FOO_BAR_APPLE, FOO_BAR_BANANA, etc.  So, in this case, I'd expect the
enum value names to start with the prefix "MIXED_SCRIPT_ACTION_".

(In theory, C++11 enum classes could provide actually namespacing, so that we
wouldn't need this ugly naming convention.  IIRC, that feature is still under
discussion on chromium-dev.)

[lgarron, 2014-10-16 22:24:30]

https://codereview.chromium.org/659573003/diff/40002/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/659573003/diff/40002/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:64:
MIXEDSCRIPT_NUM_ACTIONS
On 2014/10/16 22:20:35, Ilya Sherman wrote:
> FYI, the typical style is, if the enum name is FooBar, to have the enum values
> be named FOO_BAR_APPLE, FOO_BAR_BANANA, etc.  So, in this case, I'd expect the
> enum value names to start with the prefix "MIXED_SCRIPT_ACTION_".
> 
> (In theory, C++11 enum classes could provide actually namespacing, so that we
> wouldn't need this ugly naming convention.  IIRC, that feature is still under
> discussion on chromium-dev.)

Man, apparently I can't get nuttin' right, even if I consult the style guide.
:-P

I consolidated MIXEDSCRIPT into one word because that's how
CONTENT_TYPE_MIXEDSCRIPT does it, but I'll change it over now.

[Ilya Sherman, 2014-10-16 22:43:56]

LGTM, no more nits.  Thanks, Lucas :)

[sky, 2014-10-16 23:39:44]

LGTM

https://codereview.chromium.org/659573003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/659573003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:337: if (type
== CONTENT_SETTINGS_TYPE_MIXEDSCRIPT) {
nit: no {}

https://codereview.chromium.org/659573003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/659573003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:67: static void
RecordMixedScriptAction(MixedScriptAction action);
This should be after the destructor (see style guide for specific order) (make
sure you implementation matches position too).

[lgarron, 2014-10-17 02:31:30]

Patchset #4 (id:80001) has been deleted

[lgarron, 2014-10-17 02:32:27]

Updated to assuage sky's style concerns.

[lgarron, 2014-10-22 00:03:52]

jochen: ping

[lgarron, 2014-10-29 22:20:55]

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

[lgarron, 2014-10-29 22:20:55]

jochen is OOO now; bauerb: Would you mind reviewing the content_settings files?

  - chrome/browser/content_settings/tab_specific_content_settings.cc
  - chrome/browser/content_settings/tab_specific_content_settings.h
  - chrome/browser/ui/content_settings/content_setting_bubble_model.cc

[Bernhard Bauer, 2014-10-30 13:36:20]

lgtm

[lgarron, 2014-10-30 21:37:20]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2014-10-30 21:38:16]

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

[commit-bot: I haz the power, 2014-10-30 22:40:25]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2014-10-30 22:41:55]

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