Add a source to the result of PermissionContextBase::GetPermissionStatus

Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

BUG=679877
TBR=peter@chromium.org,michaeln@chromium.org,sergeyu@chromium.org

Review-Url: https://codereview.chromium.org/2686463002
Cr-Commit-Position: refs/heads/master@{#450590}
Committed: https://chromium.googlesource.com/chromium/src/+/ab35971d1da1c791761f65f8dbd6df9512fd2c4d

[raymes, 2017-02-07 07:21:40]

raymes@chromium.org changed reviewers:
+ kcarattini@chromium.org, meredithl@google.com

[raymes, 2017-02-07 07:21:41]

Hey Kendra, PTAL. We discussed doing this a little while back. Let me know if
you think this is reasonable.

Thanks!

[kcarattini, 2017-02-07 22:53:33]

Thanks so much for doing this, Raymes!

+cc peter since he might be interested in using that KillSwitch result.

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base.h (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base.h:34: enum class
PermissionStatusSource {
Should we have a USER_DECISION value as well?

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base_unittest.cc:684: if
(expected_permission_status == CONTENT_SETTING_ALLOW) {
Are you not planning to set a source if the content setting is ALLOW?

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:503: .content_setting;
Why not pass through the source as well?

[meredithl, 2017-02-08 03:26:12]

lgtm

Thanks, Raymes. This looks great to me (although I am by no means an experienced
reviewer :)).

[raymes, 2017-02-08 04:08:40]

Thanks, ptal!

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base.h (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base.h:34: enum class
PermissionStatusSource {
On 2017/02/07 22:53:33, kcarattini wrote:
> Should we have a USER_DECISION value as well?

Yes eventually. I didn't want to add it right now because it will be unused and
it will take care to add it in a correct way. I only want to add ones that will
get used right now so we don't have dead code. In fact for that reason I'm going
to remove the INSECURE_ORIGIN and KILL_SWITCH check for now. I'll add
KILL_SWITCH back and use it for media_permission.cc in a followup CL. The other
two should be directly used by Meredith.

That makes this CL look a little bit weird, because the reasons aren't currently
used anywhere. So if it makes more sense, Meredith could merge this into her CL
which will use it. WDYT?

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_context_base_unittest.cc:684: if
(expected_permission_status == CONTENT_SETTING_ALLOW) {
On 2017/02/07 22:53:33, kcarattini wrote:
> Are you not planning to set a source if the content setting is ALLOW?

Eventually it will be USER_DECISION (or something), but for now it will just be
UNSPECIFIED.

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2686463002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:503: .content_setting;
On 2017/02/07 22:53:33, kcarattini wrote:
> Why not pass through the source as well?

I'll do this in a followup. I would do it in the same CL, but it will require
changing many callsites that use PermissionManager. I'd rather separate that out
because it will be big, and Meredith wants to use this bit to add a UMA metric.

[raymes, 2017-02-08 04:10:09]

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

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

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

[kcarattini, 2017-02-08 04:39:48]

Ok. Could you update the cl description to say exactly which "sources" you are
adding and what you are leaving for followup? Also please make sure there are
TODOs everywhere where you intend to add a new source later.

[commit-bot: I haz the power, 2017-02-08 05:12:07]

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

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

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-08 15:51:42]

Thanks for the cc - this looks great! I'll use it to supersede 2534503002 once
it lands :)

[raymes, 2017-02-09 02:10:24]

Description was changed from

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting. Some initial reasons have been added, and tests and callsites
updated appropriately.

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

to

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

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

[raymes, 2017-02-09 02:11:53]

Description was changed from

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

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

to

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

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

[raymes, 2017-02-09 02:12:35]

On 2017/02/08 04:39:48, kcarattini wrote:
> Ok. Could you update the cl description to say exactly which "sources" you are
> adding and what you are leaving for followup? Also please make sure there are
> TODOs everywhere where you intend to add a new source later.

Updated the description. Adding TODOs everywhere would be a bit messy and I
think care has to be taken when doing that to make sure we do it right as well,
so I've just left it as a single TODO for now.

[raymes, 2017-02-09 02:13:04]

On 2017/02/08 15:51:42, Peter Beverloo wrote:
> Thanks for the cc - this looks great! I'll use it to supersede 2534503002 once
> it lands :)

You'll need to wait for one subsequent CL prior to using it for the kill switch
:)

[kcarattini, 2017-02-10 02:09:40]

lgtm

Thanks, Raymes!

[raymes, 2017-02-14 04:27:08]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-02-14 04:27:08]

The patchset sent to the CQ was uploaded after l-g-t-m from meredithl@google.com
Link to the patchset: https://codereview.chromium.org/2686463002/#ps60001
(title: "Add a source to the result of
PermissionContextBase::GetPermissionStatus")

[commit-bot: I haz the power, 2017-02-14 04:27:57]

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

[raymes, 2017-02-14 04:28:07]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2017-02-14 04:28:12]

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

[commit-bot: I haz the power, 2017-02-14 04:28:25]

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-14 05:28:38]

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

[commit-bot: I haz the power, 2017-02-14 05:28:56]

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-14 05:30:48]

Description was changed from

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

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

to

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

BUG=679877
TBR=peter@chromium.org,michaeln@chromium.org,sergeyu@chromium.org
==========

[raymes, 2017-02-14 05:32:29]

raymes@chromium.org changed reviewers:
+ michaeln@chromium.org, peter@chromium.org, sergeyu@chromium.org

[raymes, 2017-02-14 05:32:30]

TBR OWNERS of client code

michaeln:
  chrome/browser/storage/durable_storage_permission_context.cc
  chrome/browser/storage/durable_storage_permission_context_unittest.cc

sergeyu:
  chrome/browser/media/midi_permission_context_unittest.cc
  chrome/browser/media/webrtc/media_stream_device_permission_context_unittest.cc

peter:
  chrome/browser/notifications/notification_permission_context_unittest.cc

[raymes, 2017-02-14 05:32:44]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2017-02-14 05:32:45]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-02-14 05:32:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
meredithl@google.com, kcarattini@chromium.org
Link to the patchset: https://codereview.chromium.org/2686463002/#ps80001
(title: "Add a source to the result of
PermissionContextBase::GetPermissionStatus")

[commit-bot: I haz the power, 2017-02-14 05:32:53]

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-14 07:10:31]

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

[commit-bot: I haz the power, 2017-02-14 07:10:33]

Failed to apply patch for
chrome/browser/permissions/permission_context_base_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/permissions/permission_context_base_unittest.cc:355
  error: chrome/browser/permissions/permission_context_base_unittest.cc: patch
does not apply

Patch:       chrome/browser/permissions/permission_context_base_unittest.cc
Index: chrome/browser/permissions/permission_context_base_unittest.cc
diff --git a/chrome/browser/permissions/permission_context_base_unittest.cc
b/chrome/browser/permissions/permission_context_base_unittest.cc
index
44e469cda75c612cfbfcea0cccd529cef6a2633c..ae6b96461818b44705414f7682e2968ab49df63b
100644
--- a/chrome/browser/permissions/permission_context_base_unittest.cc
+++ b/chrome/browser/permissions/permission_context_base_unittest.cc
@@ -355,7 +355,10 @@ class PermissionContextBaseTests : public
ChromeRenderViewHostTestHarness {
       ASSERT_EQ(1u, permission_context.decisions().size());
       EXPECT_EQ(expected, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
-      EXPECT_EQ(expected, permission_context.GetPermissionStatus(url, url));
+      PermissionResult result =
+          permission_context.GetPermissionStatus(url, url);
+      EXPECT_EQ(expected, result.content_setting);
+      EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
     }
 
     TestPermissionContext permission_context(profile(), permission_type,
@@ -374,8 +377,9 @@ class PermissionContextBaseTests : public
ChromeRenderViewHostTestHarness {
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    EXPECT_EQ(CONTENT_SETTING_BLOCK,
-              permission_context.GetPermissionStatus(url, url));
+    PermissionResult result = permission_context.GetPermissionStatus(url, url);
+    EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
+    EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
   }
 
   void TestBlockOnSeveralDismissals_TestContent() {
@@ -490,7 +494,10 @@ class PermissionContextBaseTests : public
ChromeRenderViewHostTestHarness {
       EXPECT_EQ(1u, permission_context.decisions().size());
       ASSERT_EQ(expected, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
-      EXPECT_EQ(expected, permission_context.GetPermissionStatus(url, url));
+      PermissionResult result =
+          permission_context.GetPermissionStatus(url, url);
+      EXPECT_EQ(expected, result.content_setting);
+      EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
 
       histograms.ExpectTotalCount(
           "Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i + 1);
@@ -502,8 +509,10 @@ class PermissionContextBaseTests : public
ChromeRenderViewHostTestHarness {
     TestPermissionContext permission_context(
         profile(), content::PermissionType::MIDI_SYSEX,
         CONTENT_SETTINGS_TYPE_MIDI_SYSEX);
-    EXPECT_EQ(CONTENT_SETTING_BLOCK,
-              permission_context.GetPermissionStatus(url, url));
+    PermissionResult result = permission_context.GetPermissionStatus(url, url);
+    EXPECT_EQ(CONTENT_SETTING_BLOCK, result.content_setting);
+    EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
+
     variations::testing::ClearAllVariationParams();
   }
 
@@ -669,8 +678,9 @@ class PermissionContextBaseTests : public
ChromeRenderViewHostTestHarness {
         web_contents(), id, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
-    EXPECT_EQ(expected_permission_status,
-              permission_context.GetPermissionStatus(url, url));
+    PermissionResult result = permission_context.GetPermissionStatus(url, url);
+    EXPECT_EQ(expected_permission_status, result.content_setting);
+    EXPECT_EQ(PermissionStatusSource::UNSPECIFIED, result.source);
 
     if (expected_permission_status == CONTENT_SETTING_ALLOW) {
       ASSERT_EQ(1u, permission_context.decisions().size());

[Peter Beverloo, 2017-02-14 13:51:36]

lgtm

[raymes, 2017-02-15 01:40:24]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-02-15 01:40:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
meredithl@google.com, kcarattini@chromium.org, peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2686463002/#ps100001
(title: "Add a source to the result of
PermissionContextBase::GetPermissionStatus")

[commit-bot: I haz the power, 2017-02-15 01:41:20]

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-15 03:39:16]

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

[commit-bot: I haz the power, 2017-02-15 03:39:17]

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_...)

[raymes, 2017-02-15 05:01:15]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-02-15 05:02: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-15 06:23:29]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487134875303950,
"parent_rev": "73319e0970ff6421af9ebd06e6cef8de27a8d42b", "commit_rev":
"ab35971d1da1c791761f65f8dbd6df9512fd2c4d"}

[commit-bot: I haz the power, 2017-02-15 06:24:21]

Description was changed from

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

BUG=679877
TBR=peter@chromium.org,michaeln@chromium.org,sergeyu@chromium.org
==========

to

==========
Add a source to the result of PermissionContextBase::GetPermissionStatus

There are several different cases where it is helpful to know the source
or reason for a particular permission status being returned. The
immediate reason for needing this is to properly measure metrics for the
PermissionDecisionAutoBlocker.

This CL provides the first steps towards exposing permission decision
information. PermissionContextBase::GetPermissionStatus now returns a
PermissionResult which includes both the content setting being returned,
as well as a PermissionStatusSource which describes the reason for the
setting.

Some initial reasons have been added, specifically:
-MULTIPLE_DISMISSALS - when a permission is blocked as a result of multiple
dismissals.
-SAFE_BROWSING_BLACKLIST - when a permission is blocked because the origin is
on the safe browsing blacklist.

These will be hooked up in a subsequent CL.

BUG=679877
TBR=peter@chromium.org,michaeln@chromium.org,sergeyu@chromium.org

Review-Url: https://codereview.chromium.org/2686463002
Cr-Commit-Position: refs/heads/master@{#450590}
Committed:
https://chromium.googlesource.com/chromium/src/+/ab35971d1da1c791761f65f8dbd6...
==========

[commit-bot: I haz the power, 2017-02-15 06:24:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ab35971d1da1c791761f65f8dbd6...