Add UMA for the new generic permisison class

Add UMA for the new generic permisison class


In order to track UMA accurately we only need to track it when there is an actual prompt for permission, not when the permission was saved for the domain. I have re-purposed the PermissionDecided method for this which was essentially doing nothing before.

BUG=392145
TBR=fgorski,tommi

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

[Miguel Garcia, 2014-07-07 14:30:06]

Initial non-owner review

[Michael van Ouwerkerk, 2014-07-07 14:45:20]

lgtm with tiny nits

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8255: Triggers when the default decission for
push message permissions is changed.
Nit: s/decission/decision/

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8297: Triggers when the default decission for
push message permissions is changed.
same as above

[Takashi Toyoshima, 2014-07-08 10:46:17]

Thank you for adding nice UMAs.
LGTM!

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

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:139: PERMISSION_NUM);
This is really nice:)

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8252: <action
name="Options_DefaultMIDISysExSettingChanged">
thanks!

[Miguel Garcia, 2014-07-08 12:37:44]

+bernhard for content_settings owners
+mpearson for histograms owners

[Bernhard Bauer, 2014-07-08 12:54:52]

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

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:135:
UMA_HISTOGRAM_ENUMERATION(allowed
You were going to fix this?

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:113: static
PermissionType SettingToPermission(ContentSettingsType permission);
If this method is static and private, do you need it as a class method in the
header, or could it be in an anonymous namespace in the .cc file?

[Miguel Garcia, 2014-07-08 14:14:20]

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

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:135:
UMA_HISTOGRAM_ENUMERATION(allowed
On 2014/07/08 12:54:51, Bernhard Bauer wrote:
> You were going to fix this?

Yes, I had not uploaded the change yet, you review code too fast! :)

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:139: PERMISSION_NUM);
On 2014/07/08 10:46:17, Takashi Toyoshima (chromium) wrote:
> This is really nice:)


Thanks!

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/371933002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:113: static
PermissionType SettingToPermission(ContentSettingsType permission);
On 2014/07/08 12:54:52, Bernhard Bauer wrote:
> If this method is static and private, do you need it as a class method in the
> header, or could it be in an anonymous namespace in the .cc file?

Sure, it does not need to be a method, it cans just be an anonymous function

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8252: <action
name="Options_DefaultMIDISysExSettingChanged">
On 2014/07/08 10:46:17, Takashi Toyoshima (chromium) wrote:
> thanks!

Done.

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8255: Triggers when the default decission for
push message permissions is changed.
On 2014/07/07 14:45:20, Michael van Ouwerkerk wrote:
> Nit: s/decission/decision/

Done.

https://codereview.chromium.org/371933002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8297: Triggers when the default decission for
push message permissions is changed.
On 2014/07/07 14:45:20, Michael van Ouwerkerk wrote:
> same as above

Done.

[Miguel Garcia, 2014-07-08 14:17:13]

+dbeam for browser/ui/webui/options/ OWNERS

[Bernhard Bauer, 2014-07-08 14:51:05]

LGTM (also for WebUI)

https://codereview.chromium.org/371933002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/371933002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:26: static
PermissionType SettingToPermission(ContentSettingsType permission) {
Static is not necessary here.

[Miguel Garcia, 2014-07-08 15:37:44]

https://codereview.chromium.org/371933002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/371933002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:26: static
PermissionType SettingToPermission(ContentSettingsType permission) {
On 2014/07/08 14:51:05, Bernhard Bauer wrote:
> Static is not necessary here.

Done.

[Mark P, 2014-07-08 20:22:15]

Please revise the changelist description now that it doesn't depend on any
unsubmitted changes.

--mark

[Mark P, 2014-07-08 20:49:03]

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

https://codereview.chromium.org/371933002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:26: // add new
pemissions.
Please explicitly use = 0, = 1, and so on.  Also say to never delete or reorder
an entry; only add new entries immediately before PERMISSION_NUM.

https://codereview.chromium.org/371933002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:33: PERMISSION_NUM,
optional nit: consider PERMISSION_MAX

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8255: Triggers when the default decision for
push message permissions is changed.
Why is this description the same as the description to a different action? 
Shouldn't this description mention MIDI...?

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

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2793: +    The default permission
setting for push messages at browser start
Often when someone says "browser start", they mean "profile open".  Are you sure
you mean browser start here?

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2799: +  <summary>Number of times a
given permission was granted.</summary>
What does it mean that a given permission PermissionDenied was granted?
Please rewrite.

analogous comment on both histograms below

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:35915: +<enum name="ContentSetting"
type="int">
Can't you reuse the previous ContentSetting enum that you added yesterday?

Why is the diff showing a new entry?

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36208: +<!-- Needs to be kept in sync
with PermissionType in
Unnecessary.  Please remove.  (Every enum in this file needs to be kept in sync
with something in the code base.)

[Miguel Garcia, 2014-07-09 11:17:19]

Thanks for the review Mark! I think I addressed all your comments, PTAL

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

https://codereview.chromium.org/371933002/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:33: PERMISSION_NUM,
On 2014/07/08 20:49:03, Mark P wrote:
> optional nit: consider PERMISSION_MAX

Done.

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:8255: Triggers when the default decision for
push message permissions is changed.
On 2014/07/08 20:49:03, Mark P wrote:
> Why is this description the same as the description to a different action? 
> Shouldn't this description mention MIDI...?

Indeed

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

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2793: +    The default permission
setting for push messages at browser start
On 2014/07/08 20:49:03, Mark P wrote:
> Often when someone says "browser start", they mean "profile open".  Are you
sure
> you mean browser start here?

Yes, this was a bad merge from the CL where I moved things upstream, changed
now.

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2799: +  <summary>Number of times a
given permission was granted.</summary>
On 2014/07/08 20:49:03, Mark P wrote:
> What does it mean that a given permission PermissionDenied was granted?
> Please rewrite.
> 
> analogous comment on both histograms below

Done.

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:35915: +<enum name="ContentSetting"
type="int">
On 2014/07/08 20:49:03, Mark P wrote:
> Can't you reuse the previous ContentSetting enum that you added yesterday?
> 
> Why is the diff showing a new entry?
Merge conflict, should be fixed in the latest patch

https://codereview.chromium.org/371933002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36208: +<!-- Needs to be kept in sync
with PermissionType in
On 2014/07/08 20:49:03, Mark P wrote:
> Unnecessary.  Please remove.  (Every enum in this file needs to be kept in
sync
> with something in the code base.)

Done.

[Mark P, 2014-07-09 23:18:52]

Now that you have clearer histogram descriptions, I can understand what these
histograms are doing.  Their structure, however, seems odd.  I would think life
would be easier if you had one history for each permission type (e.g.,
MIDI_SYSEX) with values of requested, granted, and denied.  That way if you want
to find out how users reach to a particular permission, they can look at one
histogram and compare the three buckets within it.  Personally, I don't see any
benefit to structuring these histograms the way you do--I don't see the point of
being able to easily notice, say, 5x more users granted push messaging
permission than granted MIDI SysEx permission.  Am I missing something?

Please consider my alternate proposal or argue that the way you're structuring
these histograms is more useful than my suggestion.

If you think my proposal makes sense, I can give you pointers on how to
implement it easily (if you need help).

--mark

[Miguel Garcia, 2014-07-10 09:03:12]

I don't think your suggestion conflicts with this one though individual
permission owners can (and already do) still track per permission metrics. I
would love to be able to automate it but I don't know how to create the labels
automatically, every single new permission would need to update histograms.xml
(or am I missing something?)

While certainly less useful than your proposal these histograms will let us
verify in one look if the permission related features are being used at all or
not. More importantly, comparing closely related permissions (like notifications
and push messaging) will in fact provide useful data as well and let us decide
if we can collapse them for example).

The change for moving the notification permission system into the common one is
coming in https://codereview.chromium.org/376253005/


On 2014/07/09 23:18:52, Mark P wrote:
> Now that you have clearer histogram descriptions, I can understand what these
> histograms are doing.  Their structure, however, seems odd.  I would think
life
> would be easier if you had one history for each permission type (e.g.,
> MIDI_SYSEX) with values of requested, granted, and denied.  That way if you
want
> to find out how users reach to a particular permission, they can look at one
> histogram and compare the three buckets within it.  Personally, I don't see
any
> benefit to structuring these histograms the way you do--I don't see the point
of
> being able to easily notice, say, 5x more users granted push messaging
> permission than granted MIDI SysEx permission.  Am I missing something?
> 
> Please consider my alternate proposal or argue that the way you're structuring
> these histograms is more useful than my suggestion.
> 
> If you think my proposal makes sense, I can give you pointers on how to
> implement it easily (if you need help).
> 
> --mark

[Mark P, 2014-07-10 16:33:32]

On Thu, Jul 10, 2014 at 2:03 AM, <miguelg@chromium.org> wrote:

> I don't think your suggestion conflicts with this one though individual
> permission owners can (and already do) still track per permission metrics.
> I
> would love to be able to automate it but I don't know how to create the
> labels
> automatically,


You can create new histograms automatically using the "histogram suffixes"
ability of histograms.xml.  In particular, this will allow you to generate
n histograms automatically simply from a short list of the n possible
suffixes.  In your case, the suffixes can be the permission type.


> every single new permission would need to update histograms.xml
> (or am I missing something?)
>

Yes, you'd have to modify histograms.xml to add a new permission on the
suffix list.  This is no more onerous than what you currently would have to
do: modify histograms.xml to add a new permission to the enum list of
permission types.


> While certainly less useful than your proposal these histograms will let us
> verify in one look if the permission related features are being used at
> all or
> not. More importantly, comparing closely related permissions (like
> notifications
> and push messaging) will in fact provide useful data as well and let us
> decide
> if we can collapse them for example).
>

You made two claims about why this current structure is okay:
* "these histograms will let us verify in one look if the permission
related features are being used at all".  This is true.  But it's true for
the histograms in my proposal as well.
* "comparing closely related permissions (like notifications and push
messaging) will in fact provide useful data".  I don't buy this argument.
 The way these histograms will be displayed will invite comparison between
permission types, but those comparisons will be misleading without doing
further computation.  For instance, suppose you see that in the
PermissionGranted histogram the number of Notification permissions granted
is tiny compared to the number of Push Messaging permission granted.  This
may make you want to, say, eliminate Notification permissions / combine
them with Push Messaging permissions.  If they often go together, and one
is small, then you might as well eliminate the one, eh?

This would be wrong reasoning.  Comparing the permission granted numbers is
not useful without normalizing those numbers by the number of times those
permissions are requested.



> The change for moving the notification permission system into the common
> one is
> coming in https://codereview.chromium.org/376253005/
>
>
>
> On 2014/07/09 23:18:52, Mark P wrote:
>
>> Now that you have clearer histogram descriptions, I can understand what
>> these
>> histograms are doing.  Their structure, however, seems odd.  I would think
>>
> life
>
>> would be easier if you had one history for each permission type (e.g.,
>> MIDI_SYSEX) with values of requested, granted, and denied.  That way if
>> you
>>
> want
>
>> to find out how users reach to a particular permission, they can look at
>> one
>> histogram and compare the three buckets within it.  Personally, I don't
>> see
>>
> any
>
>> benefit to structuring these histograms the way you do--I don't see the
>> point
>>
> of
>
>> being able to easily notice, say, 5x more users granted push messaging
>> permission than granted MIDI SysEx permission.  Am I missing something?
>>
>
>  Please consider my alternate proposal or argue that the way you're
>> structuring
>> these histograms is more useful than my suggestion.
>>
>
>  If you think my proposal makes sense, I can give you pointers on how to
>> implement it easily (if you need help).
>>
>
>  --mark
>>
>
>
> https://codereview.chromium.org/371933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Miguel Garcia, 2014-07-14 15:04:45]

bauerb, mpearson PTAL

I chatted offline with Mark and learnt a bit more about UMA and the macro system
that supports it. I have reworked the UMA as he suggested and added two more
dimensions; ignored for when the bubble/infobar is completely ignored and
dismissed for when the infobar/bubble is explicitly dismissed (i.e the little x
is pressed).




On 2014/07/10 16:33:32, Mark P wrote:
> On Thu, Jul 10, 2014 at 2:03 AM, <mailto:miguelg@chromium.org> wrote:
> 
> > I don't think your suggestion conflicts with this one though individual
> > permission owners can (and already do) still track per permission metrics.
> > I
> > would love to be able to automate it but I don't know how to create the
> > labels
> > automatically,
> 
> 
> You can create new histograms automatically using the "histogram suffixes"
> ability of histograms.xml.  In particular, this will allow you to generate
> n histograms automatically simply from a short list of the n possible
> suffixes.  In your case, the suffixes can be the permission type.
> 
> 
> > every single new permission would need to update histograms.xml
> > (or am I missing something?)
> >
> 
> Yes, you'd have to modify histograms.xml to add a new permission on the
> suffix list.  This is no more onerous than what you currently would have to
> do: modify histograms.xml to add a new permission to the enum list of
> permission types.
> 
> 
> > While certainly less useful than your proposal these histograms will let us
> > verify in one look if the permission related features are being used at
> > all or
> > not. More importantly, comparing closely related permissions (like
> > notifications
> > and push messaging) will in fact provide useful data as well and let us
> > decide
> > if we can collapse them for example).
> >
> 
> You made two claims about why this current structure is okay:
> * "these histograms will let us verify in one look if the permission
> related features are being used at all".  This is true.  But it's true for
> the histograms in my proposal as well.
> * "comparing closely related permissions (like notifications and push
> messaging) will in fact provide useful data".  I don't buy this argument.
>  The way these histograms will be displayed will invite comparison between
> permission types, but those comparisons will be misleading without doing
> further computation.  For instance, suppose you see that in the
> PermissionGranted histogram the number of Notification permissions granted
> is tiny compared to the number of Push Messaging permission granted.  This
> may make you want to, say, eliminate Notification permissions / combine
> them with Push Messaging permissions.  If they often go together, and one
> is small, then you might as well eliminate the one, eh?
> 
> This would be wrong reasoning.  Comparing the permission granted numbers is
> not useful without normalizing those numbers by the number of times those
> permissions are requested.
> 
> 
> 
> > The change for moving the notification permission system into the common
> > one is
> > coming in https://codereview.chromium.org/376253005/
> >
> >
> >
> > On 2014/07/09 23:18:52, Mark P wrote:
> >
> >> Now that you have clearer histogram descriptions, I can understand what
> >> these
> >> histograms are doing.  Their structure, however, seems odd.  I would think
> >>
> > life
> >
> >> would be easier if you had one history for each permission type (e.g.,
> >> MIDI_SYSEX) with values of requested, granted, and denied.  That way if
> >> you
> >>
> > want
> >
> >> to find out how users reach to a particular permission, they can look at
> >> one
> >> histogram and compare the three buckets within it.  Personally, I don't
> >> see
> >>
> > any
> >
> >> benefit to structuring these histograms the way you do--I don't see the
> >> point
> >>
> > of
> >
> >> being able to easily notice, say, 5x more users granted push messaging
> >> permission than granted MIDI SysEx permission.  Am I missing something?
> >>
> >
> >  Please consider my alternate proposal or argue that the way you're
> >> structuring
> >> these histograms is more useful than my suggestion.
> >>
> >
> >  If you think my proposal makes sense, I can give you pointers on how to
> >> implement it easily (if you need help).
> >>
> >
> >  --mark
> >>
> >
> >
> > https://codereview.chromium.org/371933002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2014-07-14 15:22:15]

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.cc:34:
PermissionContextUmaUtil::PermissionIgnored(type_);
Nit: Braces are unnecessary.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_bubble_request_impl.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.h:45: // Remember
to call RegisterActionTaken fro these methods if you are
Nit: "for"

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.h:52: void
RegisterActionTaken() {action_taken_ = true; }
Nit: space after opening brace.

Also, please add an empty line back in here.

Also, you could make this protected if it's just for subclasses to call.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:61: // static
Nit: static could come immediately before the method implementations. Also,
please add it for all of them (or leave it out if you want to, but be
consistent).

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.h:22:
PermissionContextUmaUtil() {} // only static methods allowed
DISALLOW_IMPLICIT_CONSTRUCTORS that will do this and DISALLOW_COPY_AND_ASSIGN
below.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_infobar_delegate.cc:45:
RegisterActionTaken();
You could probably move this into SetPermission().

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_infobar_delegate.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_infobar_delegate.h:42: void
RegisterActionTaken() {action_taken_ = true; }
Nit: space before brace, and empty line.

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

https://codereview.chromium.org/371933002/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44159: +  <int value="2"
label="DENYED"/>
Nit: DENIED

[Miguel Garcia, 2014-07-14 16:57:50]

Michael complained that the histogram that compared requested permissions was in
fact valuable which I agreed with.

 I have therefore removed REQUESTED from the PermissionAction enum and revived
the original PermissionRequested enum.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.cc:34:
PermissionContextUmaUtil::PermissionIgnored(type_);
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: Braces are unnecessary.

Done.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_bubble_request_impl.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.h:45: // Remember
to call RegisterActionTaken fro these methods if you are
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: "for"

Done.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_bubble_request_impl.h:52: void
RegisterActionTaken() {action_taken_ = true; }
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: space after opening brace.
> 
> Also, please add an empty line back in here.
> 
> Also, you could make this protected if it's just for subclasses to call.

Done.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:61: // static
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: static could come immediately before the method implementations. Also,
> please add it for all of them (or leave it out if you want to, but be
> consistent).

I removed it, it is clear from the class that they are all static.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.h:22:
PermissionContextUmaUtil() {} // only static methods allowed
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS that will do this and DISALLOW_COPY_AND_ASSIGN
> below.
Thanks for the tip!

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_infobar_delegate.cc:45:
RegisterActionTaken();
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> You could probably move this into SetPermission().

I have actually removed the RegisterActionTaken call altogher and inlined it in
setpermission.

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/permission_infobar_delegate.h (right):

https://codereview.chromium.org/371933002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/permission_infobar_delegate.h:42: void
RegisterActionTaken() {action_taken_ = true; }
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: space before brace, and empty line.

Done and moved it to protected too

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

https://codereview.chromium.org/371933002/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44159: +  <int value="2"
label="DENYED"/>
On 2014/07/14 15:22:15, Bernhard Bauer wrote:
> Nit: DENIED

Done.

[Bernhard Bauer, 2014-07-14 17:01:01]

lgtm

[Miguel Garcia, 2014-07-15 19:27:57]

Friendly ping for the remaining owners
On Jul 14, 2014 6:01 PM, <bauerb@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/371933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-07-15 20:17:41]

histograms.xml lgtm

Some more nits on the other parts of the code I saw.  I didn't do a thorough
review.  I trusted your other reviewers to do that.  Given the style problems I
saw, though, I'm not sure I should have...  :-|

--mark

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:11: // add new
permission actions. Never delete or reorder
nit: weird line lengths

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:23: enum
PermissionType {
please add an analogous comment to that above right here.

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:84: 
in general, what's with the extra vertical space in various places in the code? 
Why is this one function set off on both sides from the other by two lines
instead of the usual one?

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.h (right):

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.h:20: 
nit: why the extra space?  Did you intend to have a label (private: ?) or
comment here?

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

https://codereview.chromium.org/371933002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2878: +    a permission was
granted,rejected...
How about inverting this summary like so:
Tracks whether a permission was granted, rejected, etc.  The suffix of the
histogram indicates which particular permission.

[Miguel Garcia, 2014-07-16 11:15:27]

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:11: // add new
permission actions. Never delete or reorder
On 2014/07/15 20:17:40, Mark P wrote:
> nit: weird line lengths

Done.

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.cc:23: enum
PermissionType {
On 2014/07/15 20:17:41, Mark P wrote:
> please add an analogous comment to that above right here.

Done.

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_uma_util.h (right):

https://codereview.chromium.org/371933002/diff/180001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_uma_util.h:20: 
On 2014/07/15 20:17:41, Mark P wrote:
> nit: why the extra space?  Did you intend to have a label (private: ?) or
> comment here?
Not really, it was an artifact of the refactor.

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

https://codereview.chromium.org/371933002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2878: +    a permission was
granted,rejected...
On 2014/07/15 20:17:41, Mark P wrote:
> How about inverting this summary like so:
> Tracks whether a permission was granted, rejected, etc.  The suffix of the
> histogram indicates which particular permission.

Done.

[Miguel Garcia, 2014-07-16 11:16:51]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-07-16 11:19:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/371933002/200001

[commit-bot: I haz the power, 2014-07-16 22:23:22]

Change committed as 283546