Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952003003:
Log site engagement scores for permission actions (Closed)
|
Created:
3 years, 6 months ago by Timothy Loh Modified:
3 years, 5 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog site engagement scores for permission actions
This patch adds histograms for tracking site engagement scores for
permission prompts, Permissions.Engagement.[Action].[Permission].
BUG=731349
Review-Url: https://codereview.chromium.org/2952003003
Cr-Commit-Position: refs/heads/master@{#488573}
Committed: https://chromium.googlesource.com/chromium/src/+/102eac87872ad8b399aeec59422e4f270826563f
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase and stuff #Patch Set 3 : add test #
Total comments: 6
Patch Set 4 : address comments #
Total comments: 12
Patch Set 5 : more comments #
Total comments: 2
Patch Set 6 : use histogram_functions #Messages
Total messages: 34 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Log site engagement scores for permission actions BUG=731349 ========== to ========== Log site engagement scores for permission actions This patch adds histograms for tracking site engagement scores for permission prompts, Permissions.Engagement.(Accepted).(Geolocation). BUG=731349 ==========
Description was changed from ========== Log site engagement scores for permission actions This patch adds histograms for tracking site engagement scores for permission prompts, Permissions.Engagement.(Accepted).(Geolocation). BUG=731349 ========== to ========== Log site engagement scores for permission actions This patch adds histograms for tracking site engagement scores for permission prompts, Permissions.Engagement.[Action].[Permission]. BUG=731349 ==========
timloh@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_request_manager.cc:222: DequeueRequestsAndShowBubble(); I think this results in us counting ignores for queued prompts that the user never sees if the PermissionService is cancelling the requests sequentially. https://codereview.chromium.org/2952003003/diff/2/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2952003003/diff/2/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:52315: + <owner>timloh@chromium.org</owner> Not sure who should be owners here, Dom and Kendra are owners of most other permission histograms so I went with them+me, but I can change it.
Would be nice to add a test if you can :) https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_request_manager.cc:131: PermissionUmaUtil::PermissionPromptIgnored(requests_, web_contents()); I think by this stage the requests may be empty. https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:112: action == "Ignored"); Are we interested in recording the total requests? Or should we just compute this from the sum of all these? https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:114: GetPermissionRequestString(type); Hmm, I think what we're really interested in is what the engagement is for each permission type? I wonder if we should just use the content settings type here? That would also save needing to have the function to convert the request type to a string. It would also make it easier to interpret the meaning of multiple prompts since they would be broken down by permission. https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:123: nit: no newline needed https://codereview.chromium.org/2952003003/diff/2/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2952003003/diff/2/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:52315: + <owner>timloh@chromium.org</owner> On 2017/06/22 08:02:20, Timothy Loh wrote: > Not sure who should be owners here, Dom and Kendra are owners of most other > permission histograms so I went with them+me, but I can change it. sgtm
Btw, sorry for the really long delay on this!
rietveld! https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_request_manager.cc:131: PermissionUmaUtil::PermissionPromptIgnored(requests_, web_contents()); On 2017/07/06 05:03:15, raymes wrote: > I think by this stage the requests may be empty. Moved to CleanUpRequests https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:112: action == "Ignored"); On 2017/07/06 05:03:15, raymes wrote: > Are we interested in recording the total requests? Or should we just compute > this from the sum of all these? I don't think we need to record the total requests. https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:114: GetPermissionRequestString(type); On 2017/07/06 05:03:15, raymes wrote: > Hmm, I think what we're really interested in is what the engagement is for each > permission type? I wonder if we should just use the content settings type here? > That would also save needing to have the function to convert the request type to > a string. It would also make it easier to interpret the meaning of multiple > prompts since they would be broken down by permission. At least as far as we only group mic+camera right now, it seems potentially more useful to have this separate? All the other Permissions.Prompt UMA logs based on PermissionRequestType too, they just don't have separate histograms for separate request types. https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:123: On 2017/07/06 05:03:15, raymes wrote: > nit: no newline needed Done.
The CQ bit was checked by timloh@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/2/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:114: GetPermissionRequestString(type); Ok - if you think so I'm ok. It just seemed simpler to use the ContentSettingsType here to me since we already measure a lot of stuff based on that. I'm still not fully convinced that recording multiple is more useful than individual permissions though, it seems potentially a bit harder to answer the question "what is the engagement for requests for microphone"? https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:227: } Since this should only be called with a single request_ right now I wonder if we can simplify this whole loop to just be: if (request_[0] == *request) FinalizeBubble(); (we would need to make FinalizeBubble handle the case when the view isn't there but I think that would be ok) The benefit then is that FinalizeBubble is always called when a permission request that displayed a bubble is finished (the bubble might be hiding because of tab switching, but conceptually it's still there). This means that we can pass an enum value to FinalizeBubble(): accepted,denied,ignored,dismissed from the various callsites and make the metric logic much clearer. We could also then remove CancelledRequest and friends. Even if we do eventually want this to work with mic/camera, I think it would be reasonable behavior to cancel both requests if either one of them was cancelled. WDYT? https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:299: CleanUpRequests(); nit: could we rename this to something that indicates that outstanding requests should be ignored. I'd just be worried that we would accidentally call this in another situation and then the metrics would be wrong https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager_unittest.cc (right): https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_unittest.cc:667: // UMAForMergedAcceptedBubble. nit: does this comment appled to the check you added above?
The CQ bit was checked by timloh@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...
https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:227: } On 2017/07/20 03:37:13, raymes wrote: > Since this should only be called with a single request_ right now I wonder if we > can simplify this whole loop to just be: > > if (request_[0] == *request) > FinalizeBubble(); > > (we would need to make FinalizeBubble handle the case when the view isn't there > but I think that would be ok) > > The benefit then is that FinalizeBubble is always called when a permission > request that displayed a bubble is finished (the bubble might be hiding because > of tab switching, but conceptually it's still there). This means that we can > pass an enum value to FinalizeBubble(): accepted,denied,ignored,dismissed from > the various callsites and make the metric logic much clearer. > > We could also then remove CancelledRequest and friends. > > Even if we do eventually want this to work with mic/camera, I think it would be > reasonable behavior to cancel both requests if either one of them was cancelled. > > WDYT? Done, but I left CancelledRequest in (if CanAcceptRequestUpdate returns false, we want to leave the prompt alone). https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:299: CleanUpRequests(); On 2017/07/20 03:37:13, raymes wrote: > nit: could we rename this to something that indicates that outstanding requests > should be ignored. I'd just be worried that we would accidentally call this in > another situation and then the metrics would be wrong Ack, will do this in a separate patch. I want to call it IgnorePendingRequests and update comments that say 'pending' to say 'queued' instead. https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager_unittest.cc (right): https://codereview.chromium.org/2952003003/diff/70001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_unittest.cc:667: // UMAForMergedAcceptedBubble. On 2017/07/20 03:37:13, raymes wrote: > nit: does this comment appled to the check you added above? hmm.. not entirely, I think the check is ok.
https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (left): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:423: DCHECK(view_); nit: I think it would be useful to hoist this DCHECK up into the callsites of this function where it still holds. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:198: // Grouped (mic+camera) requests are currently never cancelled. nit: these comments are probably in the reverse order https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:199: DCHECK_EQ(static_cast<size_t>(1), requests_.size()); nit: 1u? optional: may want to move this up above the if-statement (and check that it's 0 or 1) just so it's checked regardless of whether the condition is matched https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:67: std::string GetPermissionRequestString(PermissionRequestType type) { We will have to update this when someone adds a new request type :( Probably should remove the default: so people are reminded. I still think the benefits of using the CST outweigh using PRT here but if you really think this is better then we can land it now and revisit it if it ever becomes problematic. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:84: return "ProtectedMediaIdentifier"; Another downside to this is it's really easy to get these strings wrong :( This says ProtectedMedia in the enums.xml file. I've seen it happen in the past too. https://codereview.chromium.org/2952003003/diff/90001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2952003003/diff/90001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:53993: + dismissed. nit: move onto previous line?
https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (left): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:423: DCHECK(view_); On 2017/07/20 05:21:17, raymes wrote: > nit: I think it would be useful to hoist this DCHECK up into the callsites of > this function where it still holds. Done. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:198: // Grouped (mic+camera) requests are currently never cancelled. On 2017/07/20 05:21:17, raymes wrote: > nit: these comments are probably in the reverse order Done. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:199: DCHECK_EQ(static_cast<size_t>(1), requests_.size()); On 2017/07/20 05:21:17, raymes wrote: > nit: 1u? > > optional: may want to move this up above the if-statement (and check that it's 0 > or 1) just so it's checked regardless of whether the condition is matched Don't think 1u works on 64 bit platforms? But we can get into a state where we have mic+camera showing but we cancel a different request, so I can't move the check up. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:67: std::string GetPermissionRequestString(PermissionRequestType type) { On 2017/07/20 05:21:17, raymes wrote: > We will have to update this when someone adds a new request type :( Probably > should remove the default: so people are reminded. > > I still think the benefits of using the CST outweigh using PRT here but if you > really think this is better then we can land it now and revisit it if it ever > becomes problematic. Yeah it's not great but let's run with this for now. https://codereview.chromium.org/2952003003/diff/90001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:84: return "ProtectedMediaIdentifier"; On 2017/07/20 05:21:17, raymes wrote: > Another downside to this is it's really easy to get these strings wrong :( This > says ProtectedMedia in the enums.xml file. I've seen it happen in the past too. augh... fixed and removed the unused DurableStorage from histograms https://codereview.chromium.org/2952003003/diff/90001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2952003003/diff/90001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:53993: + dismissed. On 2017/07/20 05:21:17, raymes wrote: > nit: move onto previous line? git cl format does this, it's 81 characters :(
The CQ bit was checked by timloh@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, thanks!
timloh@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms.xml OWNERS, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2952003003/diff/110001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/110001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:123: base::LinearHistogram::FactoryGet( you should be able to switch tot he histogram_function version: https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.cc?sq=p...
https://codereview.chromium.org/2952003003/diff/110001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2952003003/diff/110001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:123: base::LinearHistogram::FactoryGet( On 2017/07/20 14:50:52, rkaplow wrote: > you should be able to switch tot he histogram_function version: > > https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.cc?sq=p... Thanks, done!
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2952003003/#ps130001 (title: "use histogram_functions")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1500605208749210,
"parent_rev": "fd8f995919fd2f2b87d966293c8fceec22c705df", "commit_rev":
"102eac87872ad8b399aeec59422e4f270826563f"}
Message was sent while issue was closed.
Description was changed from ========== Log site engagement scores for permission actions This patch adds histograms for tracking site engagement scores for permission prompts, Permissions.Engagement.[Action].[Permission]. BUG=731349 ========== to ========== Log site engagement scores for permission actions This patch adds histograms for tracking site engagement scores for permission prompts, Permissions.Engagement.[Action].[Permission]. BUG=731349 Review-Url: https://codereview.chromium.org/2952003003 Cr-Commit-Position: refs/heads/master@{#488573} Committed: https://chromium.googlesource.com/chromium/src/+/102eac87872ad8b399aeec59422e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:130001) as https://chromium.googlesource.com/chromium/src/+/102eac87872ad8b399aeec59422e... |
