Content Settings: Add RevocationObserver to measure when permissions are revoked

Content Settings: Add method to measure when permissions are revoked

This CL adds a new method which wraps SetContentSettingDefaultScope to
determine when a permission is revoked. This allows us to collect
revocation metrics from Android Site Settings and desktop Page Action
bubbles.

BUG=469221
Committed: https://crrev.com/aa3dab6655aa3e927fb381691824f471172ce479
Cr-Commit-Position: refs/heads/master@{#385132}

[tsergeant, 2016-03-15 23:03:16]

Description was changed from

==========
[Do not commit] Add RevocationObserver to collect metrics when permissions are
revoked

BUG=
==========

to

==========
Content Settings: Add RevocationObserver to measure when permissions are revoked

We implement the observer to collect UMA and RAPPOR metrics for permission
revocation.

BUG=469221
==========

[tsergeant, 2016-03-15 23:03:55]

Description was changed from

==========
Content Settings: Add RevocationObserver to measure when permissions are revoked

We implement the observer to collect UMA and RAPPOR metrics for permission
revocation.

BUG=469221
==========

to

==========
Content Settings: Add RevocationObserver to measure when permissions are revoked

We implement the observer to collect UMA and RAPPOR metrics for permission
revocation.

BUG=469221
==========

[tsergeant, 2016-03-15 23:05:52]

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

[tsergeant, 2016-03-15 23:05:53]

Can you please take an initial look?

I still have the problem where PermissionManager does not start up until a
permission bubble is shown, but I'm not sure how to change this.

[mlamouri (slow - plz ping), 2016-03-16 11:07:03]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-03-16 11:07:04]

That sounds like a lot of code added just to do the UMA on the
chrome/browser/permissions/ layer. Is there any other usage of the revocation
observer planned?

Also, wouldn't that be easier to reuse OnContentSettingsChanged? I understand
that we would need to figure out if the permission was revoked but it might be
less boilerplate?

[tsergeant, 2016-03-16 22:46:29]

On 2016/03/16 11:07:04, Mounir Lamouri wrote:
> That sounds like a lot of code added just to do the UMA on the
> chrome/browser/permissions/ layer. Is there any other usage of the revocation
> observer planned?

Kendra is interested in using it for her work with Chrome Sync,
although it hasn't been decided yet for sure.

> Also, wouldn't that be easier to reuse OnContentSettingsChanged? I understand
> that we would need to figure out if the permission was revoked but it might be
> less boilerplate?

I took a look into doing this. There's a very preliminary CL here:
https://codereview.chromium.org/1767423002/

I personally feel that this version with the new Observer class is simpler, more
correct, and fits better into Raymes' proposed Permissions Layer refactor.

[raymes, 2016-03-17 02:05:43]

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:445: if
(PermissionUtil::GetPermissionType(content_type, &permission_type)) {
nit: no {}

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_observer.h
(right):

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_observer.h:29: virtual
void OnContentSettingRevoked(
What are the downsides to adding this to Observer above?

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:473: bool
final_value_not_allowed =
Can you just check whether it's not ALLOW?

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:787:
resource_identifier));
nit: perhaps just inline this for now?

[kcarattini, 2016-03-17 02:51:21]

kcarattini@chromium.org changed reviewers:
+ kcarattini@chromium.org

[kcarattini, 2016-03-17 02:51:22]

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1110: //
Allow->Default triggers a revocation when default is 'ask'.
There's no way to clear or reset all permissions from the settings menu at the
moment, but I know people are interested in making that happen. If it does,
we'll need to think about whether resetting all permissions should count as a
revocation.

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:446:
PermissionUmaUtil::PermissionRevoked(permission_type, primary_url);
Is going to change how often revocation is reported? How about adding a comment
on the RAPPOR metric with the date of the reporting change release, that way we
won't mistake it for a change in user behaviour?

[tsergeant, 2016-03-17 06:25:40]

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1110: //
Allow->Default triggers a revocation when default is 'ask'.
On 2016/03/17 02:51:22, kcarattini wrote:
> There's no way to clear or reset all permissions from the settings menu at the
> moment, but I know people are interested in making that happen. If it does,
> we'll need to think about whether resetting all permissions should count as a
> revocation.

Hmm, good point. Under this system the 'Clear & Reset' button in the Android
Site Settings page will count as revoking each individual permission, which I
think is a good thing. I don't think we would want revoking every single
permission on the device to count, though.

It will also depend on the implementation of that button as to whether this code
gets called.

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:445: if
(PermissionUtil::GetPermissionType(content_type, &permission_type)) {
On 2016/03/17 02:05:43, raymes wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1803973002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:446:
PermissionUmaUtil::PermissionRevoked(permission_type, primary_url);
On 2016/03/17 02:51:22, kcarattini wrote:
> Is going to change how often revocation is reported? How about adding a
comment
> on the RAPPOR metric with the date of the reporting change release, that way
we
> won't mistake it for a change in user behaviour?

Good idea, done.

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_observer.h
(right):

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_observer.h:29: virtual
void OnContentSettingRevoked(
On 2016/03/17 02:05:43, raymes wrote:
> What are the downsides to adding this to Observer above?

I was having some problems with it initially, but I've added it to Observer with
a blank implementation and it works fine.

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:473: bool
final_value_not_allowed =
On 2016/03/17 02:05:43, raymes wrote:
> Can you just check whether it's not ALLOW?

Doing that would not trigger in the case where the permission goes from ALLOW ->
ASK and the default for that content setting type is ALLOW. 

I think this goes back to the question about whether we want to capture user
intent, or capture exactly what happens in the backend.

Kendra, what do you think?

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:787:
resource_identifier));
On 2016/03/17 02:05:43, raymes wrote:
> nit: perhaps just inline this for now?

Done.

[mlamouri (slow - plz ping), 2016-03-18 14:11:48]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org

[raymes, 2016-03-21 01:52:57]

https://codereview.chromium.org/1803973002/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_observer.h
(right):

https://codereview.chromium.org/1803973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_observer.h:23: virtual
void OnContentSettingRevoked(
nit: perhaps add a comment about when this is called? Specifically it's only
fired if a content setting goes from ALLOW to something else when the user's
preference for a particular origin is changed (via
SetContentSettingDefaultScope)

[tsergeant, 2016-03-21 03:38:38]

Bernhard, can you please take a look? This is something that would fit better
higher up in the permissions layer, but that's not possible at the moment.

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1803973002/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:473: bool
final_value_not_allowed =
On 2016/03/17 06:25:40, tsergeant wrote:
> On 2016/03/17 02:05:43, raymes wrote:
> > Can you just check whether it's not ALLOW?
> 
> Doing that would not trigger in the case where the permission goes from ALLOW
->
> ASK and the default for that content setting type is ALLOW. 
> 
> I think this goes back to the question about whether we want to capture user
> intent, or capture exactly what happens in the backend.
> 
> Kendra, what do you think?

As discussed, I've simplified this logic without changing the behavior. I was
misunderstanding how content settings works.

https://codereview.chromium.org/1803973002/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_observer.h
(right):

https://codereview.chromium.org/1803973002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_observer.h:23: virtual
void OnContentSettingRevoked(
On 2016/03/21 01:52:56, raymes wrote:
> nit: perhaps add a comment about when this is called? Specifically it's only
> fired if a content setting goes from ALLOW to something else when the user's
> preference for a particular origin is changed (via
> SetContentSettingDefaultScope)

Done.

[raymes, 2016-03-21 06:14:02]

lgtm (but as discussed we'll make sure bauerb@ is ok with this)

[kcarattini, 2016-03-21 10:40:27]

lgtm

[Bernhard Bauer, 2016-03-21 11:56:31]

I find this a bit... odd. For one, tracking "revocations" implies that a
permission is being revoked, which is something that we are trying to remove
from content_settings. Also, the new interface seems to be tailored for one
specific use case, as it is only concerned with user changes to content settings
for origin pairs. As such, this basically tracks usage of one specific method on
HCSM, right? Couldn't we just instrument the call sites to that method? Ideally,
we could add a wrapper method in permissions that does this, and have all the
call sites go through that. Layering! 😃

https://codereview.chromium.org/1803973002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/1803973002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:89: //
content_settings::RevocationObserver implementation.
Nit: s/Revocation//

[tsergeant, 2016-03-22 05:43:16]

Hi Bernhard, thanks for your feedback. 

I've uploaded a new patch which takes a different approach based on your
suggestions. I've added a new method to instrument calls from the specific sites
we're interested. In the longer term, the plan is that there'll be a permanent
home for this instrumentation more centrally within the permissions layer. In
the mean-time, this approach avoids adding extra complexity to content settings.

Please take a look, we're hoping for this to land in M51 so we can start
collecting data.

[tsergeant, 2016-03-22 05:46:22]

Description was changed from

==========
Content Settings: Add RevocationObserver to measure when permissions are revoked

We implement the observer to collect UMA and RAPPOR metrics for permission
revocation.

BUG=469221
==========

to

==========
Content Settings: Add method to measure when permissions are revoked

This CL adds a new method which wraps SetContentSettingDefaultScope to
determine when a permission is revoked. This allows us to collect
revocation metrics from Android Site Settings and desktop Page Action
bubbles.

BUG=469221
==========

[Bernhard Bauer, 2016-03-22 10:13:30]

That's much nicer, thanks!

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:13: 
Nit: This empty line is unnecessary.

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

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:45: static bool
SetContentSettingRecordRevocation(
Nit: SetContentSetting*And*RecordRevocation maybe?

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util_unittest.cc (right):

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util_unittest.cc:24:
map->SetContentSettingDefaultScope(host, host, type, "",
Nit: Use an empty std::string() constructor instead of a literal to save a
couple of instructions. Less important in a test, but tests are often used as
examples of usage, so I'd like to behave there ☺

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util_unittest.cc:26:
EXPECT_TRUE(PermissionUtil::SetContentSettingRecordRevocation(
If the return value is only used for tests, it might be better to verify that
this method actually records the metric and doesn't just claim it did ☺ I think
you should be able to use base::HistogramTester to verify what gets recorded.

[tsergeant, 2016-03-22 23:55:20]

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:13: 
On 2016/03/22 10:13:29, Bernhard Bauer wrote:
> Nit: This empty line is unnecessary.

Done.

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

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:45: static bool
SetContentSettingRecordRevocation(
On 2016/03/22 10:13:29, Bernhard Bauer wrote:
> Nit: SetContentSetting*And*RecordRevocation maybe?

Done.

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util_unittest.cc (right):

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util_unittest.cc:24:
map->SetContentSettingDefaultScope(host, host, type, "",
On 2016/03/22 10:13:30, Bernhard Bauer wrote:
> Nit: Use an empty std::string() constructor instead of a literal to save a
> couple of instructions. Less important in a test, but tests are often used as
> examples of usage, so I'd like to behave there ☺

Done.

https://codereview.chromium.org/1803973002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util_unittest.cc:26:
EXPECT_TRUE(PermissionUtil::SetContentSettingRecordRevocation(
On 2016/03/22 10:13:29, Bernhard Bauer wrote:
> If the return value is only used for tests, it might be better to verify that
> this method actually records the metric and doesn't just claim it did ☺ I
think
> you should be able to use base::HistogramTester to verify what gets recorded.

Done. I don't want to get too deep into testing the implementation of
PermissionUmaUtil, I've made a TODO for doing that separately.

[tsergeant, 2016-03-29 00:08:30]

Ping bauerb/mlamouri: Can you please take another look?

[Bernhard Bauer, 2016-03-29 08:25:38]

LGTM, thanks!

[tsergeant, 2016-03-30 23:49:22]

+felt@ for c/b/permissions/ and the comment in
c/b/ui/website_settings/website_settings.cc.

[tsergeant, 2016-03-30 23:50:29]

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

[tsergeant, 2016-03-30 23:50:30]

...actually +felt@ this time (for c/b/permissions/ and the comment in
c/b/ui/website_settings/website_settings.cc)

Thanks!

[mlamouri (slow - plz ping), 2016-03-31 10:10:15]

lgtm

[mlamouri (slow - plz ping), 2016-03-31 10:10:34]

Sorry for the delay, I was offline for the last few days.

[tsergeant, 2016-03-31 22:24:05]

On 2016/03/31 10:10:34, Mounir Lamouri wrote:
> Sorry for the delay, I was offline for the last few days.

No problem, thanks for reviewing!

[tsergeant, 2016-03-31 22:43:09]

tsergeant@chromium.org changed reviewers:
+ holte@google.com

[tsergeant, 2016-03-31 22:43:10]

+holte@ for rappor.xml

[Steven Holte, 2016-04-01 18:26:40]

On 2016/03/31 22:43:10, tsergeant wrote:
> +holte@ for rappor.xml

lgtm

[felt, 2016-04-04 13:20:27]

just a nit/question

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

https://codereview.chromium.org/1803973002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:44: // crbug.com/469221.
Seems a little weird to have this return a boolean for revocation. I'm wondering
why any caller would care about it. Is that to make testing easier?

[tsergeant, 2016-04-04 23:48:29]

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

https://codereview.chromium.org/1803973002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:44: // crbug.com/469221.
On 2016/04/04 13:20:27, felt wrote:
> Seems a little weird to have this return a boolean for revocation. I'm
wondering
> why any caller would care about it. Is that to make testing easier?

Yeah, it was only used for testing. I've removed it, since I'm using
HistogramTester now anyway.

[felt, 2016-04-05 01:15:23]

lgtm

[tsergeant, 2016-04-05 07:42:40]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-04-05 07:42:40]

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

[commit-bot: I haz the power, 2016-04-05 07:42:51]

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

[commit-bot: I haz the power, 2016-04-05 08:14:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-05 08:15:54]

Description was changed from

==========
Content Settings: Add method to measure when permissions are revoked

This CL adds a new method which wraps SetContentSettingDefaultScope to
determine when a permission is revoked. This allows us to collect
revocation metrics from Android Site Settings and desktop Page Action
bubbles.

BUG=469221
==========

to

==========
Content Settings: Add method to measure when permissions are revoked

This CL adds a new method which wraps SetContentSettingDefaultScope to
determine when a permission is revoked. This allows us to collect
revocation metrics from Android Site Settings and desktop Page Action
bubbles.

BUG=469221

Committed: https://crrev.com/aa3dab6655aa3e927fb381691824f471172ce479
Cr-Commit-Position: refs/heads/master@{#385132}
==========

[commit-bot: I haz the power, 2016-04-05 08:15:57]

Patchset 10 (id:??) landed as
https://crrev.com/aa3dab6655aa3e927fb381691824f471172ce479
Cr-Commit-Position: refs/heads/master@{#385132}