Remove incognito data from the Geolocation.Header.* histograms.

Remove incognito data from the Geolocation.Header.* histograms.

Also, for Geolocation.Header.PermissionState separate out the unsuitable url and not-https cases in the histogram.

BUG=668923
Review-Url: https://codereview.chromium.org/2594443002
Cr-Commit-Position: refs/heads/master@{#442134}
Committed: https://chromium.googlesource.com/chromium/src/+/e21b84f9d52d63c3a5e4d89d8316534669c756af

[pdyson, 2016-12-20 00:42:03]

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

[commit-bot: I haz the power, 2016-12-20 00:42:43]

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

[pdyson, 2016-12-20 00:54:22]

pdyson@chromium.org changed reviewers:
+ dominickn@chromium.org

[benwells, 2016-12-20 00:54:49]

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

[benwells, 2016-12-20 00:54:50]

https://codereview.chromium.org/2594443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):

https://codereview.chromium.org/2594443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:159:
private static final int INCOGNITO_OR_UNSUITABLE_URL = 1;
Could we separate these out into two values? That is, INCOGNITO and
UNSUITABLE_URL? Seems like an easy change and we might see some interesting
data.

[pdyson, 2016-12-20 01:33:55]

On 2016/12/20 at 00:54:50, benwells wrote:
>
https://codereview.chromium.org/2594443002/diff/1/chrome/android/java/src/org...
> File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):
> 
>
https://codereview.chromium.org/2594443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:159:
private static final int INCOGNITO_OR_UNSUITABLE_URL = 1;
> Could we separate these out into two values? That is, INCOGNITO and
UNSUITABLE_URL? Seems like an easy change and we might see some interesting
data.

Ok. I've separated them out.

[benwells, 2016-12-20 01:49:02]

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

[commit-bot: I haz the power, 2016-12-20 01:49:26]

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

[pdyson, 2016-12-20 01:53:53]

pdyson@chromium.org changed reviewers:
+ isherman@chromium.org

[pdyson, 2016-12-20 01:54:25]

Description was changed from

==========
Separate out incognito and not-https cases for the
Geolocation.Header.PermissionState histogram.

BUG=668923
==========

to

==========
Separate out incognito, unsuitable url and not-https cases for the
Geolocation.Header.PermissionState histogram.

BUG=668923
==========

[commit-bot: I haz the power, 2016-12-20 03:02:50]

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

[commit-bot: I haz the power, 2016-12-20 03:02:51]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-12-20 04:08:00]

lgtm

[Ilya Sherman, 2016-12-20 04:29:17]

Hmm, I thought we typically do not measure incognito-specific behavior.  Could
you please check with the privacy team whether this is ok?

[benwells, 2016-12-20 04:33:35]

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

[benwells, 2016-12-20 04:33:35]

+msramek - is it ok to log this?

[msramek, 2016-12-22 02:37:58]

I agree with Ilya, I don't think we use incognito-specific metrics.

In this particular case, I could do the following:
1. Get a UMA protobuf containing Geolocation.Header.PermissionState.
2. Check the timestamp.
3. Check the Search.DefaultSearchProviderType
4. If #3 was a popular DSE, e.g. Google, I now know that Google was visited
approximately at the timestamp from #2.
5. => I have an incognito-specific browsing history entry.

Now, your histogram will necessarily leak the fact that the user has visited the
DSE, which I think is already borderline of what UMA can be used for. I think
that recognizing in addition that this visit happened in incognito would be past
that line.

[benwells, 2016-12-22 03:17:01]

On 2016/12/22 02:37:58, msramek (OOO until Jan 3) wrote:
> I agree with Ilya, I don't think we use incognito-specific metrics.
> 
> In this particular case, I could do the following:
> 1. Get a UMA protobuf containing Geolocation.Header.PermissionState.
> 2. Check the timestamp.
> 3. Check the Search.DefaultSearchProviderType
> 4. If #3 was a popular DSE, e.g. Google, I now know that Google was visited
> approximately at the timestamp from #2.
> 5. => I have an incognito-specific browsing history entry.
> 
> Now, your histogram will necessarily leak the fact that the user has visited
the
> DSE, which I think is already borderline of what UMA can be used for. I think
> that recognizing in addition that this visit happened in incognito would be
past
> that line.

[benwells, 2016-12-22 03:17:32]

On 2016/12/22 03:17:01, benwells wrote:
> On 2016/12/22 02:37:58, msramek (OOO until Jan 3) wrote:
> > I agree with Ilya, I don't think we use incognito-specific metrics.
> > 
> > In this particular case, I could do the following:
> > 1. Get a UMA protobuf containing Geolocation.Header.PermissionState.
> > 2. Check the timestamp.
> > 3. Check the Search.DefaultSearchProviderType
> > 4. If #3 was a popular DSE, e.g. Google, I now know that Google was visited
> > approximately at the timestamp from #2.
> > 5. => I have an incognito-specific browsing history entry.
> > 
> > Now, your histogram will necessarily leak the fact that the user has visited
> the
> > DSE, which I think is already borderline of what UMA can be used for. I
think
> > that recognizing in addition that this visit happened in incognito would be
> past
> > that line.

Great, thanks for the info. We'll just not record the new historgram at all when
in incognito - that will work for us.

[benwells, 2016-12-22 03:17:33]

On 2016/12/22 03:17:01, benwells wrote:
> On 2016/12/22 02:37:58, msramek (OOO until Jan 3) wrote:
> > I agree with Ilya, I don't think we use incognito-specific metrics.
> > 
> > In this particular case, I could do the following:
> > 1. Get a UMA protobuf containing Geolocation.Header.PermissionState.
> > 2. Check the timestamp.
> > 3. Check the Search.DefaultSearchProviderType
> > 4. If #3 was a popular DSE, e.g. Google, I now know that Google was visited
> > approximately at the timestamp from #2.
> > 5. => I have an incognito-specific browsing history entry.
> > 
> > Now, your histogram will necessarily leak the fact that the user has visited
> the
> > DSE, which I think is already borderline of what UMA can be used for. I
think
> > that recognizing in addition that this visit happened in incognito would be
> past
> > that line.

Great, thanks for the info. We'll just not record the new historgram at all when
in incognito - that will work for us.

[pdyson, 2017-01-03 03:57:01]

Description was changed from

==========
Separate out incognito, unsuitable url and not-https cases for the
Geolocation.Header.PermissionState histogram.

BUG=668923
==========

to

==========
Separate out incognito, unsuitable url and not-https cases for the
Geolocation.Header.PermissionState histogram. Do not update histogram
for incognito requests.

BUG=668923
==========

[pdyson, 2017-01-03 03:59:12]

I have now removed logging to this histogram for incognito requests.

[pdyson, 2017-01-03 03:59:31]

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

[commit-bot: I haz the power, 2017-01-03 03:59:43]

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

[benwells, 2017-01-03 04:08:44]

nice, lgtm!

[commit-bot: I haz the power, 2017-01-03 05:00:48]

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

[commit-bot: I haz the power, 2017-01-03 05:00:51]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-03 11:42:24]

Thanks for fixing! LGTM.

[pdyson, 2017-01-04 06:04:03]

Description was changed from

==========
Separate out incognito, unsuitable url and not-https cases for the
Geolocation.Header.PermissionState histogram. Do not update histogram
for incognito requests.

BUG=668923
==========

to

==========
Remove incognito data from the Geolocation.Header.* histograms.

Also, for Geolocation.Header.PermissionState separate out the unsuitable url and
not-https cases in the histogram.

BUG=668923
==========

[pdyson, 2017-01-04 06:08:01]

Dominick please take a look.

With the most recent patch today I've added in a !isIncognito test to remove the
incognito data from the other Geolocation.Header.* histograms. I had just added
those histograms in https://codereview.chromium.org/2576633002

[dominickn, 2017-01-04 06:14:15]

lgtm, thanks Paul!

[pdyson, 2017-01-04 06:22:31]

pdyson@chromium.org changed reviewers:
+ tedchoc@chromium.org

[pdyson, 2017-01-04 06:23:26]

Ted - Could you take a look at this? Thanks.

Dominick - thanks for the quick review.

[Ted C, 2017-01-04 19:18:31]

lgtm

[pdyson, 2017-01-04 22:22:58]

The CQ bit was checked by pdyson@chromium.org

[pdyson, 2017-01-04 22:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2594443002/#ps60001
(title: "Remove incognito stats from other Geolocation.Header.* histograms")

[commit-bot: I haz the power, 2017-01-04 22:23:22]

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-01-04 22:31:06]

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

[commit-bot: I haz the power, 2017-01-04 22:31:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pdyson, 2017-01-04 23:01:50]

Ilya, it looks like you are back at work tomorrow. Can you take a look at this
then? Thanks.

[Ilya Sherman, 2017-01-07 00:47:19]

histograms.xml lgtm

[Ilya Sherman, 2017-01-07 00:47:34]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2017-01-07 00:47:59]

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-01-07 01:49:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483750054033060,
"parent_rev": "62886b0305bfda888d83046d3972314313522be9", "commit_rev":
"e21b84f9d52d63c3a5e4d89d8316534669c756af"}

[commit-bot: I haz the power, 2017-01-07 01:50:22]

Description was changed from

==========
Remove incognito data from the Geolocation.Header.* histograms.

Also, for Geolocation.Header.PermissionState separate out the unsuitable url and
not-https cases in the histogram.

BUG=668923
==========

to

==========
Remove incognito data from the Geolocation.Header.* histograms.

Also, for Geolocation.Header.PermissionState separate out the unsuitable url and
not-https cases in the histogram.

BUG=668923

Review-Url: https://codereview.chromium.org/2594443002
Cr-Commit-Position: refs/heads/master@{#442134}
Committed:
https://chromium.googlesource.com/chromium/src/+/e21b84f9d52d63c3a5e4d89d8316...
==========

[commit-bot: I haz the power, 2017-01-07 01:50:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e21b84f9d52d63c3a5e4d89d8316...