Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518963002:
Record metric of geo permission before and after showing the disclosure. (Closed)
DescriptionRecord metric of geo permission before and after showing the disclosure.
Recording this metric gives insight into how many people update their
settings after seeing the disclosure.
BUG=661011
Committed: https://crrev.com/528ee96d12423ce125a0b0ab0077fff06c26231b
Cr-Commit-Position: refs/heads/master@{#434953}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Feedback #
Total comments: 10
Patch Set 3 : Feedback 2 #
Messages
Total messages: 27 (12 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 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_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
benwells@chromium.org changed reviewers: + raymes@chromium.org
lgtm https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:123: // shown. nit: Perhaps // shown and won't be shown again. https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... File chrome/browser/android/search_geolocation_disclosure_tab_helper.h (right): https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.h:39: void RecordPreDisclosureMetrics(GURL gurl); nit: const GURL& gurl (may want to update the one above too if you like) https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.h:39: void RecordPreDisclosureMetrics(GURL gurl); nit: consider adding comments for these. In particular I missed the fact that it would only be recorded once per client. https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19008: + after the search geolocation disclosure has been shown. It could be useful to note this is only recorded once per client and also that it's recorded after the disclosure has been shown and won't be shown again. https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19016: + before the search geolocation disclosure has been shown. nit: similarly here
benwells@chromium.org changed reviewers: + dfalcantara@chromium.org, isherman@chromium.org
+dfalcantara for c/b/android +isherman for histograms https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:123: // shown. On 2016/11/22 01:54:00, raymes wrote: > nit: Perhaps // shown and won't be shown again. Done. https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... File chrome/browser/android/search_geolocation_disclosure_tab_helper.h (right): https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.h:39: void RecordPreDisclosureMetrics(GURL gurl); On 2016/11/22 01:54:00, raymes wrote: > nit: const GURL& gurl (may want to update the one above too if you like) Done. https://codereview.chromium.org/2518963002/diff/1/chrome/browser/android/sear... chrome/browser/android/search_geolocation_disclosure_tab_helper.h:39: void RecordPreDisclosureMetrics(GURL gurl); On 2016/11/22 01:54:00, raymes wrote: > nit: consider adding comments for these. In particular I missed the fact that it > would only be recorded once per client. Done. https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19008: + after the search geolocation disclosure has been shown. On 2016/11/22 01:54:00, raymes wrote: > It could be useful to note this is only recorded once per client and also that > it's recorded after the disclosure has been shown and won't be shown again. Done. https://codereview.chromium.org/2518963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19016: + before the search geolocation disclosure has been shown. On 2016/11/22 01:54:00, raymes wrote: > nit: similarly here Done.
https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:124: RecordPostDisclosureMetrics(gurl); Why does this happen up here instead of at the end of the function, where the shown count goes up?
https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:124: RecordPostDisclosureMetrics(gurl); On 2016/11/22 21:36:59, dfalcantara (check my queue) wrote: > Why does this happen up here instead of at the end of the function, where the > shown count goes up? I will update the comment to the below. Is it clear? Record metrics for the state of permissions after the disclosure has been shown. This is not done immediately after showing the last disclosure (i.e. at the end of this function), but on the next omnibox search, to allow the metric to capture changes to settings done by the user as a result of clicking on the Settings link in the disclosure.
lgtm % switching the comment text https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:124: RecordPostDisclosureMetrics(gurl); On 2016/11/22 21:42:34, benwells (slow) wrote: > On 2016/11/22 21:36:59, dfalcantara (check my queue) wrote: > > Why does this happen up here instead of at the end of the function, where the > > shown count goes up? > > I will update the comment to the below. Is it clear? > > Record metrics for the state of permissions after the disclosure has been shown. > This is not done immediately after showing the last disclosure (i.e. at the end > of this function), but on the next omnibox search, to allow the metric to > capture changes to settings done by the user as a result of clicking on the > Settings link in the disclosure. Ah yeah, that'd be good. Thanks!
https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:170: blink::mojom::PermissionStatus::LAST) + Near its definition, please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only (if it's not documented this way already). https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19004: +<histogram name="GeolocationDisclosure.PostDisclosurePermission"> Please add an enum attribute (and ditto below). https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19017: + before the search geolocation disclosure has been shown. This metric is only "before the search geolocation disclosure has been shown" gives a lot of wiggle room -- does this actually mean "immediately before"? If so, please add that word in. If not, could this be more specific?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right): https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:124: RecordPostDisclosureMetrics(gurl); On 2016/11/22 21:48:22, dfalcantara (check my queue) wrote: > On 2016/11/22 21:42:34, benwells (slow) wrote: > > On 2016/11/22 21:36:59, dfalcantara (check my queue) wrote: > > > Why does this happen up here instead of at the end of the function, where > the > > > shown count goes up? > > > > I will update the comment to the below. Is it clear? > > > > Record metrics for the state of permissions after the disclosure has been > shown. > > This is not done immediately after showing the last disclosure (i.e. at the > end > > of this function), but on the next omnibox search, to allow the metric to > > capture changes to settings done by the user as a result of clicking on the > > Settings link in the disclosure. > > Ah yeah, that'd be good. Thanks! Done. https://codereview.chromium.org/2518963002/diff/20001/chrome/browser/android/... chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:170: blink::mojom::PermissionStatus::LAST) + On 2016/11/22 22:36:16, Ilya Sherman wrote: > Near its definition, please document that this enum is used to back an UMA > histogram, and should therefore be treated as append-only (if it's not > documented this way already). Done. https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19004: +<histogram name="GeolocationDisclosure.PostDisclosurePermission"> On 2016/11/22 22:36:16, Ilya Sherman wrote: > Please add an enum attribute (and ditto below). Done. https://codereview.chromium.org/2518963002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19017: + before the search geolocation disclosure has been shown. This metric is only On 2016/11/22 22:36:17, Ilya Sherman wrote: > "before the search geolocation disclosure has been shown" gives a lot of wiggle > room -- does this actually mean "immediately before"? If so, please add that > word in. If not, could this be more specific? I made it immediately before, and updated the comment here.
benwells@chromium.org changed reviewers: + meacer@chromium.org
+meacer for new comment in the .mojom
mojom lgtm
On 2016/11/23 18:26:50, Mustafa Emre Acer wrote: > mojom lgtm ping isherman
metrics lgtm, thanks
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, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2518963002/#ps60001 (title: "Feedback 2")
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": 60001, "attempt_start_ts": 1480409338866080,
"parent_rev": "2f622ebcdc4e23c5a555abd95f4a12f6a368ed5b", "commit_rev":
"ed074582849662d5fbcc59276c096081310bb708"}
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Record metric of geo permission before and after showing the disclosure. Recording this metric gives insight into how many people update their settings after seeing the disclosure. BUG=661011 ========== to ========== Record metric of geo permission before and after showing the disclosure. Recording this metric gives insight into how many people update their settings after seeing the disclosure. BUG=661011 Committed: https://crrev.com/528ee96d12423ce125a0b0ab0077fff06c26231b Cr-Commit-Position: refs/heads/master@{#434953} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/528ee96d12423ce125a0b0ab0077fff06c26231b Cr-Commit-Position: refs/heads/master@{#434953} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
