Change the method of getting Location Mode for Geolocation histograms.

Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString
with LOCATION_PROVIDER_ALLOWED to get the same information.

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

[pdyson, 2017-01-11 02:35:05]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not on.

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

[pdyson, 2017-01-11 02:35:44]

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

[commit-bot: I haz the power, 2017-01-11 02:36:15]

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

[pdyson, 2017-01-11 02:52:28]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not on.

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

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

[pdyson, 2017-01-11 02:52:28]

pdyson@chromium.org changed reviewers:
+ dominickn@chromium.org, lbargu@google.com

[commit-bot: I haz the power, 2017-01-11 03:39:52]

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

[commit-bot: I haz the power, 2017-01-11 03:39:53]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-11 04:24:33]

https://codereview.chromium.org/2621223002/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/2621223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:414:
assert false : "Error getting the LOCATION_MODE";
When would this setting not be found? Is it a crash-the-browser scenario?

Otherwise, I would just log the error and continue without asserting false or
printing the stack trace.

[lbargu, 2017-01-11 15:32:14]

lgtm

Actual changes look great. Thanks Paul for improving this.
Regarding the error case, I'll let Dominick review.

https://codereview.chromium.org/2621223002/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/2621223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:414:
assert false : "Error getting the LOCATION_MODE";
On 2017/01/11 04:24:33, dominickn wrote:
> When would this setting not be found? Is it a crash-the-browser scenario?
> 
> Otherwise, I would just log the error and continue without asserting false or
> printing the stack trace.

Right, best effort seems reasonable here :)
Maybe return OFF in this case, or not log an event at all.

[pdyson, 2017-01-12 02:38:52]

https://codereview.chromium.org/2621223002/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/2621223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:414:
assert false : "Error getting the LOCATION_MODE";
On 2017/01/11 at 15:32:13, lbargu wrote:
> On 2017/01/11 04:24:33, dominickn wrote:
> > When would this setting not be found? Is it a crash-the-browser scenario?
> > 
> > Otherwise, I would just log the error and continue without asserting false
or
> > printing the stack trace.
> 
> Right, best effort seems reasonable here :)
> Maybe return OFF in this case, or not log an event at all.

Ok. I've removed the assert and the stacktrace output. Keeping the return as
MASTER_OFF as
that will not log anything for the timing histograms, although it will still
appear in the
PermissionState histogram. That should be ok.

[pdyson, 2017-01-12 02:39:46]

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

[dominickn, 2017-01-12 03:13:12]

lgtm

[Ted C, 2017-01-12 20:07:37]

lgtm

Can you update the CL commit comment to describe why you need different values
on KK vs other platforms?

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

https://codereview.chromium.org/2621223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:424:

remove this blank line

[pdyson, 2017-01-12 23:49:20]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString with
LOCATION_PROVIDER_ALLOWED to get the same information.

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

[pdyson, 2017-01-12 23:54:02]

On 2017/01/12 at 20:07:37, tedchoc wrote:
> lgtm
> 
> Can you update the CL commit comment to describe why you need different values
on KK vs other platforms?
> 
>
https://codereview.chromium.org/2621223002/diff/20001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):
> 
>
https://codereview.chromium.org/2621223002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:424:

> remove this blank line

Updated the commit comment. Note that it is not different for only KitKat, but
different from KitKat onwards. The old way of getting the location mode was
deprecated and a new way introduced for KitKat onwards.

[pdyson, 2017-01-12 23:54:27]

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

[commit-bot: I haz the power, 2017-01-12 23:55:28]

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-01-13 00:00:12]

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

[commit-bot: I haz the power, 2017-01-13 00:00:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dominickn, 2017-01-13 00:07:06]

lgtm - please wrap the CL description to 80 characters, thanks!

[pdyson, 2017-01-13 00:59:52]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString with
LOCATION_PROVIDER_ALLOWED to get the same information.

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString with
LOCATION_PROVIDER_ALLOWED to get the same information.

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

[pdyson, 2017-01-13 01:01:34]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString with
LOCATION_PROVIDER_ALLOWED to get the same information.

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString
with LOCATION_PROVIDER_ALLOWED to get the same information.

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

[pdyson, 2017-01-13 01:01:55]

On 2017/01/13 at 00:07:06, dominickn wrote:
> lgtm - please wrap the CL description to 80 characters, thanks!

Done.

[pdyson, 2017-01-13 01:39:53]

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

[commit-bot: I haz the power, 2017-01-13 01:40:42]

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-01-13 01:44:07]

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

[commit-bot: I haz the power, 2017-01-13 01:44:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pdyson, 2017-01-13 02:45:48]

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

[commit-bot: I haz the power, 2017-01-13 02:46:24]

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

[pdyson, 2017-01-13 02:47:19]

A cl has replaced all uses of 'context' in this file with
ContextUtils.getApplicationContext(). So I've had to make the same change in
this cl.

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

https://codereview.chromium.org/2621223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:424:

On 2017/01/12 at 20:07:36, Ted C wrote:
> remove this blank line

Done.

[commit-bot: I haz the power, 2017-01-13 03:51:24]

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

[commit-bot: I haz the power, 2017-01-13 03:51:25]

Dry run: This issue passed the CQ dry run.

[pdyson, 2017-01-13 08:13:39]

The CQ bit was checked by pdyson@chromium.org

[pdyson, 2017-01-13 08:13:40]

The patchset sent to the CQ was uploaded after l-g-t-m from lbargu@google.com,
tedchoc@chromium.org, dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2621223002/#ps60001
(title: "Use ContextUtils.getApplicationContext() instead of 'context'")

[commit-bot: I haz the power, 2017-01-13 08:13:55]

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-13 08:18:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484295219899430,
"parent_rev": "6c6f4d8570cfbef74a7bb8659147658da2224b2d", "commit_rev":
"07023fe24b20ec9ac60ed4fc72ef569e19e38b87"}

[commit-bot: I haz the power, 2017-01-13 08:18:48]

Description was changed from

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString
with LOCATION_PROVIDER_ALLOWED to get the same information.

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

to

==========
Change the method of getting Location Mode for Geolocation histograms.

Changing to a method that works even when the app permission is not "allowed".

For KitKat and later using getInt with LOCATION_MODE to get the location mode.
Versions pre KitKat do not have this. So for those versions using getString
with LOCATION_PROVIDER_ALLOWED to get the same information.

BUG=668923

Review-Url: https://codereview.chromium.org/2621223002
Cr-Commit-Position: refs/heads/master@{#443517}
Committed:
https://chromium.googlesource.com/chromium/src/+/07023fe24b20ec9ac60ed4fc72ef...
==========

[commit-bot: I haz the power, 2017-01-13 08:18:52]

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