Log metrics for the new Default Search Engine geolocation setting.

Log metrics for the new Default Search Engine geolocation setting.

Recently a new system was introduced which added a preference for
whether the default search engine has geolocation access or not. This
change adds metrics for the state of this setting before the disclosure
UI is shown and after the disclosure UI is shown for the last time.

BUG=700777
Review-Url: https://codereview.chromium.org/2793253002
Cr-Commit-Position: refs/heads/master@{#463204}
Committed: https://chromium.googlesource.com/chromium/src/+/5754a56b61f227091f272bc84bfb212b9ba78416

[benwells, 2017-04-04 06:01:01]

The CQ bit was checked by benwells@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 06:01:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[benwells, 2017-04-04 06:57:56]

Description was changed from

==========
Log a metric for the new Default Search Engine geolocation setting.

Recently a new system was introduced which added a preference for
whether the default search engine has geolocation access or not. This
change adds metrics for the state of this setting before the disclosure
UI is shown and after the disclosure UI is shown for the last time.

BUG=700777
==========

to

==========
Log metrics for the new Default Search Engine geolocation setting.

Recently a new system was introduced which added a preference for
whether the default search engine has geolocation access or not. This
change adds metrics for the state of this setting before the disclosure
UI is shown and after the disclosure UI is shown for the last time.

BUG=700777
==========

[benwells, 2017-04-04 06:58:10]

benwells@chromium.org changed reviewers:
+ raymes@chromium.org

[commit-bot: I haz the power, 2017-04-04 06:59:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 06:59:19]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-04 07:00:04]

+isherman for histograms.xml

I realize now these should be Geolocation.Disclosure.*. There are preexisting
GeolocationDisclosure.* metrics... I think changing them all will be a bit of a
pain. Maybe we can just wait for these metrics to not be useful (probably a
couple more milestones) and make them all obsolete.

[raymes, 2017-04-04 22:36:39]

https://codereview.chromium.org/2793253002/diff/1/chrome/browser/android/sear...
File
chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc
(right):

https://codereview.chromium.org/2793253002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:239:
service->GetDSEGeolocationSetting());
Sorry my memory on this code isn't the best. Is it possible that if the user
never has their DSE setting set to true, that this will never get executed? In
other words is it possible that:
1) the DSE setting is initialized to false
2) We never show the disclosure
3) The shown count is never incremented, and the user never dismisses the
prompt,
4) We don't enter the codepath above to call RecordPostDisclosureMetrics

Not sure if this affects what you're trying to determine from these metrics
though? Is it hard to test these metrics?

[benwells, 2017-04-05 08:39:50]

https://codereview.chromium.org/2793253002/diff/1/chrome/browser/android/sear...
File
chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc
(right):

https://codereview.chromium.org/2793253002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.cc:239:
service->GetDSEGeolocationSetting());
On 2017/04/04 22:36:39, raymes wrote:
> Sorry my memory on this code isn't the best. Is it possible that if the user
> never has their DSE setting set to true, that this will never get executed? In
> other words is it possible that:
> 1) the DSE setting is initialized to false
> 2) We never show the disclosure
> 3) The shown count is never incremented, and the user never dismisses the
> prompt,
> 4) We don't enter the codepath above to call RecordPostDisclosureMetrics
> 
> Not sure if this affects what you're trying to determine from these metrics
> though? Is it hard to test these metrics?

Yes that's right, but not a problem. What we're trying to measure with these
metrics is what effect the disclosure has on the setting. The first metric is
pretty much redundant as this code path won't be entered unless the setting is
true, but I'm capturing it anyway for (a) consistency with the other metric and
(b) as a sanity check.

[benwells, 2017-04-06 08:43:14]

The CQ bit was checked by benwells@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 08:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 09:42:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 09:42:21]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-06 11:16:11]

Now with a test

[raymes, 2017-04-07 03:02:37]

lgtm, thanks for adding the test!

[benwells, 2017-04-07 03:03:41]

benwells@chromium.org changed reviewers:
+ dfalcantara@chromium.org, isherman@chromium.org

[benwells, 2017-04-07 03:03:41]

actually +isherman this time
+dfalcantara for java test

[Ilya Sherman, 2017-04-07 08:09:33]

I'm kind of curious how you plan to use these metrics in analysis.  You're going
to tend to see bias towards new users.  What types of user behavior are you
trying to understand?

For comparison, in Autofill, we found that recording the "Is Autofill enabled?"
setting once per page load was a much more helpful view than recording when the
setting changed.  This gave us a good sense of how many people had Autofill
enabled, weighted roughly in proportion to how often it would potentially offer
user value.  I wonder whether it might make sense to similarly record the state
of the geolocation setting whenever the user performs an action where the value
of the setting would matter.

Anywho, LGTM % nits if you are still convinced these metrics record the data
that you're interested in.

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21338: +<histogram
name="GeolocationDisclosure.PostDisclosureDSESetting" enum="Boolean">
nit: (Just within this file), please provide a custom enum with semantically
rich labels.  I'm not actually sure what "true" and "false" map to.  Is it
"Allow geolocation for DSE" vs. disallow?

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21370: +<histogram
name="GeolocationDisclosure.PreDisclosureDSESetting" enum="Boolean">
nit: (Ditto)

[benwells, 2017-04-07 08:32:01]

On 2017/04/07 08:09:33, Ilya Sherman wrote:
> I'm kind of curious how you plan to use these metrics in analysis.  You're
going
> to tend to see bias towards new users.  What types of user behavior are you
> trying to understand?
> 
> For comparison, in Autofill, we found that recording the "Is Autofill
enabled?"
> setting once per page load was a much more helpful view than recording when
the
> setting changed.  This gave us a good sense of how many people had Autofill
> enabled, weighted roughly in proportion to how often it would potentially
offer
> user value.  I wonder whether it might make sense to similarly record the
state
> of the geolocation setting whenever the user performs an action where the
value
> of the setting would matter.

Thanks for the review. For these metrics we are interested in the change in the
setting due to the user seeing the disclosure. So we record the state of new
users, and then we record the state after the disclosure had been shown for the
last time.

The disclosure is only shown a limited number of times, so this will definitely
be biased to new users, but that's fine.

Let me know if you're interested in more details.

> 
> Anywho, LGTM % nits if you are still convinced these metrics record the data
> that you're interested in.
> 
>
https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:21338: +<histogram
> name="GeolocationDisclosure.PostDisclosureDSESetting" enum="Boolean">
> nit: (Just within this file), please provide a custom enum with semantically
> rich labels.  I'm not actually sure what "true" and "false" map to.  Is it
> "Allow geolocation for DSE" vs. disallow?
> 
>
https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:21370: +<histogram
> name="GeolocationDisclosure.PreDisclosureDSESetting" enum="Boolean">
> nit: (Ditto)

[gone, 2017-04-07 17:10:34]

lgtm

[benwells, 2017-04-10 08:41:45]

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21338: +<histogram
name="GeolocationDisclosure.PostDisclosureDSESetting" enum="Boolean">
On 2017/04/07 08:09:33, Ilya Sherman wrote:
> nit: (Just within this file), please provide a custom enum with semantically
> rich labels.  I'm not actually sure what "true" and "false" map to.  Is it
> "Allow geolocation for DSE" vs. disallow?

Good point - it's 'allow' vs. 'disallow'. Done.

https://codereview.chromium.org/2793253002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21370: +<histogram
name="GeolocationDisclosure.PreDisclosureDSESetting" enum="Boolean">
On 2017/04/07 08:09:33, Ilya Sherman wrote:
> nit: (Ditto)

Done.

[benwells, 2017-04-10 08:41:53]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-04-10 08:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2793253002/#ps40001
(title: "Use BooleanAllowed")

[commit-bot: I haz the power, 2017-04-10 08:42:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:42:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491813713485840,
"parent_rev": "9bb7c0c57c6be13b19f47a289bafed94d55c3c45", "commit_rev":
"5754a56b61f227091f272bc84bfb212b9ba78416"}

[commit-bot: I haz the power, 2017-04-10 09:42:59]

Description was changed from

==========
Log metrics for the new Default Search Engine geolocation setting.

Recently a new system was introduced which added a preference for
whether the default search engine has geolocation access or not. This
change adds metrics for the state of this setting before the disclosure
UI is shown and after the disclosure UI is shown for the last time.

BUG=700777
==========

to

==========
Log metrics for the new Default Search Engine geolocation setting.

Recently a new system was introduced which added a preference for
whether the default search engine has geolocation access or not. This
change adds metrics for the state of this setting before the disclosure
UI is shown and after the disclosure UI is shown for the last time.

BUG=700777

Review-Url: https://codereview.chromium.org/2793253002
Cr-Commit-Position: refs/heads/master@{#463204}
Committed:
https://chromium.googlesource.com/chromium/src/+/5754a56b61f227091f272bc84bfb...
==========

[commit-bot: I haz the power, 2017-04-10 09:43:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5754a56b61f227091f272bc84bfb...