Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684913002:
Add UMA for recording embargo reasons and autoblocker interactions. (Closed)
|
Created:
3 years, 10 months ago by meredithl Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, asvitkine+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for recording embargo reasons and autoblocker interactions.
PermissionContextBase will now record interactions with
PermissionDecisionAutoBlocker, in particular those that lead to embargoing of
permission requests. Permission requests will fall into one of the following
buckets: Embargo due to blacklisting, embargo due to dismissals, or no embargo.
The purpose of this UMA is to record when and for what reason requests are
placed under embargo. A follow up CL will provide UMA for recording how often
permission prompts aren't shown due to existing embargo, and another will cover
repeated embargo.
BUG=679877
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 10
Patch Set 3 : Review #
Total comments: 5
Patch Set 4 : Change enum name. #Patch Set 5 : Add android embargo tracking. #
Total comments: 4
Patch Set 6 : Histograms description expansion. #Patch Set 7 : Fix android include. #Patch Set 8 : Rebase forever #Messages
Total messages: 48 (33 generated)
The CQ bit was checked by meredithl@google.com 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...
meredithl@google.com changed reviewers: + dominickn@chromium.org, kcarattini@chromium.org, raymes@chromium.org
Hey guys, this is one part of the UMA for embargoing. PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by meredithl@google.com 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by meredithl@google.com 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...
https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:318: if (content_setting == CONTENT_SETTING_DEFAULT) { Can we move the contents of this if-block into the else statement above? It should be the same codepath. https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:325: content_setting = CONTENT_SETTING_BLOCK; nit: Can we actually remove this line? I think the request will be blocked as a result of the dismiss, so I don't think it's actually needed. https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:899: } Are we testing that NOT_EMBARGOED will be recorded when the prompt is allowed/blocked/ignored? Is that hard to test? https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:127: PermissionEmbargoReason embargo_reason); nit: please add a newline after this
The CQ bit was checked by meredithl@google.com 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...
https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:318: if (content_setting == CONTENT_SETTING_DEFAULT) { On 2017/02/08 23:20:09, raymes wrote: > Can we move the contents of this if-block into the else statement above? It > should be the same codepath. Done. https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:325: content_setting = CONTENT_SETTING_BLOCK; On 2017/02/08 23:20:09, raymes wrote: > nit: Can we actually remove this line? I think the request will be blocked as a > result of the dismiss, so I don't think it's actually needed. Done. I had to change the test logic slightly to expect a CONTENT_SETTING_ASK rather than a BLOCK, but the block is still reflected in GetPermissionStatus so still works as intended. Good catch. https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:899: } On 2017/02/08 23:20:09, raymes wrote: > Are we testing that NOT_EMBARGOED will be recorded when the prompt is > allowed/blocked/ignored? Is that hard to test? Not at all. I've added a histogram check into TestAskAndDecide to check that only NOT_EMBARGOED is recorded, and that test is only called for the allow/block/ignore content settings. https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:127: PermissionEmbargoReason embargo_reason); On 2017/02/08 23:20:09, raymes wrote: > nit: please add a newline after this Done.
https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:68: enum PermissionEmbargoReason { Nit: What about PermissionEmbargoStatus instead of "reason"? I think reason sounds a bit odd in the case where it is not embargoed.
https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:316: if (PermissionDecisionAutoBlocker::GetForProfile(profile_) All of this logic needs to be added to PermissionQueueController::OnPermissionSet to make sure it works on Android. :( https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_request_impl.cc:42: PermissionEmbargoReason::NOT_EMBARGOED); This also needs to be called in PermissionInfoBarDelegate::~PermissionInfoBarDelegate so it's recorded on Android.
On 2017/02/09 00:54:11, dominickn wrote: > https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... > File chrome/browser/permissions/permission_context_base.cc (right): > > https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... > chrome/browser/permissions/permission_context_base.cc:316: if > (PermissionDecisionAutoBlocker::GetForProfile(profile_) > All of this logic needs to be added to > PermissionQueueController::OnPermissionSet to make sure it works on Android. :( > > https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... > File chrome/browser/permissions/permission_request_impl.cc (right): > > https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... > chrome/browser/permissions/permission_request_impl.cc:42: > PermissionEmbargoReason::NOT_EMBARGOED); > This also needs to be called in > PermissionInfoBarDelegate::~PermissionInfoBarDelegate so it's recorded on > Android. (Android stuff can be done in a separate CL)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
meredithl@google.com changed reviewers: + rkaplow@chromium.org
Hey Robert, PTAL, thanks :)
Okay, follow up CL to include metrics in the right places for Android. Cheers! https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2684913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:68: enum PermissionEmbargoReason { On 2017/02/09 00:17:33, kcarattini wrote: > Nit: What about PermissionEmbargoStatus instead of "reason"? I think reason > sounds a bit odd in the case where it is not embargoed. Done. https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:316: if (PermissionDecisionAutoBlocker::GetForProfile(profile_) On 2017/02/09 00:54:11, dominickn wrote: > All of this logic needs to be added to > PermissionQueueController::OnPermissionSet to make sure it works on Android. :( Acknowledged. https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_impl.cc (right): https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_request_impl.cc:42: PermissionEmbargoReason::NOT_EMBARGOED); On 2017/02/09 00:54:11, dominickn wrote: > This also needs to be called in > PermissionInfoBarDelegate::~PermissionInfoBarDelegate so it's recorded on > Android. Acknowledged.
The CQ bit was checked by meredithl@google.com 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2684913002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:316: if (PermissionDecisionAutoBlocker::GetForProfile(profile_) On 2017/02/09 00:54:11, dominickn wrote: > All of this logic needs to be added to > PermissionQueueController::OnPermissionSet to make sure it works on Android. :( 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lg besides one comment https://codereview.chromium.org/2684913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2684913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:246: ->RecordDismissAndEmbargo(requesting_origin, permission_type_)) { Unfortunately this is going to be untested code :( I think we would want to make sure we have tests before shipping this to stable, which we should get for free after the refactoring is done. I'm less concerned about the metrics, though it's good to have tests for those too. dominickn@ what do you think?
lgtm histogram lg https://codereview.chromium.org/2684913002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2684913002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45992: + Tracks the reason that an (origin, permission) pair has been placed under can you add more detail on when this is triggered
https://codereview.chromium.org/2684913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2684913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_queue_controller.cc:246: ->RecordDismissAndEmbargo(requesting_origin, permission_type_)) { On 2017/02/09 05:46:39, raymes wrote: > Unfortunately this is going to be untested code :( I think we would want to make > sure we have tests before shipping this to stable, which we should get for free > after the refactoring is done. > > I'm less concerned about the metrics, though it's good to have tests for those > too. > > dominickn@ what do you think? We could look at adding Java side tests for this. That should be fairly straightforward.
https://codereview.chromium.org/2684913002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2684913002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45992: + Tracks the reason that an (origin, permission) pair has been placed under On 2017/02/09 19:07:07, rkaplow wrote: > can you add more detail on when this is triggered Done.
The CQ bit was checked by meredithl@google.com 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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by meredithl@google.com 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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add UMA for recording embargo reasons and autoblocker interactions. PermissionContextBase will now record interactions with PermissionDecisionAutoBlocker, in particular those that lead to embargoing of permission requests. Permission requests will fall into one of the following buckets: Embargo due to blacklisting, embargo due to dismissals, or no embargo. The purpose of this UMA is to record when and for what reason requests are placed under embargo. A follow up CL will provide UMA for recording how often permission prompts aren't shown due to existing embargo, and another will cover repeated embargo. BUG=679877 ========== to ========== Add UMA for recording embargo reasons and autoblocker interactions. PermissionContextBase will now record interactions with PermissionDecisionAutoBlocker, in particular those that lead to embargoing of permission requests. Permission requests will fall into one of the following buckets: Embargo due to blacklisting, embargo due to dismissals, or no embargo. The purpose of this UMA is to record when and for what reason requests are placed under embargo. A follow up CL will provide UMA for recording how often permission prompts aren't shown due to existing embargo, and another will cover repeated embargo. BUG=679877 ==========
Message was sent while issue was closed.
Closing this issue since meredithl@ has finished her internship. :( Taking this over in https://codereview.chromium.org/2690543004/
Message was sent while issue was closed.
lgtm once the merge issue is fixed. |
