Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791193002:
Log metrics for the Location Settings Dialog prompt for geolocation. (Closed)
|
Created:
3 years, 8 months ago by benwells Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, Michael van Ouwerkerk, mcasas+geolocation_chromium.org, raymes+watch_chromium.org, mlamouri+watch-geolocation_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog metrics for the Location Settings Dialog prompt for geolocation.
This prompt is shown when the user has turned off location and a web
site asks for geolocation access. There are various controls to prevent
spamminess (a gesture requirement for non-default search engine sites,
and a backoff system).
This change adds metrics for number of times the dialog was shown,
suppressed (for backoff), accepted or denied. Each metric has two
versions, one for the default search engine and one for other sites.
The value logged for each metric is the current back off level.
This change also cleans up some code for the backoff, using an enum
instead of an int to represent it. It also caches some calculations
done on whether an origin is the default search engine to prevent
it happening quite as much as it was.
BUG=703493
Review-Url: https://codereview.chromium.org/2791193002
Cr-Commit-Position: refs/heads/master@{#462446}
Committed: https://chromium.googlesource.com/chromium/src/+/dc48af3732f96f60c30396ed10b158314bc0b8c9
Patch Set 1 #Patch Set 2 : Added histograms #Patch Set 3 : Fix tests #
Total comments: 23
Patch Set 4 : Feedback #
Total comments: 2
Patch Set 5 : Undo histograms change #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by benwells@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 checked by benwells@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...
benwells@chromium.org changed reviewers: + isherman@chromium.org, raymes@chromium.org
isherman for histograms raymes for the rest
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:42: } Could this just be a function? I might be missing some reason why it needs to be a macro :) https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:95: "GeolocationSettingsDialog.ShowEvent.DSE"; nit: should this just be called LocationSettingsDialog for consistency with other places? https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:370: case LocationSettingsDialogBackOff::kOneWeek: I wonder if 0 should represent "kNoBackOff". In other words instead of representing what the backoff will be changed to, it instead represents what the backoff was last applied. It might make the metrics a bit easier to read. For example it would mean that you would read something like Shown 10, no backoff 1, 1 week backoff, 1, 1 month backoff 2, 3 month backoff For Suppress, "no backoff" would never appear since it should be impossible to get into that state. It's not a major thing, but just a thought :)
https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:33: #define LSD_UMA(dse_metric_name, non_dse_metric_name, is_dse, backoff) \ LSD? Sounds psychedelic! Could you please expand this acronym to something that's likely to be clearer for readers of the code? https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:34: if (is_dse) { \ nit: What is "dse"? https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:42: } On 2017/04/03 23:30:10, raymes (OOO until April 10) wrote: > Could this just be a function? I might be missing some reason why it needs to be > a macro :) UMA_HISTOGRAM_ENUMERATION requires runtime-constant names. However, UmaHistogramEnumeration() allows runtime-variable names, if you'd prefer to use a function. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:98: "GeolocationSettingsDialog.ShowEvent.NonDSE"; It looks like all of these names have a "DSE" or "NonDSE" suffix. Could that just be added programmatically, rather than doubling the number of string constants? https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:48: enum class LocationSettingsDialogBackOff { Please document that this enum is used to back an UMA histogram and should therefore be treated as append-only. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:52: kMax, nit: I'd suggest using "kCount" to mean "the number of items in the enum" and "kMax" as an alias for the last item in the enum. So, I'd use "kCount" here. https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21374: +<histogram name="GeolocationSettingsDialog.AcceptEvent" nit: Please add base="true" here and for the other histograms, since these names are just partial histogram names, used to construct full names via histogram_suffixes.
https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:95: "GeolocationSettingsDialog.ShowEvent.DSE"; On 2017/04/03 23:30:10, raymes (OOO until April 10) wrote: > nit: should this just be called LocationSettingsDialog for consistency with > other places? I deliberately made it start with Geo to be near the other strings alphabetically. I think it makes editing histograms, and finding things on the uma site easier. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:98: "GeolocationSettingsDialog.ShowEvent.NonDSE"; On 2017/04/04 00:04:40, Ilya Sherman wrote: > It looks like all of these names have a "DSE" or "NonDSE" suffix. Could that > just be added programmatically, rather than doubling the number of string > constants? Sure, together with using runtime variables in UmaHistogramEnumeration this will make the code a lot nicer. Do you mind if I hard code the metric strings in the corresponding unit test, instead of having functions or something to get the name with the suffix?
https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:98: "GeolocationSettingsDialog.ShowEvent.NonDSE"; On 2017/04/04 01:46:23, benwells wrote: > On 2017/04/04 00:04:40, Ilya Sherman wrote: > > It looks like all of these names have a "DSE" or "NonDSE" suffix. Could that > > just be added programmatically, rather than doubling the number of string > > constants? > > Sure, together with using runtime variables in UmaHistogramEnumeration this will > make the code a lot nicer. > > Do you mind if I hard code the metric strings in the corresponding unit test, > instead of having functions or something to get the name with the suffix? I don't mind -- I actually prefer simpler, more hardcoded tests. They're generally easier to read and understand.
https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21374: +<histogram name="GeolocationSettingsDialog.AcceptEvent" Sorry, just noticing this: Could you please name these histograms "Geolocation.SettingsDialog.*", so that they fit into the existing "Geolocation" category? Everything before the first dot in the histogram name essentially falls into a global namespace, so we want to limit top-level entries in that namespace to a reasonably small set.
The CQ bit was checked by benwells@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/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:33: #define LSD_UMA(dse_metric_name, non_dse_metric_name, is_dse, backoff) \ On 2017/04/04 00:04:40, Ilya Sherman wrote: > LSD? Sounds psychedelic! Could you please expand this acronym to something > that's likely to be clearer for readers of the code? Done. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:34: if (is_dse) { \ On 2017/04/04 00:04:40, Ilya Sherman wrote: > nit: What is "dse"? Ah ... Default Search Engine :) Changed and updated this particular string throughout. There is still DSE is some existing code and histograms, changing it everywhere would make this a bigger change and would also make some names really long. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:42: } On 2017/04/04 00:04:40, Ilya Sherman wrote: > On 2017/04/03 23:30:10, raymes (OOO until April 10) wrote: > > Could this just be a function? I might be missing some reason why it needs to > be > > a macro :) > > UMA_HISTOGRAM_ENUMERATION requires runtime-constant names. However, > UmaHistogramEnumeration() allows runtime-variable names, if you'd prefer to use > a function. UmaHistogramEnumeration taking runtime-variables - how awesome is that! Done, but I had to fix a tiny thing in UmaHistogramEnumeration to let it be used with enum classes. Let me know if you want that pulled into a separate CL, or if I'm just holding it wrong. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:98: "GeolocationSettingsDialog.ShowEvent.NonDSE"; On 2017/04/04 05:08:04, Ilya Sherman wrote: > On 2017/04/04 01:46:23, benwells wrote: > > On 2017/04/04 00:04:40, Ilya Sherman wrote: > > > It looks like all of these names have a "DSE" or "NonDSE" suffix. Could > that > > > just be added programmatically, rather than doubling the number of string > > > constants? > > > > Sure, together with using runtime variables in UmaHistogramEnumeration this > will > > make the code a lot nicer. > > > > Do you mind if I hard code the metric strings in the corresponding unit test, > > instead of having functions or something to get the name with the suffix? > > I don't mind -- I actually prefer simpler, more hardcoded tests. They're > generally easier to read and understand. Done. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:370: case LocationSettingsDialogBackOff::kOneWeek: On 2017/04/03 23:30:10, raymes wrote: > I wonder if 0 should represent "kNoBackOff". In other words instead of > representing what the backoff will be changed to, it instead represents what the > backoff was last applied. It might make the metrics a bit easier to read. For > example it would mean that you would read something like > Shown > 10, no backoff > 1, 1 week backoff, > 1, 1 month backoff > 2, 3 month backoff > > For Suppress, "no backoff" would never appear since it should be impossible to > get into that state. > > It's not a major thing, but just a thought :) Done. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.h (right): https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:48: enum class LocationSettingsDialogBackOff { On 2017/04/04 00:04:40, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2791193002/diff/40001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.h:52: kMax, On 2017/04/04 00:04:40, Ilya Sherman wrote: > nit: I'd suggest using "kCount" to mean "the number of items in the enum" and > "kMax" as an alias for the last item in the enum. So, I'd use "kCount" here. Done. https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21374: +<histogram name="GeolocationSettingsDialog.AcceptEvent" On 2017/04/04 00:04:41, Ilya Sherman wrote: > nit: Please add base="true" here and for the other histograms, since these names > are just partial histogram names, used to construct full names via > histogram_suffixes. Done. https://codereview.chromium.org/2791193002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21374: +<histogram name="GeolocationSettingsDialog.AcceptEvent" On 2017/04/04 00:04:41, Ilya Sherman wrote: > nit: Please add base="true" here and for the other histograms, since these names > are just partial histogram names, used to construct full names via > histogram_suffixes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM % the concern about the change to UmaHistogramEnumeration -- let's split that off to a separate CL. https://codereview.chromium.org/2791193002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2791193002/diff/60001/base/metrics/histogram_... base/metrics/histogram_functions.h:47: static_cast<int>(max)); I'd prefer that you make this change in a separate CL, and check in with dcheng@ about his plans, as he's been making similar improvements to the UMA_HISTOGRAM_ENUMERATION macro. I'm a bit worried that adding static_casts without any additional compile-time checks might result in some confusion and potentially lead to bugs. However, I really have no explanation for why the sample was being typecast whereas the max wasn't...
lgtm, thanks! https://codereview.chromium.org/2791193002/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_android.cc (right): https://codereview.chromium.org/2791193002/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_android.cc:341: // fall through nit: // Fall through. I'd tend to write the longhand here with repetition but I'll leave it up to you :)
The CQ bit was checked by benwells@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/2791193002/#ps80001 (title: "Undo histograms change")
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": 80001, "attempt_start_ts": 1491477399784420,
"parent_rev": "8a407067fd4c8d33aa36779f0738984b2d85186e", "commit_rev":
"dc48af3732f96f60c30396ed10b158314bc0b8c9"}
Message was sent while issue was closed.
Description was changed from ========== Log metrics for the Location Settings Dialog prompt for geolocation. This prompt is shown when the user has turned off location and a web site asks for geolocation access. There are various controls to prevent spamminess (a gesture requirement for non-default search engine sites, and a backoff system). This change adds metrics for number of times the dialog was shown, suppressed (for backoff), accepted or denied. Each metric has two versions, one for the default search engine and one for other sites. The value logged for each metric is the current back off level. This change also cleans up some code for the backoff, using an enum instead of an int to represent it. It also caches some calculations done on whether an origin is the default search engine to prevent it happening quite as much as it was. BUG=703493 ========== to ========== Log metrics for the Location Settings Dialog prompt for geolocation. This prompt is shown when the user has turned off location and a web site asks for geolocation access. There are various controls to prevent spamminess (a gesture requirement for non-default search engine sites, and a backoff system). This change adds metrics for number of times the dialog was shown, suppressed (for backoff), accepted or denied. Each metric has two versions, one for the default search engine and one for other sites. The value logged for each metric is the current back off level. This change also cleans up some code for the backoff, using an enum instead of an int to represent it. It also caches some calculations done on whether an origin is the default search engine to prevent it happening quite as much as it was. BUG=703493 Review-Url: https://codereview.chromium.org/2791193002 Cr-Commit-Position: refs/heads/master@{#462446} Committed: https://chromium.googlesource.com/chromium/src/+/dc48af3732f96f60c30396ed10b1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/dc48af3732f96f60c30396ed10b1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
