Log UMA metrics for permission type when permission bubbles shown.

Log UMA metrics for permission type when permission bubbles shown.

BUG=603384
Committed: https://crrev.com/597ad9d072afd837dd59bf80ac6fb6fcf4c7094c
Cr-Commit-Position: refs/heads/master@{#390292}

[benwells, 2016-04-21 03:58:23]

benwells@chromium.org changed reviewers:
+ tsergeant@chromium.org

[benwells, 2016-04-21 03:58:23]

Ignore the bot redness, I think it's unrelated

[tsergeant, 2016-04-21 06:45:55]

On 2016/04/21 03:58:23, benwells wrote:
> Ignore the bot redness, I think it's unrelated

Is there a corresponding change to histograms.xml that needs to be made?

Related to what you asked me about this morning: Is the plan for media
permissions just to wait for the glorious Permission Context future? You could
have it work in the current system for requests for Audio or Video, but requests
for Audio and Video together are awkward...

[benwells, 2016-04-21 07:31:26]

On 2016/04/21 06:45:55, tsergeant wrote:
> On 2016/04/21 03:58:23, benwells wrote:
> > Ignore the bot redness, I think it's unrelated
> 
> Is there a corresponding change to histograms.xml that needs to be made?

Oops, there is now :)

> 
> Related to what you asked me about this morning: Is the plan for media
> permissions just to wait for the glorious Permission Context future? You could
> have it work in the current system for requests for Audio or Video, but
requests
> for Audio and Video together are awkward...

Waiting for the glorious future is what I was thinking. These will be tracked by
the MEDIA bubble type in the other histogram in the meantime, just not broken
out into this new permission histogram.

[tsergeant, 2016-04-21 07:37:07]

lgtm

[benwells, 2016-04-21 07:40:16]

benwells@chromium.org changed reviewers:
+ asvitkine@chromium.org, felt@chromium.org

[benwells, 2016-04-21 07:40:17]

+felt, +asvitkine for histograms.

felt - this is no. 2 in a series of patches to add prompt metrics. Coming up
will be metrics for coalesced bubbles and metrics recording bubble responses.

[felt, 2016-04-21 13:52:31]

why do we need both the permission bubble type, and the permission type?

[benwells, 2016-04-21 23:15:42]

On 2016/04/21 13:52:31, felt wrote:
> why do we need both the permission bubble type, and the permission type?

They are kinda different, e.g. for bubble type you can have register protocol
handler or permission (among others). Of these only permission actually has a
permission type. Confusing but that's the way the system is.

Originally I was going to have numbering system to represent all of them but it
seemed a bit messy. To do that I'd need to have some constant from which to
start the permission types (e.g. 100), and that would make the histogram enums
sparse. That could be made to work but this way seemed cleaner.

[Alexei Svitkine (slow), 2016-04-22 14:59:20]

lgtm

[benwells, 2016-04-27 12:07:42]

felt - I've rearranged this to just use the one enum. I think this will be
easier to see in UMA and build a dashboard over.

[felt, 2016-04-27 15:37:24]

thanks i think this looks much better

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_bubble_request_impl.cc:186: return
PermissionBubbleType::UNKNOWN;
weird to have a return after a NOTREACHED()?

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_request.h (right):

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:31:
PERMISSION_GEOLOCATION,
nit: why are these (but not the others) prefixed with PERMISSION_ ?

[benwells, 2016-04-28 00:16:59]

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_bubble_request_impl.cc:186: return
PermissionBubbleType::UNKNOWN;
On 2016/04/27 15:37:24, felt wrote:
> weird to have a return after a NOTREACHED()?

To compile the function has to return something, plus NOTREACHED will be
compiled out on Release builds.

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_request.h (right):

https://codereview.chromium.org/1903103005/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:31:
PERMISSION_GEOLOCATION,
On 2016/04/27 15:37:24, felt wrote:
> nit: why are these (but not the others) prefixed with PERMISSION_ ?

No big reason, just because they correspond to PermissionType.

[felt, 2016-04-28 00:23:28]

lgtm

[benwells, 2016-04-28 03:41:55]

The CQ bit was checked by benwells@chromium.org

[benwells, 2016-04-28 03:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1903103005/#ps80001
(title: "Fix chromeos compile")

[commit-bot: I haz the power, 2016-04-28 03:42:10]

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

[commit-bot: I haz the power, 2016-04-28 03:46:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-30 17:16:00]

Patchset 5 (id:??) landed as
https://crrev.com/597ad9d072afd837dd59bf80ac6fb6fcf4c7094c
Cr-Commit-Position: refs/heads/master@{#390292}

[commit-bot: I haz the power, 2016-04-30 17:54:03]

Description was changed from

==========
Log UMA metrics for permission type when permission bubbles shown.

BUG=603384
==========

to

==========
Log UMA metrics for permission type when permission bubbles shown.

BUG=603384

Committed: https://crrev.com/597ad9d072afd837dd59bf80ac6fb6fcf4c7094c
Cr-Commit-Position: refs/heads/master@{#390292}
==========

[rkaplow, 2016-05-02 20:39:13]

On 2016/04/30 17:16:00, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/597ad9d072afd837dd59bf80ac6fb6fcf4c7094c
> Cr-Commit-Position: refs/heads/master@{#390292}

Looks like some files were missing on the commit,  permission_bubble_request.cc,
permission_uma_util.cc.
These should be readded it looks like since the new metric
Permissions.Prompt.ShownForPermission isn't being recorded.

[benwells, 2016-05-04 18:41:32]

On 2016/05/02 20:39:13, rkaplow wrote:
> On 2016/04/30 17:16:00, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/597ad9d072afd837dd59bf80ac6fb6fcf4c7094c
> > Cr-Commit-Position: refs/heads/master@{#390292}
> 
> Looks like some files were missing on the commit, 
permission_bubble_request.cc,
> permission_uma_util.cc.
> These should be readded it looks like since the new metric
> Permissions.Prompt.ShownForPermission isn't being recorded.

Actually that part of this histograms.xml change shouldn't have been included.
It is removed in https://codereview.chromium.org/1926003002/.