Implement permission embargo suppression metrics.

Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
Review-Url: https://codereview.chromium.org/2701343002
Cr-Commit-Position: refs/heads/master@{#452466}
Committed: https://chromium.googlesource.com/chromium/src/+/2391315fa51475da9ea5314ed81990e709644659

[dominickn, 2017-02-20 03:38:14]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 03:38:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-02-20 03:39:11]

Description was changed from

==========
Implement permission status source metrics.

This CL expands the PermissionStatusSource enum with several additional
buckets, and adds a new UMA histogram to measure the source of every
permission request that enters the permissions system.

Potential statuses are that the permission request was blocked for
a variety of reasons (kill switch, invalid/insecure origin, multiple
dismissals, or Safe Browsing blacklisting), or that the value according
to the permissions system was returned (e.g. enterprise policy,
prompting the user, etc).

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

to

==========
Implement permission decision source metrics.

This CL expands the PermissionStatusSource enum with several additional
buckets, and adds a new UMA histogram to measure the source of every
permission request decision.

Potential statuses are that the permission request was blocked for
a variety of reasons (kill switch, invalid/insecure origin, multiple
dismissals, or Safe Browsing blacklisting), or that the value according
to the permissions system was returned (e.g. enterprise policy,
prompting the user, etc).

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

[dominickn, 2017-02-20 03:39:11]

dominickn@chromium.org changed reviewers:
+ raymes@chromium.org

[dominickn, 2017-02-20 03:39:52]

PTAL, thanks! This involved a bunch of code moving around to tidy things up, as
well as the introduction of a bunch of new statuses. Hopefully it all makes
sense.

[commit-bot: I haz the power, 2017-02-20 04:34:56]

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

[commit-bot: I haz the power, 2017-02-20 04:34:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-02-20 05:18:06]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 05:18:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 07:10:31]

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

[commit-bot: I haz the power, 2017-02-20 07:10:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-02-21 02:46:26]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 02:46:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 03:38:46]

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

[commit-bot: I haz the power, 2017-02-21 03:38:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-02-21 03:49:47]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 03:50:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 04:05:21]

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

[commit-bot: I haz the power, 2017-02-21 04:05:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2017-02-21 04:16:03]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 04:16:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 07:18:54]

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

[commit-bot: I haz the power, 2017-02-21 07:18:55]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2017-02-22 03:36:27]

Description was changed from

==========
Implement permission decision source metrics.

This CL expands the PermissionStatusSource enum with several additional
buckets, and adds a new UMA histogram to measure the source of every
permission request decision.

Potential statuses are that the permission request was blocked for
a variety of reasons (kill switch, invalid/insecure origin, multiple
dismissals, or Safe Browsing blacklisting), or that the value according
to the permissions system was returned (e.g. enterprise policy,
prompting the user, etc).

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

to

==========
Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

[dominickn, 2017-02-22 03:38:06]

Revamped based on our discussion. Unfortunately it's a bit big, but most of that
is because of the updates to test files and moving declarations around. PTAL,
thanks

[dominickn, 2017-02-22 03:38:12]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 03:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-02-22 06:33:59]

Description was changed from

==========
Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

to

==========
Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

[dominickn, 2017-02-22 06:54:42]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 06:55:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 08:40:26]

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

[commit-bot: I haz the power, 2017-02-22 08:40:27]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-02-22 23:13:50]

Thanks Dom! Overall looks good, just minor comments.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:128: }
nit: Could we make the conversion a helper function in permission uma util?

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:198:
->GetEmbargoResult(content_settings_type_, requesting_origin);
Could we DCHECK that the content setting of the return value here is ASK or
BLOCK? I think this code is making an assumption that it will never be ALLOW.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:722:
PermissionEmbargoStatus::NOT_EMBARGOED, 2);
I think the autoblocking stuff is fairly orthogonal to this test, so it might
make sense to leave this out.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:764:
EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
should we check the source in the else case?

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_decision_auto_blocker.cc:273:
CONTENT_SETTING_ASK);
nit: DCHECK_EQ(CONTENT_SETTING_ASK, ...)

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_decision_auto_blocker.cc:313: setting =
CONTENT_SETTING_BLOCK;
Can we just 
return PermissionResult(CONTENT_SETTING_BLOCK,
PermissionStatusSource::SAFE_BROWSING_BLACKLIST) 

here (and likewise below)? It saves storing the local state which is slightly
more error prone when adding new cases.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:83:
PermissionEmbargoStatus::NOT_EMBARGOED);
I guess this is fixing a bug with Meredith's CL for Android?

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:56: NUM
nit: I don't think this is needed anymore.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:66: };
I think it would be good to move these into their own permission_result.h file
at some point (PermissionResult and PermissionStatusSource). Doesn't have to be
in this CL though.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:70: };
Is this needed?

[dominickn, 2017-02-22 23:38:43]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-02-22 23:38:55]

Thanks!

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:128: }
On 2017/02/22 23:13:50, raymes wrote:
> nit: Could we make the conversion a helper function in permission uma util?

This can only be done by proxying the call to RecordEmbargoPromptSuppression in
the helper method entirely since we only record something for 2 cases. I don't
know if this is actually clearer or not.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:198:
->GetEmbargoResult(content_settings_type_, requesting_origin);
On 2017/02/22 23:13:50, raymes wrote:
> Could we DCHECK that the content setting of the return value here is ASK or
> BLOCK? I think this code is making an assumption that it will never be ALLOW.

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:722:
PermissionEmbargoStatus::NOT_EMBARGOED, 2);
On 2017/02/22 23:13:50, raymes wrote:
> I think the autoblocking stuff is fairly orthogonal to this test, so it might
> make sense to leave this out.

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:764:
EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
On 2017/02/22 23:13:50, raymes wrote:
> should we check the source in the else case?

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_decision_auto_blocker.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_decision_auto_blocker.cc:273:
CONTENT_SETTING_ASK);
On 2017/02/22 23:13:50, raymes wrote:
> nit: DCHECK_EQ(CONTENT_SETTING_ASK, ...)

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_decision_auto_blocker.cc:313: setting =
CONTENT_SETTING_BLOCK;
On 2017/02/22 23:13:50, raymes wrote:
> Can we just 
> return PermissionResult(CONTENT_SETTING_BLOCK,
> PermissionStatusSource::SAFE_BROWSING_BLACKLIST) 
> 
> here (and likewise below)? It saves storing the local state which is slightly
> more error prone when adding new cases.

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:83:
PermissionEmbargoStatus::NOT_EMBARGOED);
On 2017/02/22 23:13:50, raymes wrote:
> I guess this is fixing a bug with Meredith's CL for Android?

Yes - see the corresponding PermissionRequestImpl destructor, which calls this
method.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:56: NUM
On 2017/02/22 23:13:50, raymes wrote:
> nit: I don't think this is needed anymore.

Done.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:66: };
On 2017/02/22 23:13:50, raymes wrote:
> I think it would be good to move these into their own permission_result.h file
> at some point (PermissionResult and PermissionStatusSource). Doesn't have to
be
> in this CL though.

Acknowledged.

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:70: };
On 2017/02/22 23:13:50, raymes wrote:
> Is this needed?

Odd, looks like whatever used it has been removed. Must have been timloh's
refactoring.

[commit-bot: I haz the power, 2017-02-22 23:39:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2017-02-23 01:28:58]

lgtm thanks!

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2701343002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:128: }
On 2017/02/22 23:38:54, dominickn wrote:
> On 2017/02/22 23:13:50, raymes wrote:
> > nit: Could we make the conversion a helper function in permission uma util?
> 
> This can only be done by proxying the call to RecordEmbargoPromptSuppression
in
> the helper method entirely since we only record something for 2 cases. I don't
> know if this is actually clearer or not.

I agree - it's not ideal. We could alternatively have a function which does the
conversion and returns a bool if it should be recorded, e.g.

bool PermissionStatusSourceToPermissionEmbargoStatus(PermissionStatusSource
source, PermissionEmbargoStatus* result);

I don't feel too strongly though.

[dominickn, 2017-02-23 01:33:08]

dominickn@chromium.org changed reviewers:
+ isherman@chromium.org

[dominickn, 2017-02-23 01:33:09]

+isherman: PTAL at histograms, thanks!

[commit-bot: I haz the power, 2017-02-23 02:41:43]

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

[commit-bot: I haz the power, 2017-02-23 02:41:44]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-02-23 07:58:52]

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

https://codereview.chromium.org/2701343002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:46306: +    prompt is shown and a no
embargo reason is recorded.
Hmm, I'm a bit confused: Here you say that nothing is recorded if there was no
embargo; but above, you say that the metric tracks *whether* the request is
suppressed, which implies that there's a way to tell how often the permission is
*not* embargoed.  These two statements seem to contradict each other.  What am I
missing?

[dominickn, 2017-02-23 08:05:50]

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

https://codereview.chromium.org/2701343002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:46306: +    prompt is shown and a no
embargo reason is recorded.
On 2017/02/23 07:58:52, Ilya Sherman wrote:
> Hmm, I'm a bit confused: Here you say that nothing is recorded if there was no
> embargo; but above, you say that the metric tracks *whether* the request is
> suppressed, which implies that there's a way to tell how often the permission
is
> *not* embargoed.  These two statements seem to contradict each other.  What am
I
> missing?

A "no embargo" reason is recorded, not "no embargo reason is recorded". Does
that clarify?

[Ilya Sherman, 2017-02-23 08:08:41]

Metrics LGTM

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

https://codereview.chromium.org/2701343002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:46306: +    prompt is shown and a no
embargo reason is recorded.
On 2017/02/23 08:05:49, dominickn wrote:
> On 2017/02/23 07:58:52, Ilya Sherman wrote:
> > Hmm, I'm a bit confused: Here you say that nothing is recorded if there was
no
> > embargo; but above, you say that the metric tracks *whether* the request is
> > suppressed, which implies that there's a way to tell how often the
permission
> is
> > *not* embargoed.  These two statements seem to contradict each other.  What
am
> I
> > missing?
> 
> A "no embargo" reason is recorded, not "no embargo reason is recorded". Does
> that clarify?

Ah, yes, I misread that sentence.  Makes sense now, thanks!

[dominickn, 2017-02-23 08:09:41]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-02-23 08:09:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 08:15:22]

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

[commit-bot: I haz the power, 2017-02-23 08:15:24]

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

[dominickn, 2017-02-23 10:55:22]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-02-23 10:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2701343002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-23 10:55:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 12:04:08]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487847322924670,
"parent_rev": "2075239e1a890ce7bd32ae54ca5575c145528649", "commit_rev":
"2391315fa51475da9ea5314ed81990e709644659"}

[commit-bot: I haz the power, 2017-02-23 12:04:43]

Description was changed from

==========
Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877
==========

to

==========
Implement permission embargo suppression metrics.

This CL adds a new UMA histogram to record whether or not permission
prompts are suppressed due to embargo. This allows us to measure how
effective embargo is at reducing the number of prompts seen by users.

New histogram tests are implemented to verify that the metrics are
recorded as expected.

BUG=679877

Review-Url: https://codereview.chromium.org/2701343002
Cr-Commit-Position: refs/heads/master@{#452466}
Committed:
https://chromium.googlesource.com/chromium/src/+/2391315fa51475da9ea5314ed819...
==========

[commit-bot: I haz the power, 2017-02-23 12:04:44]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/2391315fa51475da9ea5314ed819...