GeoLocation: add support for GmsCore location provider

GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

The tests content/public/.../LocationProviderTest.java are removed
and instead a new pumped up one is added in device/geolocation,
doing what the previous one was doing and parameterized to try
other location provider APIs (was: android only, now: mock,
android and GMS core).


BUG=706014
Review-Url: https://codereview.chromium.org/2809813002
Cr-Commit-Position: refs/heads/master@{#466239}
Committed: https://chromium.googlesource.com/chromium/src/+/638d9c4adea26e0f8a9d2e429546a44f5603b99c

[mcasas, 2017-04-13 18:52:15]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-04-13 18:52:22]

Description was changed from

==========
[wip] GeoLocation: add support for GmsCore location provider

Haz https://crbug.com/708373  local patch for enabling GMS location


BUG=706014
==========

to

==========
[wip] GeoLocation: add support for GmsCore location provider




BUG=706014
==========

[mcasas, 2017-04-13 21:40:28]

Patchset #2 (id:40001) has been deleted

[mcasas, 2017-04-14 02:27:52]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-04-14 16:40:22]

Description was changed from

==========
[wip] GeoLocation: add support for GmsCore location provider




BUG=706014
==========

to

==========
GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

GMS is not available in Chromium/Clankium builds, so 
LocationProviderFactory tests for the existence of 
"org.chromium.chrome.browser.ChromeActivity"; if not available
the Android location provider is used instead of the GmsCore.

For compilation under Clankium a stub GmsCore class is 
provided.


BUG=706014
==========

[mcasas, 2017-04-14 16:40:30]

Patchset #1 (id:60001) has been deleted

[mcasas, 2017-04-14 17:22:51]

Patchset #1 (id:80001) has been deleted

[mcasas, 2017-04-14 17:24:50]

mcasas@chromium.org changed reviewers:
+ agrieve@chromium.org, jbudorick@chromium.org

[mcasas, 2017-04-14 17:24:51]

agrieve@/jbudorick@ can you PTAL at how I'm using
LocationProviderGmsCore.java in Chromium/Chrome
builds?  (See LocationProviderFactory.java as well)

[jbudorick, 2017-04-14 20:57:03]

I'm not entirely sure I understand the constraints. GMSCore is downloaded at
gclient runhooks time for all android chromium checkouts
(https://codesearch.chromium.org/chromium/src/DEPS?rcl=14d5464e149236d40c40fe0...).

Does using //third_party/android_tools:google_play_services_location_java etc
not work?

[mcasas, 2017-04-19 05:10:50]

Patchset #2 (id:120001) has been deleted

[mcasas, 2017-04-19 05:11:19]

Patchset #2 (id:140001) has been deleted

[mcasas, 2017-04-19 18:43:58]

Patchset #2 (id:160001) has been deleted

[mcasas, 2017-04-19 18:46:54]

Description was changed from

==========
GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

GMS is not available in Chromium/Clankium builds, so 
LocationProviderFactory tests for the existence of 
"org.chromium.chrome.browser.ChromeActivity"; if not available
the Android location provider is used instead of the GmsCore.

For compilation under Clankium a stub GmsCore class is 
provided.


BUG=706014
==========

to

==========
GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

The tests content/public/.../LocationProviderTest.java are removed
and instead a new pumped up one is added in device/geolocation,
doing what the previous one was doing and parameterized to try
other location provider APIs (was: android only, now: mock,
android and GMS core).


BUG=706014
==========

[mcasas, 2017-04-19 18:51:03]

mcasas@chromium.org changed reviewers:
+ reillyg@chromium.org, timvolodine@chromium.org

[mcasas, 2017-04-19 18:51:04]

reillyg@ PTAL  (ignore PS1)


timvolodine@ FYI/PTAL



jbudorick@ thanks for the tip and the offline chat !

PS2 is changed to always compile and use LocationProviderGmsCore.java
(unless it's not created for some reason, case in which we fall back
to ToT's Android location provider).

Also I moved the .java test from content/public/android/... to device/
(and beefed it up) so we don't need to change content_test_shell_apk 
manifest after all.  Thanks!

Will add code-reviewers now, agrieve@ please shout if something 
looks funky.

[Reilly Grant (use Gerrit), 2017-04-20 00:53:32]

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:42:
public static void
setLocationProviderImpl(LocationProviderFactory.LocationProvider provider) {
I'm confused that we have both this and
LocationProviderAndroid.setLocationManagerForTesting().

[mcasas, 2017-04-20 15:05:16]

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:42:
public static void
setLocationProviderImpl(LocationProviderFactory.LocationProvider provider) {
On 2017/04/20 00:53:32, Reilly Grant wrote:
> I'm confused that we have both this and
> LocationProviderAndroid.setLocationManagerForTesting().

There's several levels of injection for testing.  The 
LocationProviderFactory can get a LocationProvider injected, which 
is this method.  

LocationProviderAndroid, which is one of the potential 
LocationProvider implementations, uses an internal 
android.location.LocationManager, which we also want to be able to
inject for test purposes. 

The other meaningful LocationProvider, LocationProviderGmsCore,
has a constructor where we inject the
com.google.android.gms.common.api.GoogleApiClient and
com.google.android.gms.location.FusedLocationProviderApi for
the purpose of this test.

[agrieve, 2017-04-20 15:22:24]

https://codereview.chromium.org/2809813002/diff/180001/device/BUILD.gn
File device/BUILD.gn (right):

https://codereview.chromium.org/2809813002/diff/180001/device/BUILD.gn#newcod...
device/BUILD.gn:318: google_play_services_library,
nit: please use "google_play_services_package" rather than depending on all gms
libraries here:
https://cs.chromium.org/chromium/src/build/config/android/config.gni?type=cs&...

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:50:
if (sProviderImpl == null) {
This will never be true since it's assigned on the previous line.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:36:
private boolean mEnablehighAccuracy = false;
nit: don't explicitly initialize fields to default values. It results in more
bytecode.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:73:
Log.e(TAG, "Caught IllegalStateException registering for location updates.");
When does this happen? Would be good to have a comment about it.

Also - it's not super helpful to swallow the stack trace, better would be:
Log.e(TAG, "Unexpected exception", e)

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:84:
Log.e(TAG, "onConnectionFailed " + result.toString());
Use ("onConnectionFailed %s", result). Although... Maybe it would make more
sense to Log from within newErrorAvailable rather than constructing two separate
messages?

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:119:
Log.i(TAG, "onLocationChanged: " + location.toString());
Might be PII here? Don't want to log user's location.

[mcasas, 2017-04-20 18:59:32]

ptal

https://codereview.chromium.org/2809813002/diff/180001/device/BUILD.gn
File device/BUILD.gn (right):

https://codereview.chromium.org/2809813002/diff/180001/device/BUILD.gn#newcod...
device/BUILD.gn:318: google_play_services_library,
On 2017/04/20 15:22:23, agrieve wrote:
> nit: please use "google_play_services_package" rather than depending on all
gms
> libraries here:
>
https://cs.chromium.org/chromium/src/build/config/android/config.gni?type=cs&...

That didn't work exactly for me, so instead I'm relying
directly on the appropriate 

"//third_party/android_tools:google_play_services_*_java"

please instruct :-)

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:50:
if (sProviderImpl == null) {
On 2017/04/20 15:22:23, agrieve wrote:
> This will never be true since it's assigned on the previous line.

Hmm yeah, that's not what I meant.  
Added a static method to check for GmsCore and used
it here to see which LocationProvider should be instantiated.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
(right):

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:36:
private boolean mEnablehighAccuracy = false;
On 2017/04/20 15:22:23, agrieve wrote:
> nit: don't explicitly initialize fields to default values. It results in more
> bytecode.

Ouch! Done.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:73:
Log.e(TAG, "Caught IllegalStateException registering for location updates.");
On 2017/04/20 15:22:23, agrieve wrote:
> When does this happen? Would be good to have a comment about it.
> 
> Also - it's not super helpful to swallow the stack trace, better would be:
> Log.e(TAG, "Unexpected exception", e)

Done.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:84:
Log.e(TAG, "onConnectionFailed " + result.toString());
On 2017/04/20 15:22:23, agrieve wrote:
> Use ("onConnectionFailed %s", result). Although... Maybe it would make more
> sense to Log from within newErrorAvailable rather than constructing two
separate
> messages?

Moved to newErrorAvailable() and logged there.

https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:119:
Log.i(TAG, "onLocationChanged: " + location.toString());
On 2017/04/20 15:22:23, agrieve wrote:
> Might be PII here? Don't want to log user's location.

Correct. This was for development only anyway. Removed.

[agrieve, 2017-04-20 20:39:43]

for the BUILD.gn file:
"$google_play_services_package:google_play_services_base_java"

https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
(right):

https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:95:
"Failed to connect to Google Play Services :" + result.toString());
nit: remove space before :

[agrieve, 2017-04-20 20:39:58]

On 2017/04/20 20:39:43, agrieve wrote:
> for the BUILD.gn file:
> "$google_play_services_package:google_play_services_base_java"
> 
>
https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
> File
>
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
> (right):
> 
>
https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
>
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:95:
> "Failed to connect to Google Play Services :" + result.toString());
> nit: remove space before :

plus, lgtm with that addressed :)

[mcasas, 2017-04-20 21:49:28]

Patchset #4 (id:220001) has been deleted

[mcasas, 2017-04-20 21:50:26]

mcasas@chromium.org changed reviewers:
+ aelias@chromium.org

[mcasas, 2017-04-20 21:50:27]

aelias@ RS plz content/public/android removals/extractions
  (or PTAL, or please forward appropriately, thanks!)

https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java
(right):

https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:95:
"Failed to connect to Google Play Services :" + result.toString());
On 2017/04/20 20:39:43, agrieve wrote:
> nit: remove space before :

Done.

[aelias_OOO_until_Jul13, 2017-04-20 22:29:48]

content/public/android lgtm

[Reilly Grant (use Gerrit), 2017-04-20 22:35:37]

lgtm with question

https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java
(right):

https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java:54:
assert ThreadUtils.runningOnUiThread();
ThreadUtils.assertOnUiThread()?

[mcasas, 2017-04-20 22:55:35]

https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java
(right):

https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java:54:
assert ThreadUtils.runningOnUiThread();
On 2017/04/20 22:35:37, Reilly Grant wrote:
> ThreadUtils.assertOnUiThread()?

Done.

[mcasas, 2017-04-21 01:27:34]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-04-21 01:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, aelias@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2809813002/#ps260001
(title: "reillyg@ nit")

[commit-bot: I haz the power, 2017-04-21 01:28:00]

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-21 03:00:57]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1492738054076140,
"parent_rev": "8623ad421d2a9c367b1dd9a98a2bd26e54f7ba65", "commit_rev":
"638d9c4adea26e0f8a9d2e429546a44f5603b99c"}

[commit-bot: I haz the power, 2017-04-21 03:01:10]

Description was changed from

==========
GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

The tests content/public/.../LocationProviderTest.java are removed
and instead a new pumped up one is added in device/geolocation,
doing what the previous one was doing and parameterized to try
other location provider APIs (was: android only, now: mock,
android and GMS core).


BUG=706014
==========

to

==========
GeoLocation: add support for GmsCore location provider

This CL adds LocationProviderGmsCore.java, an implementation
of LocationProviderFactory.LocationProvider interface using the
Google Play Services (GMS) provided location.

The tests content/public/.../LocationProviderTest.java are removed
and instead a new pumped up one is added in device/geolocation,
doing what the previous one was doing and parameterized to try
other location provider APIs (was: android only, now: mock,
android and GMS core).


BUG=706014

Review-Url: https://codereview.chromium.org/2809813002
Cr-Commit-Position: refs/heads/master@{#466239}
Committed:
https://chromium.googlesource.com/chromium/src/+/638d9c4adea26e0f8a9d2e429546...
==========

[commit-bot: I haz the power, 2017-04-21 03:01:11]

Committed patchset #5 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/638d9c4adea26e0f8a9d2e429546...

[mattcary, 2017-04-21 08:55:06]

A revert of this CL (patchset #5 id:260001) has been created in
https://codereview.chromium.org/2832983003/ by mattcary@chromium.org.

The reason for reverting is: crbug.com/714053

This CL apparently broke WebKit CTS geolocation tests..

[sgurun-gerrit only, 2017-04-26 22:22:53]

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

[sgurun-gerrit only, 2017-04-26 22:22:54]

https://codereview.chromium.org/2809813002/diff/260001/device/geolocation/and...
File
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java
(right):

https://codereview.chromium.org/2809813002/diff/260001/device/geolocation/and...
device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:50:
sProviderImpl = new LocationProviderGmsCore(context);
instead of doing this, can you set sProviderImpl from chrome? this way WebView
will continue to use LocationProviderAndroid