Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250993002:
Add prior dismissal and ignore count metrics for all permission actions. (Closed)
|
Created:
4 years, 4 months ago by dominickn Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, asvitkine+watch_chromium.org, kcarattini Base URL:
https://chromium.googlesource.com/chromium/src.git@kendra-permission-action-reporting Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd prior dismissal and ignore count metrics for all permission actions.
This CL removes the recently added Permission.Prompt.DismissCount and
Permission.Prompt.IgnoreCount metrics, and replaces them with
equivalents over all permission actions (Accept, Deny, Dismiss, and
Ignore). The metrics are now defined strictly as the number of prompt
dismissals and ignores prior to (rather than inclusive of) the current
action for consistency with permission action reporting. Additional
histogram tests are added to verify that the metrics are recorded.
BUG=632269, 638076
Committed: https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608
Cr-Commit-Position: refs/heads/master@{#413827}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments #
Total comments: 9
Patch Set 3 : Comments #
Total comments: 2
Messages
Total messages: 31 (19 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dominickn@chromium.org changed reviewers: + isherman@chromium.org, raymes@chromium.org
PTAL, thanks!
Description was changed from ========== Add prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 ========== to ========== Add prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 ==========
This is starting to be quite a lot of metrics. Please try to set aside some time in the future to reevaluate whether these metrics all remain useful; and if not, clean up some of the ones which are not. https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:420: permission != PermissionType::NUM); nit: Please write this as three separate DCHECKs, so that check failures are clearer. https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:420: permission != PermissionType::NUM); Please preserve the comment "The user is not prompted for these permissions, thus there is no ignore nor dismiss recorded for them." https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:137: const char* prefix, nit: Probably best to pass a string or a StringPiece https://codereview.chromium.org/2250993002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40047: + Renamed to Permissions.Prompt.Dismissed.PriorDismissCount on 17 August 2016. Please move this text to an <obsolete> section. Ditto for any other renamed histograms.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! I've already filed crbug.com/638076 to track improvements to permissions metrics - including cleaning up callsites and evaluating which ones are and are not useful. https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:420: permission != PermissionType::NUM); On 2016/08/18 00:59:00, Ilya Sherman wrote: > nit: Please write this as three separate DCHECKs, so that check failures are > clearer. Done. https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2250993002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:137: const char* prefix, On 2016/08/18 00:59:00, Ilya Sherman wrote: > nit: Probably best to pass a string or a StringPiece Done. https://codereview.chromium.org/2250993002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40047: + Renamed to Permissions.Prompt.Dismissed.PriorDismissCount on 17 August 2016. On 2016/08/18 00:59:00, Ilya Sherman wrote: > Please move this text to an <obsolete> section. Ditto for any other renamed > histograms. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/18 01:21:12, dominickn wrote: > Thanks! I've already filed crbug.com/638076 to track improvements to permissions > metrics - including cleaning up callsites and evaluating which ones are and are > not useful. Sweet, thanks! LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm I chatted with Kendra and it seems like a good idea to have a way of tracking the experiments and their associated metrics. That way when the experiment is finished we can remove the metrics if they aren't needed. Maybe we can file separate bugs for each experiment or have a spreadsheet? https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:392: "Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i, 1); Should we add some tests for blocked/ignored? https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:223: "Permissions.Prompt.Ignored.PriorIgnoreCount."; Can we group together the strings related to this particular experiment? https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:55: static const char kPermissionsPromptIgnoredPriorIgnoreCountPrefix[]; If these aren't used externally, can we just put them in the anonymous namespace in the cc file? https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40014: </histogram> Since these have only been on canary for a very short time, can we just delete them? isherman@ will know the answer.
https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40014: </histogram> On 2016/08/22 02:55:09, raymes wrote: > Since these have only been on canary for a very short time, can we just delete > them? isherman@ will know the answer. It's generally preferable to mark as <obsolete> so that historical data remains. But, if the metrics have *only* been shipped to Canary, deleting might be ok as well. I'd still recommend marking as <obsolete> unless you have a particularly compelling reason to delete.
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...
Regarding tracking the metrics, we will be filing separate launch bugs per experiment, so those might be a good place to collect these metrics. https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:392: "Permissions.Prompt.Dismissed.PriorDismissCount.MidiSysEx", i, 1); On 2016/08/22 02:55:09, raymes wrote: > Should we add some tests for blocked/ignored? crrev.com/2258763002 refactors this testing a bit which will make this easier. I'll add those tests in a follow-up. https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:223: "Permissions.Prompt.Ignored.PriorIgnoreCount."; On 2016/08/22 02:55:09, raymes wrote: > Can we group together the strings related to this particular experiment? Done. https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2250993002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:55: static const char kPermissionsPromptIgnoredPriorIgnoreCountPrefix[]; On 2016/08/22 02:55:09, raymes wrote: > If these aren't used externally, can we just put them in the anonymous namespace > in the cc file? Arguably all the strings here that are like that should be in the anonymous namespace. I'd like to defer that to later. https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2250993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40014: </histogram> On 2016/08/22 20:42:33, Ilya Sherman wrote: > On 2016/08/22 02:55:09, raymes wrote: > > Since these have only been on canary for a very short time, can we just delete > > them? isherman@ will know the answer. > > It's generally preferable to mark as <obsolete> so that historical data remains. > But, if the metrics have *only* been shipped to Canary, deleting might be ok as > well. I'd still recommend marking as <obsolete> unless you have a particularly > compelling reason to delete. Acknowledged. Leaving as obsolete.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kcarattini@chromium.org changed reviewers: + kcarattini@chromium.org
https://codereview.chromium.org/2250993002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2250993002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:36: // Records that an ignore of a prompt for |permission| was made. You may want to clarify where this is recorded, and add a note that this should be called after any PermissionUmaUtil Record methods in order to get the prior metrics correct. I think the order is still wrong in permission_request_impl.cc (I'll send a followup cl).
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, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2250993002/#ps40001 (title: "Comments")
https://codereview.chromium.org/2250993002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2250993002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:36: // Records that an ignore of a prompt for |permission| was made. On 2016/08/23 04:28:46, kcarattini wrote: > You may want to clarify where this is recorded, and add a note that this should > be called after any PermissionUmaUtil Record methods in order to get the prior > metrics correct. I think the order is still wrong in permission_request_impl.cc > (I'll send a followup cl). Will clarify this in a follow up CL which splits out block ignoring from ShouldChangeDismissalToBlock.
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 prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 ========== to ========== Add prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 ========== to ========== Add prior dismissal and ignore count metrics for all permission actions. This CL removes the recently added Permission.Prompt.DismissCount and Permission.Prompt.IgnoreCount metrics, and replaces them with equivalents over all permission actions (Accept, Deny, Dismiss, and Ignore). The metrics are now defined strictly as the number of prompt dismissals and ignores prior to (rather than inclusive of) the current action for consistency with permission action reporting. Additional histogram tests are added to verify that the metrics are recorded. BUG=632269,638076 Committed: https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608 Cr-Commit-Position: refs/heads/master@{#413827} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6da2b3835157a2178a5951ff42f2e1a7d3404608 Cr-Commit-Position: refs/heads/master@{#413827} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
