Add a new metric for how many times permissions prompts are shown.

Add a new metric for how many times permissions prompts are shown.

BUG=603384
Committed: https://crrev.com/46b02faec924adb3bdf9d4e62e8d878edc7cdc2d
Cr-Commit-Position: refs/heads/master@{#388407}

[benwells, 2016-04-14 06:44:52]

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

[tsergeant, 2016-04-15 00:54:10]

Just a couple small things, overall this seems like a good way to measure what
you're after.

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/chrome_q...
File chrome/browser/chrome_quota_permission_context.cc (right):

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/chrome_q...
chrome/browser/chrome_quota_permission_context.cc:55: private:
Why does everything go from public to private here (and in several of the other
classes you're changing)?

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

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:15: // Used for
UMA to record the types of permission prompts shown.
Maybe add a note that this corresponds to the PermissionPromptType histograms
enum?

[benwells, 2016-04-18 01:02:06]

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/chrome_q...
File chrome/browser/chrome_quota_permission_context.cc (right):

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/chrome_q...
chrome/browser/chrome_quota_permission_context.cc:55: private:
On 2016/04/15 00:54:10, tsergeant wrote:
> Why does everything go from public to private here (and in several of the
other
> classes you're changing)?

As discussed offline, this means users of these methods have to know of
instances of this class as a PermissionBubbleRequest, not a
QuotaPermissionRequest, which is more abstract and in general better as it means
it is being treated generally.

This is something I've been asked many times and seems to be a convention in
chromium. Note that if you google around some people think it is a bad idea.

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

https://codereview.chromium.org/1884073002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:15: // Used for
UMA to record the types of permission prompts shown.
On 2016/04/15 00:54:10, tsergeant wrote:
> Maybe add a note that this corresponds to the PermissionPromptType histograms
> enum?

Done.

[tsergeant, 2016-04-18 01:14:20]

lgtm!

[benwells, 2016-04-18 01:25:56]

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

[benwells, 2016-04-18 01:25:57]

+felt for owner reviews of bubble stuff

[felt, 2016-04-18 21:15:14]

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

https://codereview.chromium.org/1884073002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:21: // - keep the
PermissionPromptType enum in sync with this definition.
"Your search - PermissionPromptType - did not match any documents." 
which enum do you mean?

also, in general, I am very wary of requiring future people who update enums to
keep two enums in sync.

[benwells, 2016-04-19 00:09:25]

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

https://codereview.chromium.org/1884073002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:21: // - keep the
PermissionPromptType enum in sync with this definition.
On 2016/04/18 21:15:14, felt wrote:
> "Your search - PermissionPromptType - did not match any documents." 
> which enum do you mean?
> 
> also, in general, I am very wary of requiring future people who update enums
to
> keep two enums in sync.

I don't know of any way to get around needing the two enums. The one in
histograms.xml is just a description of this one for reporting in the UMA front
end.

However I think I should name the enums the same to make it clearer. I had kept
the UMA one more general in case it one day needs to refer to more than just
bubbles (e.g. infobars), but if that ever happens we can just rename it then.

[felt, 2016-04-19 00:21:20]

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

https://codereview.chromium.org/1884073002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:21: // - keep the
PermissionPromptType enum in sync with this definition.
On 2016/04/19 00:09:24, benwells wrote:
> On 2016/04/18 21:15:14, felt wrote:
> > "Your search - PermissionPromptType - did not match any documents." 
> > which enum do you mean?
> > 
> > also, in general, I am very wary of requiring future people who update enums
> to
> > keep two enums in sync.
> 
> I don't know of any way to get around needing the two enums. The one in
> histograms.xml is just a description of this one for reporting in the UMA
front
> end.
> 
> However I think I should name the enums the same to make it clearer. I had
kept
> the UMA one more general in case it one day needs to refer to more than just
> bubbles (e.g. infobars), but if that ever happens we can just rename it then.

Ohhhh, I missed that this was the name of the enum in histograms.xml. I thought
you were referring to a histogram somewhere else in C++ code that I couldn't
find. I am not concerned anymore now that I see what was meant.

[felt, 2016-04-19 00:22:18]

lgtm

[benwells, 2016-04-19 01:36:58]

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

[benwells, 2016-04-19 01:36:59]

+thestig for chrome/ owners
+asvitkine for histograms review

[Lei Zhang, 2016-04-19 01:46:05]

lgtm

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.cc:281: PermissionBubbleType
permission_prompt_type = PermissionBubbleType::MULTIPLE;
DCHECK(!requests.empty());

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.h:66: const
std::vector<PermissionBubbleRequest*> requests);
Pass by const-ref?

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

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:22: enum class
PermissionBubbleType {
s/class //

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:30: NUM
Add comment to note this must be last.

[Lei Zhang, 2016-04-19 01:55:45]

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

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:22: enum class
PermissionBubbleType {
On 2016/04/19 01:46:05, Lei Zhang wrote:
> s/class //

Ignore. I've been hiding under a rock and forgot about fancy C++11 bits.

[Alexei Svitkine (slow), 2016-04-19 13:49:24]

lgtm

[benwells, 2016-04-20 01:32:10]

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.cc:281: PermissionBubbleType
permission_prompt_type = PermissionBubbleType::MULTIPLE;
On 2016/04/19 01:46:05, Lei Zhang wrote:
> DCHECK(!requests.empty());

Done.

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.h:66: const
std::vector<PermissionBubbleRequest*> requests);
On 2016/04/19 01:46:05, Lei Zhang wrote:
> Pass by const-ref?

Done.

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

https://codereview.chromium.org/1884073002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_request.h:30: NUM
On 2016/04/19 01:46:05, Lei Zhang wrote:
> Add comment to note this must be last.

Done.

[benwells, 2016-04-20 01:32:15]

The CQ bit was checked by benwells@chromium.org

[benwells, 2016-04-20 01:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, thestig@chromium.org, felt@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1884073002/#ps100001
(title: "Feedback")

[commit-bot: I haz the power, 2016-04-20 01:32:29]

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

[commit-bot: I haz the power, 2016-04-20 02:37:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:20:09]

Description was changed from

==========
Add a new metric for how many times permissions prompts are shown.

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

to

==========
Add a new metric for how many times permissions prompts are shown.

BUG=603384

Committed: https://crrev.com/46b02faec924adb3bdf9d4e62e8d878edc7cdc2d
Cr-Commit-Position: refs/heads/master@{#388407}
==========

[commit-bot: I haz the power, 2016-04-22 19:20:10]

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