Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184823007:
Add a feature which, when enabled, blocks permissions after X prompt dismissals. (Closed)
|
Created:
4 years, 4 months ago by dominickn Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, tsergeant, benwells, kcarattini, lgarron Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a feature which, when enabled, blocks permissions after X prompt dismissals.
This CL implements automatic permission blocking after a user has
dismissed a prompt a certain number of times (3 by default). This is
gated behind a feature which is disabled by default. The number of
dismissals required to trigger the blocking is controllable via a
variation parameter. Additional unittests are added to ensure this
behaves as expected, with and without custom parameters from
variations.
This CL also adds a hook to BrowsingDataRemover to remove the
counts when the user clears their site data. Unittests for this are
also implemented.
At each dismiss|ignore of a permission prompt, the number
of preceding dismissals|ignores is logged to UMA so we can
sample how many users are repeatedly dismissing prompts or
repeatedly ignoring prompts.
It is intended to gather metrics (and potentially experiment with)
the dismissal switch starting with M54 on stable to observe whether
it improves permission prompt UX and decision rates.
BUG=632269
Committed: https://crrev.com/6947d75c181dc3f7484b77c25ba053cbda75fbe1
Cr-Commit-Position: refs/heads/master@{#410920}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase #Patch Set 3 : Improve test #
Total comments: 17
Patch Set 4 : Extract into independent class #Patch Set 5 : Remove unnecessary PermissionUtil changes #
Total comments: 18
Patch Set 6 : Metrics. Enable test on Android. Variations test. Clear data when history cleared #Patch Set 7 : Unify implementation in permission_context_base, make log static #
Total comments: 36
Patch Set 8 : Address comments #
Total comments: 8
Patch Set 9 : Add ignore logging and UMA. Addressing comments #Patch Set 10 : Fix unit test #
Total comments: 6
Patch Set 11 : Address comments #
Total comments: 6
Patch Set 12 : Addressing comments, adding BDR test #
Total comments: 2
Patch Set 13 : Addressing nits #
Total comments: 1
Messages
Total messages: 85 (59 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dominickn@chromium.org changed reviewers: + raymes@chromium.org
Hi raymes, PTAL! This is the first time I've worked in permissions code, so please let me know everything that I haven't done right. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times. This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. An additional unittest is added to ensure this behaves as expected. It is intended to experiment with this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. An additional unittest is added to ensure this behaves as expected. It is intended to experiment with this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This is really cool Dom! I made a start at reviewing but didn't have time to finish yet. https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:35: if (!settings) I believe this should never be null https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:56: base::WrapUnique(permission_dict)); It might be potentially confusing that this Get* function modifies the origin_dict. Perhaps we can move this line down to RecordDismissalCount? (it will be called unnecessarily in some cases but it shouldn't be expensive). https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:70: if (!dict.get()) Can this ever be null? https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:76: if (!permission_dict) Can this ever be null? https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:195: int current_dismissal_count = RecordDismissalCount(profile, url, permission); Do we want to record the dismissal count even if the experiment isn't enabled? https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:204: if (base::StringToInt(value, &prompt_dismissals) && prompt_dismissals > 0) Do you know in what cases this won't succeed? If it's only because of a programming error, we could just add else NOTREACHED(). But if it could happen because the user received a broken finch config or something, we should bail out. In the bail out case should we somehow ensure this function always returns false? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to be called below instead of Dismissed. Also, I think we could tweak this a bit for readability (this logic could be beside PermissionUmaUtil::PermissionDismissed). https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:222: allowed = false; I think there is a similar problem here. Thankfully Tim is unifying these codepaths https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:27: bool gUpdatedFromVariations = false; nit: globals tend to use camel case with a g_ prefix https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:234: } nit: suggestion: since there is a reasonable amount of logic here, along with the two globals associated with it, perhaps we should put all of this into a singleton class to make it more self-contained. Take a look at LazyInstance. This would also make it easy to move if we want to move it to PermissionContextBase after Tim does refactoring.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
kcarattini@chromium.org changed reviewers: + kcarattini@chromium.org
Thanks for doing this Dom! https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; On 2016/07/28 08:07:50, raymes wrote: > Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to be > called below instead of Dismissed. Also, I think we could tweak this a bit for > readability (this logic could be beside PermissionUmaUtil::PermissionDismissed). > Is that not what we want? Do we want to measure something else? I'm not sure "dismissed" is the right metric to record in this case. We should decide on what we want to measure and what change in which metrics are required for meaningful results upfront. What does success look like? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:25: int gPromptDismissalsBeforeBlock = 3; This value isn't used unless it's been updated from the variation param, right? Why not initialise it to something less meaningful? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:27: bool gUpdatedFromVariations = false; On 2016/07/28 08:07:50, raymes wrote: > nit: globals tend to use camel case with a g_ prefix Isn't it lowercase with underscores? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:218: int current_dismissal_count = RecordDismissalCount(profile, url, permission); So this count is kept forever, is that correct? It doesn't reset after an hour/day/week? Is that something we want to consider? When the experiment is over, do we want to reset the permission for the affected users?
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
PTAL, thanks https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:35: if (!settings) On 2016/07/28 08:07:50, raymes wrote: > I believe this should never be null Done. https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:56: base::WrapUnique(permission_dict)); On 2016/07/28 08:07:50, raymes wrote: > It might be potentially confusing that this Get* function modifies the > origin_dict. Perhaps we can move this line down to RecordDismissalCount? (it > will be called unnecessarily in some cases but it shouldn't be expensive). I've renamed the method to GetOrCreatePermissionDict; is that OK, or do you prefer doing the setting outside? https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:70: if (!dict.get()) On 2016/07/28 08:07:50, raymes wrote: > Can this ever be null? Done. https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:76: if (!permission_dict) On 2016/07/28 08:07:50, raymes wrote: > Can this ever be null? Done. https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:195: int current_dismissal_count = RecordDismissalCount(profile, url, permission); On 2016/07/28 08:07:50, raymes wrote: > Do we want to record the dismissal count even if the experiment isn't enabled? This was a deliberate choice - the overhead seems minimal, and if we want to promote this experiment to stable then all clients will have the dismissal counts ready to go. WDYT? https://codereview.chromium.org/2184823007/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_util.cc:204: if (base::StringToInt(value, &prompt_dismissals) && prompt_dismissals > 0) On 2016/07/28 08:07:50, raymes wrote: > Do you know in what cases this won't succeed? If it's only because of a > programming error, we could just add else NOTREACHED(). But if it could happen > because the user received a broken finch config or something, we should bail > out. In the bail out case should we somehow ensure this function always returns > false? Hmm, catching a potential bad Finch config seems like a good choice. Done. In the restructured version it will always fall back to the default value (if the experiment is enabled). https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; On 2016/07/28 08:07:50, raymes wrote: > Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to be > called below instead of Dismissed. Also, I think we could tweak this a bit for > readability (this logic could be beside PermissionUmaUtil::PermissionDismissed). > This was a deliberate choice - I wanted the triggering dismissal to simply turn into a deny, which was why I structured the code like this. Do you prefer it some other way? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; On 2016/07/28 08:54:49, kcarattini wrote: > On 2016/07/28 08:07:50, raymes wrote: > > Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to > be > > called below instead of Dismissed. Also, I think we could tweak this a bit for > > readability (this logic could be beside > PermissionUmaUtil::PermissionDismissed). > > > > Is that not what we want? Do we want to measure something else? I'm not sure > "dismissed" is the right metric to record in this case. We should decide on what > we want to measure and what change in which metrics are required for meaningful > results upfront. What does success look like? With respect to metrics, we have a couple of paths here: 1. don't add any specific metrics, and just compare what the existing metrics look like. The ~25% dismissals should drop and denys should increase. 2. add a new specific metric for dismissal -> deny. I thought about this, and wondered whether it was worth adding a new metric per permission type just for this experiment. 3. add a new bucket to the Permission.Action.* metric, which is dismissal -> deny. This was the option I was most leaning towards, but it does mean another (often not meaningful) bucket in that histogram. Thoughts? https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:222: allowed = false; On 2016/07/28 08:07:50, raymes wrote: > I think there is a similar problem here. Thankfully Tim is unifying these > codepaths Acknowledged (see my comment above). https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:25: int gPromptDismissalsBeforeBlock = 3; On 2016/07/28 08:54:49, kcarattini wrote: > This value isn't used unless it's been updated from the variation param, right? > Why not initialise it to something less meaningful? This is now a static constant - it's the default value if nothing is sent from the variation param. https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:27: bool gUpdatedFromVariations = false; On 2016/07/28 08:07:50, raymes wrote: > nit: globals tend to use camel case with a g_ prefix Now a constant not a global. https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:218: int current_dismissal_count = RecordDismissalCount(profile, url, permission); On 2016/07/28 08:54:49, kcarattini wrote: > So this count is kept forever, is that correct? It doesn't reset after an > hour/day/week? Is that something we want to consider? When the experiment is > over, do we want to reset the permission for the affected users? I think that keeping the count forever is correct. When the experiment is over, I don't think we want to set things back the way they were; that's kind of painful to do (a one-time task which checks the count, sees if the permission is denied, and changes it). https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:234: } On 2016/07/28 08:07:50, raymes wrote: > nit: suggestion: since there is a reasonable amount of logic here, along with > the two globals associated with it, perhaps we should put all of this into a > singleton class to make it more self-contained. Take a look at LazyInstance. > This would also make it easy to move if we want to move it to > PermissionContextBase after Tim does refactoring. Done. Naming is hard, please let me know if you have a better name for this class.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for breaking out the new code :) Just a few more comments. https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; On 2016/08/01 01:08:51, dominickn wrote: > On 2016/07/28 08:07:50, raymes wrote: > > Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to > be > > called below instead of Dismissed. Also, I think we could tweak this a bit for > > readability (this logic could be beside > PermissionUmaUtil::PermissionDismissed). > > > > This was a deliberate choice - I wanted the triggering dismissal to simply turn > into a deny, which was why I structured the code like this. Do you prefer it > some other way? This is supposed to measure a user's action on the permission prompt. In this case, the user is still clicking the dismiss button, so I think we should still record a dismiss. We should really update the names of PermissionGranted, etc. to make it clear what we're measuring. As to what metrics we should use, I'm flexible with all those approaches. Adding to Permission.Action isn't the best in my mind because it's not really a separate action from the user's perspective (they're still just dismissing) and it might confuse the dismiss metrics. The others sound good though. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:215: EXPECT_EQ(i+1, permission_context.decisions().size()); nit: i + 1 https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:243: EXPECT_EQ(i+1, permission_context.decisions().size()); nit: i + 1 https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:269: } Should we also test changing the parameter? https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:417: // The permission should be blocked after several dismissals. I think this should work on Android - RespondToPermission looks like it calls into the PermissionQueueController https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:420: TestBlockOnSeveralDismissals_TestContent(); Hmm it feels strange not to inline this, but I see the other tests do it. Can you see a good reason not to? If not maybe we should just inline it. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.cc:82: } // anonymous namespace nit: I think // namespace is more common https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_prompt_decision_log.h (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.h:15: class PermissionPromptDecisionLog { Yeah naming is hard! Maybe PermissionPromptDismissalLog? Or maybe even PermissionPromptDismissalExperiment. I'm ok if we don't feel it's strictly better. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.h:28: nit: no newline https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:242: } I actually suspect we can merge this code into PermissionContextbase now, but let's leave that for a separate CL. Would you be interested in doing this? It would eliminate the code duplication.
https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2184823007/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:224: persist = true; On 2016/08/01 05:09:11, raymes wrote: > On 2016/08/01 01:08:51, dominickn wrote: > > On 2016/07/28 08:07:50, raymes wrote: > > > Hmm I think right now this will cause PermissionUmaUtil::PermissionDenied to > > be > > > called below instead of Dismissed. Also, I think we could tweak this a bit > for > > > readability (this logic could be beside > > PermissionUmaUtil::PermissionDismissed). > > > > > > > This was a deliberate choice - I wanted the triggering dismissal to simply > turn > > into a deny, which was why I structured the code like this. Do you prefer it > > some other way? > > This is supposed to measure a user's action on the permission prompt. In this > case, the user is still clicking the dismiss button, so I think we should still > record a dismiss. We should really update the names of PermissionGranted, etc. > to make it clear what we're measuring. > > As to what metrics we should use, I'm flexible with all those approaches. Adding > to Permission.Action isn't the best in my mind because it's not really a > separate action from the user's perspective (they're still just dismissing) and > it might confuse the dismiss metrics. The others sound good though. As discussed, I think we should move this discussion to a doc. Will be useful for future experiments as well.
battre@chromium.org changed reviewers: + battre@chromium.org
I did not review this in detail but have a thought on this: It would be great if we don't not introduce a counter in your Preferences that shows which domains you have visited without being able to delete the data.
On 2016/08/02 07:09:07, battre wrote: > I did not review this in detail but have a thought on this: It would be great if > we don't not introduce a counter in your Preferences that shows which domains > you have visited without being able to delete the data. Thanks dominic - I was wondering about this. I will add a hook into the browsing data remover to remove the information.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dominickn@chromium.org changed reviewers: + msramek@chromium.org - battre@chromium.org
dominickn@chromium.org changed reviewers: - msramek@chromium.org
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. An additional unittest is added to ensure this behaves as expected. It is intended to experiment with this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. At each dismissal, a count of the preceding dismissals is logged to UMA. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. At each dismissal, a count of the preceding dismissals is logged to UMA. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. At each dismissal, the number of preceding dismissals is logged to UMA so we can sample how many users are repeatedly dismissing prompts. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
PTAL, thanks! https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:215: EXPECT_EQ(i+1, permission_context.decisions().size()); On 2016/08/01 05:09:11, raymes wrote: > nit: i + 1 Done. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:243: EXPECT_EQ(i+1, permission_context.decisions().size()); On 2016/08/01 05:09:11, raymes wrote: > nit: i + 1 Done. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:269: } On 2016/08/01 05:09:11, raymes wrote: > Should we also test changing the parameter? Done. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:417: // The permission should be blocked after several dismissals. On 2016/08/01 05:09:11, raymes wrote: > I think this should work on Android - RespondToPermission looks like it calls > into the PermissionQueueController After much spelunking, got the tests working on Android. Woo! https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:420: TestBlockOnSeveralDismissals_TestContent(); On 2016/08/01 05:09:11, raymes wrote: > Hmm it feels strange not to inline this, but I see the other tests do it. Can > you see a good reason not to? If not maybe we should just inline it. I mainly did it for consistency with the other tests - it seemed strange to have one inlined, but none of the others inlined. Also, the test calls a private method in PermissionPromptDecisionLog, so having the non-inline means less boilerplate to be able to call it. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.cc:82: } // anonymous namespace On 2016/08/01 05:09:11, raymes wrote: > nit: I think // namespace is more common Done. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_prompt_decision_log.h (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.h:15: class PermissionPromptDecisionLog { On 2016/08/01 05:09:11, raymes wrote: > Yeah naming is hard! Maybe PermissionPromptDismissalLog? Or maybe even > PermissionPromptDismissalExperiment. I'm ok if we don't feel it's strictly > better. I didn't want to call it "Dismissal" in case we want to store other decision types in the future. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_prompt_decision_log.h:28: On 2016/08/01 05:09:11, raymes wrote: > nit: no newline Done. https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:242: } On 2016/08/01 05:09:11, raymes wrote: > I actually suspect we can merge this code into PermissionContextbase now, but > let's leave that for a separate CL. Would you be interested in doing this? It > would eliminate the code duplication. I think I can add it to my todo list, but it's a lower priority than other experiments at this point.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Thanks for getting the tests working Dom! The comments below are mainly minor. Maybe Ben can approve next week if I'm not around. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:167: static base::LazyInstance<PermissionPromptDecisionLog> prompt_decision_log_; It actually seems like the overhead would be negligible to just instantiate one of these per PermissionContextBase. It would simplify some of the test code too. wdyt? https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:205: unsigned int iterations) { nit: I think uint32/64_t is preferred over unsigned int https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:235: } nit: as a sanity check against an off-by-one bug in the test logic above, I'd suggest checking that the value is BLOCK underneath this loop. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:359: } A similar sanity check here would be good. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:509: // The permission should be blocked after several dismissals. nit: fill 80 chars on previous line or add a newline before this line https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:27: int kPromptDismissalsBeforeBlock = 3; nit: const int https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:97: // |current_dismissal_count| is always (count_before_this_dismissal + 1) nit: What's count_before_this_dismissal? Maybe just change the _ to spaces so it doesn't sound like a variable name? https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:111: content::PermissionType permission) { Since this code is only used in the test, I'd suggest just moving the implementation into the test (always better to keep test code separate from production code). The only thing needed would be kPromptDismissCountKey which I think is available in the test. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:121: PermissionPromptDecisionLog::kPromptDismissCountKey, ¤t_count); nit: shouldn't need PermissionPromptDecisionLog:: here I don't think https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:141: PermissionPromptDecisionLog::kPromptDismissCountKey, ++current_count); nit: same here and above https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:16: class PermissionPromptDecisionLog { Sorry this is nitpicky but I think this name will confuse people (it sounds too general). How about we call it PermissionDecisionAutoBlocker or something to do with the fact that it will be blocking? https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:18: static const char kPromptDismissCountKey[]; nit: this can be private and the test is a friend anyway. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:21: base::Callback<bool(const GURL& url)> filter); Since this object is a singleton, I'd suggest that all the functions in it could just be member functions without harm https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log_unittest.cc:35: return PermissionPromptDecisionLog::RecordDismissalCount(profile(), url, nit: I think it's usually better to use the public API in tests where possible. In this case it seems like you could just call ShouldChangeDismissalToBlock and use GetDismissalCount to check the counts where needed. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_queue_controller.cc:220: nit: remove from this CL https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_queue_controller.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_queue_controller.h:71: friend class PermissionContextBaseTests; nit: Is this needed? I didn't see anything added to the test that would use it. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:390: // permission action recorded for them. nit: this comment is inaccurate (no dismiss recorded for them)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:167: static base::LazyInstance<PermissionPromptDecisionLog> prompt_decision_log_; On 2016/08/05 03:27:47, raymes wrote: > It actually seems like the overhead would be negligible to just instantiate one > of these per PermissionContextBase. It would simplify some of the test code too. > wdyt? Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:205: unsigned int iterations) { On 2016/08/05 03:27:48, raymes wrote: > nit: I think uint32/64_t is preferred over unsigned int Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:235: } On 2016/08/05 03:27:47, raymes wrote: > nit: as a sanity check against an off-by-one bug in the test logic above, I'd > suggest checking that the value is BLOCK underneath this loop. Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:359: } On 2016/08/05 03:27:48, raymes wrote: > A similar sanity check here would be good. Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:509: // The permission should be blocked after several dismissals. On 2016/08/05 03:27:47, raymes wrote: > nit: fill 80 chars on previous line or add a newline before this line Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:27: int kPromptDismissalsBeforeBlock = 3; On 2016/08/05 03:27:48, raymes wrote: > nit: const int Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:97: // |current_dismissal_count| is always (count_before_this_dismissal + 1) On 2016/08/05 03:27:48, raymes wrote: > nit: What's count_before_this_dismissal? Maybe just change the _ to spaces so it > doesn't sound like a variable name? Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:111: content::PermissionType permission) { On 2016/08/05 03:27:48, raymes wrote: > Since this code is only used in the test, I'd suggest just moving the > implementation into the test (always better to keep test code separate from > production code). The only thing needed would be kPromptDismissCountKey which I > think is available in the test. Both GetOriginDict() and GetOrCreatePermissionDict() are defined only in the anonymous namespace in this file, and both of them are required for this method. Either those would need to be exposed in this class (worse), or they'd need to be duplicated in the test (bad), so I think the best option is to keep this method here. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:121: PermissionPromptDecisionLog::kPromptDismissCountKey, ¤t_count); On 2016/08/05 03:27:48, raymes wrote: > nit: shouldn't need PermissionPromptDecisionLog:: here I don't think Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:141: PermissionPromptDecisionLog::kPromptDismissCountKey, ++current_count); On 2016/08/05 03:27:48, raymes wrote: > nit: same here and above Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:16: class PermissionPromptDecisionLog { On 2016/08/05 03:27:48, raymes wrote: > Sorry this is nitpicky but I think this name will confuse people (it sounds too > general). How about we call it PermissionDecisionAutoBlocker or something to do > with the fact that it will be blocking? Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:18: static const char kPromptDismissCountKey[]; On 2016/08/05 03:27:48, raymes wrote: > nit: this can be private and the test is a friend anyway. Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.h:21: base::Callback<bool(const GURL& url)> filter); On 2016/08/05 03:27:48, raymes wrote: > Since this object is a singleton, I'd suggest that all the functions in it could > just be member functions without harm RemoveCountsByUrl is called from the BrowsingDataRemover, which doesn't have any way to get to this object. I think that method needs to be static. Otherwise updated. Also, this required some changes to the unittest to get right. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log_unittest.cc:35: return PermissionPromptDecisionLog::RecordDismissalCount(profile(), url, On 2016/08/05 03:27:48, raymes wrote: > nit: I think it's usually better to use the public API in tests where possible. > In this case it seems like you could just call ShouldChangeDismissalToBlock and > use GetDismissalCount to check the counts where needed. Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_queue_controller.cc:220: On 2016/08/05 03:27:48, raymes wrote: > nit: remove from this CL Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_queue_controller.h (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_queue_controller.h:71: friend class PermissionContextBaseTests; On 2016/08/05 03:27:48, raymes wrote: > nit: Is this needed? I didn't see anything added to the test that would use it. Done. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:390: // permission action recorded for them. On 2016/08/05 03:27:48, raymes wrote: > nit: this comment is inaccurate (no dismiss recorded for them) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
permissions lgtm with a few comments :) https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:111: content::PermissionType permission) { On 2016/08/05 04:29:30, dominickn wrote: > On 2016/08/05 03:27:48, raymes wrote: > > Since this code is only used in the test, I'd suggest just moving the > > implementation into the test (always better to keep test code separate from > > production code). The only thing needed would be kPromptDismissCountKey which > I > > think is available in the test. > > Both GetOriginDict() and GetOrCreatePermissionDict() are defined only in the > anonymous namespace in this file, and both of them are required for this method. > Either those would need to be exposed in this class (worse), or they'd need to > be duplicated in the test (bad), so I think the best option is to keep this > method here. Ah that makes sense. In that case we always suffix test-only functions with ForTest. So this should be called GetDismissalCountForTest https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (left): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:39: // static nit: probably leave this // static as I think it is in-line with the style guide. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:167: base::LazyInstance<PermissionDecisionAutoBlocker> decision_auto_blocker_ = I think we can just use a unique_ptr here (or instantiate the thing directly?) Depending on which way you go you may be able to forward declare the class and only add the #include in the .cc file. If we do that, we can also pass the profile in as a member variable and avoid having to pass it every function call. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:59: const char PermissionDecisionAutoBlocker::kPromptDismissCountKey[] = nit: // static https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:359: void PermissionUmaUtil::PermissionPromptDismissCount( Please get Kendra to take a look at this for the metrics side :)
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. The origin + dismissal count is a form of history, so this CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their history. Unittests for this are also implemented. At each dismissal, the number of preceding dismissals is logged to UMA so we can sample how many users are repeatedly dismissing prompts. It is intended to gather metrics (and potentially experiment with) this starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismissal|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! This new version also adds logging and UMA metrics for ignores on prompts. https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_prompt_decision_log.cc (right): https://codereview.chromium.org/2184823007/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_prompt_decision_log.cc:111: content::PermissionType permission) { On 2016/08/06 00:50:21, raymes (OOO until 15 Aug) wrote: > On 2016/08/05 04:29:30, dominickn wrote: > > On 2016/08/05 03:27:48, raymes wrote: > > > Since this code is only used in the test, I'd suggest just moving the > > > implementation into the test (always better to keep test code separate from > > > production code). The only thing needed would be kPromptDismissCountKey > which > > I > > > think is available in the test. > > > > Both GetOriginDict() and GetOrCreatePermissionDict() are defined only in the > > anonymous namespace in this file, and both of them are required for this > method. > > Either those would need to be exposed in this class (worse), or they'd need to > > be duplicated in the test (bad), so I think the best option is to keep this > > method here. > > Ah that makes sense. In that case we always suffix test-only functions with > ForTest. So this should be called GetDismissalCountForTest Done. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (left): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:39: // static On 2016/08/06 00:50:21, raymes (OOO until 15 Aug) wrote: > nit: probably leave this // static as I think it is in-line with the style > guide. Done. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:167: base::LazyInstance<PermissionDecisionAutoBlocker> decision_auto_blocker_ = On 2016/08/06 00:50:21, raymes (OOO until 15 Aug) wrote: > I think we can just use a unique_ptr here (or instantiate the thing directly?) > Depending on which way you go you may be able to forward declare the class and > only add the #include in the .cc file. If we do that, we can also pass the > profile in as a member variable and avoid having to pass it every function call. Done. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:59: const char PermissionDecisionAutoBlocker::kPromptDismissCountKey[] = On 2016/08/06 00:50:21, raymes (OOO until 15 Aug) wrote: > nit: // static Done. https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2184823007/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:359: void PermissionUmaUtil::PermissionPromptDismissCount( On 2016/08/06 00:50:21, raymes (OOO until 15 Aug) wrote: > Please get Kendra to take a look at this for the metrics side :) Will do.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dominickn@chromium.org changed reviewers: + isherman@chromium.org, msramek@chromium.org, thestig@chromium.org
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismissal|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismiss|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
+msramek: PTAL at the browsing data remover, and advise whether removing this new count with site data is appropriate +isherman: PTAL at histograms. +thestig: PTAL at chrome/common/ kcarattini: please make sure the permission metrics are sane. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:100: RecordAction(url, permission, kPromptIgnoreCountKey); Can you rename RecordAction to make it more clear how it differs from PermissionPromptIgnoreCount? Maybe RecordActionInContentSetting or something. https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:102: // |current_ignore_count| is always (count before this ignore + 1) I find it confusing that this and the previous RecordAction method are recording different numbers. Can you either 1) record the same value (i.e. number of dismissals) or 2) Rename the UMA metrics to indicate how they differ from the number of dismissals count? You've got it in the description already but the name is misleading. https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.h:119: static void PermissionPromptDismissCount(content::PermissionType permission, nit: Maybe "RecordPermissionPrompt....", otherwise I half-expected it to return a count. I'm not sure what you mean by "prior to the time when this function is called". Doesn't it just record the passed in |count|? Same below.
https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:100: RecordAction(url, permission, kPromptIgnoreCountKey); On 2016/08/08 06:14:31, kcarattini wrote: > Can you rename RecordAction to make it more clear how it differs from > PermissionPromptIgnoreCount? Maybe RecordActionInContentSetting or something. Done. https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:102: // |current_ignore_count| is always (count before this ignore + 1) On 2016/08/08 06:14:31, kcarattini wrote: > I find it confusing that this and the previous RecordAction method are recording > different numbers. > > Can you either 1) record the same value (i.e. number of dismissals) or 2) Rename > the UMA metrics to indicate how they differ from the number of dismissals count? > You've got it in the description already but the name is misleading. Done. https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2184823007/diff/180001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.h:119: static void PermissionPromptDismissCount(content::PermissionType permission, On 2016/08/08 06:14:31, kcarattini wrote: > nit: Maybe "RecordPermissionPrompt....", otherwise I half-expected it to return > a count. > > I'm not sure what you mean by "prior to the time when this function is called". > Doesn't it just record the passed in |count|? > > Same below. Done.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Kudos for proactively implementing this with a URL filter :) https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:795: Please add a test to BrowsingDataRemoverTest. https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:796: PermissionDecisionAutoBlocker::RemoveCountsByUrl(profile_, filter); The guarantee that we want to give when the user deletes history is that no URLs remain stored anywhere (except search engines and content settings which have their own UI, so they're auditable separately). So please add a REMOVE_HISTORY condition as well - for example by moving this three lines below. Unless this listens to local history item deletions, as SITE_ENGAGEMENT does (IIRC). In that case it's OK not to specify REMOVE_HISTORY explicitly, but add a comment instead.
Metrics LGTM % a request for clarification: https://codereview.chromium.org/2184823007/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184823007/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39512: + the total number of prompt dismissal events for this origin, inclusive of Is this the total number across all of time? Or over the last N months? That is, does the dismissal data ever expire?
On 2016/08/08 04:13:07, dominickn wrote: > +thestig: PTAL at chrome/common/ lgtm
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
msramek: Thanks! PTAL. https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:795: On 2016/08/08 17:09:53, msramek wrote: > Please add a test to BrowsingDataRemoverTest. Done. https://codereview.chromium.org/2184823007/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:796: PermissionDecisionAutoBlocker::RemoveCountsByUrl(profile_, filter); On 2016/08/08 17:09:53, msramek wrote: > The guarantee that we want to give when the user deletes history is that no URLs > remain stored anywhere (except search engines and content settings which have > their own UI, so they're auditable separately). > > So please add a REMOVE_HISTORY condition as well - for example by moving this > three lines below. > > Unless this listens to local history item deletions, as SITE_ENGAGEMENT does > (IIRC). In that case it's OK not to specify REMOVE_HISTORY explicitly, but add a > comment instead. Done. https://codereview.chromium.org/2184823007/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184823007/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39512: + the total number of prompt dismissal events for this origin, inclusive of On 2016/08/08 20:04:56, Ilya Sherman wrote: > Is this the total number across all of time? Or over the last N months? That > is, does the dismissal data ever expire? The dismissal data never expires - though it will be reset if the user clears their browsing data. I'll clarify.
LGTM % style nits. Thanks for the thorough test! https://codereview.chromium.org/2184823007/diff/220001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1029: blocker_(new PermissionDecisionAutoBlocker(profile)) { } nit: 4 spaces offset w.r.t. the previous line, no space between {}, and IIRC the colon belongs on the next line https://codereview.chromium.org/2184823007/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2654: content::PermissionType::MIDI_SYSEX); nit: alignment
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, kcarattini@chromium.org, thestig@chromium.org, isherman@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2184823007/#ps240001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismiss|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismiss|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismiss|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 ========== to ========== Add a feature which, when enabled, blocks permissions after X prompt dismissals. This CL implements automatic permission blocking after a user has dismissed a prompt a certain number of times (3 by default). This is gated behind a feature which is disabled by default. The number of dismissals required to trigger the blocking is controllable via a variation parameter. Additional unittests are added to ensure this behaves as expected, with and without custom parameters from variations. This CL also adds a hook to BrowsingDataRemover to remove the counts when the user clears their site data. Unittests for this are also implemented. At each dismiss|ignore of a permission prompt, the number of preceding dismissals|ignores is logged to UMA so we can sample how many users are repeatedly dismissing prompts or repeatedly ignoring prompts. It is intended to gather metrics (and potentially experiment with) the dismissal switch starting with M54 on stable to observe whether it improves permission prompt UX and decision rates. BUG=632269 Committed: https://crrev.com/6947d75c181dc3f7484b77c25ba053cbda75fbe1 Cr-Commit-Position: refs/heads/master@{#410920} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/6947d75c181dc3f7484b77c25ba053cbda75fbe1 Cr-Commit-Position: refs/heads/master@{#410920}
Message was sent while issue was closed.
https://codereview.chromium.org/2184823007/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2184823007/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:292: base::FeatureList::ClearInstanceForTesting(); If you read the comments for ClearInstanceForTesting(), you should know not to use it, because you leave the base::FeatureList global instance as a nullptr at the end. https://codereview.chromium.org/2373883002 changes this test to use a base::test::ScopedFeatureList. |
