Chromium Code Reviews
Help | Chromium Project | Sign in
(25)

Issue 2809813002: GeoLocation: add support for GmsCore location provider (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 4 days ago by mcasas
Modified:
2 days, 16 hours ago
CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Parameterized test and removed unused variable assignment #

Total comments: 14

Patch Set 3 : agrieve@s comments #

Total comments: 2

Patch Set 4 : Sorted out BUILD.gn usage of and a nit, and added forgotten (?) LocationRequest.setInterval() #

Total comments: 2

Patch Set 5 : reillyg@ nit #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+389 lines, -110 lines) Patch
M content/public/android/BUILD.gn View 1 2 1 chunk +0 lines, -1 line 0 comments Download
D content/public/android/javatests/src/org/chromium/content/browser/LocationProviderTest.java View 1 2 1 chunk +0 lines, -72 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/OWNERS View 1 1 chunk +0 lines, -3 lines 0 comments Download
M device/BUILD.gn View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
M device/geolocation/BUILD.gn View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAdapter.java View 1 2 3 chunks +10 lines, -5 lines 0 comments Download
M device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java View 1 2 3 4 8 chunks +23 lines, -21 lines 0 comments Download
M device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java View 1 2 3 chunks +9 lines, -6 lines 1 comment Download
A device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java View 1 2 3 1 chunk +136 lines, -0 lines 0 comments Download
M device/geolocation/android/javatests/src/org/chromium/device/geolocation/MockLocationProvider.java View 2 chunks +5 lines, -2 lines 0 comments Download
A device/geolocation/android/junit/src/org/chromium/device/geolocation/LocationProviderTest.java View 1 1 chunk +194 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (20 generated)
mcasas
agrieve@/jbudorick@ can you PTAL at how I'm using LocationProviderGmsCore.java in Chromium/Chrome builds? (See LocationProviderFactory.java as ...
2 weeks ago (2017-04-14 17:24:51 UTC) #9
jbudorick
I'm not entirely sure I understand the constraints. GMSCore is downloaded at gclient runhooks time ...
2 weeks ago (2017-04-14 20:57:03 UTC) #10
mcasas
reillyg@ PTAL (ignore PS1) timvolodine@ FYI/PTAL jbudorick@ thanks for the tip and the offline chat ...
1 week, 2 days ago (2017-04-19 18:51:04 UTC) #16
Reilly Grant
https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java (right): https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java#newcode42 device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java:42: public static void setLocationProviderImpl(LocationProviderFactory.LocationProvider provider) { I'm confused that ...
1 week, 2 days ago (2017-04-20 00:53:32 UTC) #17
mcasas
https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java (right): https://codereview.chromium.org/2809813002/diff/180001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderFactory.java#newcode42 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, ...
1 week, 2 days ago (2017-04-20 15:05:16 UTC) #18
agrieve
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#newcode318 device/BUILD.gn:318: google_play_services_library, nit: please use "google_play_services_package" rather than depending on ...
1 week, 2 days ago (2017-04-20 15:22:24 UTC) #19
mcasas
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#newcode318 device/BUILD.gn:318: google_play_services_library, On 2017/04/20 15:22:23, agrieve wrote: > nit: ...
1 week, 1 day ago (2017-04-20 18:59:32 UTC) #20
agrieve
for the BUILD.gn file: "$google_play_services_package:google_play_services_base_java" https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java (right): https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java#newcode95 device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java:95: "Failed to connect to ...
1 week, 1 day ago (2017-04-20 20:39:43 UTC) #21
agrieve
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/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java ...
1 week, 1 day ago (2017-04-20 20:39:58 UTC) #22
mcasas
aelias@ RS plz content/public/android removals/extractions (or PTAL, or please forward appropriately, thanks!) https://codereview.chromium.org/2809813002/diff/200001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderGmsCore.java ...
1 week, 1 day ago (2017-04-20 21:50:27 UTC) #25
aelias
content/public/android lgtm
1 week, 1 day ago (2017-04-20 22:29:48 UTC) #26
Reilly Grant
lgtm with question https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java (right): https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java#newcode54 device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java:54: assert ThreadUtils.runningOnUiThread(); ThreadUtils.assertOnUiThread()?
1 week, 1 day ago (2017-04-20 22:35:37 UTC) #27
mcasas
https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java File device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java (right): https://codereview.chromium.org/2809813002/diff/240001/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAndroid.java#newcode54 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: > ...
1 week, 1 day ago (2017-04-20 22:55:35 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2809813002/260001
1 week, 1 day ago (2017-04-21 01:28:00 UTC) #31
commit-bot: I haz the power
Committed patchset #5 (id:260001) as https://chromium.googlesource.com/chromium/src/+/638d9c4adea26e0f8a9d2e429546a44f5603b99c
1 week, 1 day ago (2017-04-21 03:01:11 UTC) #34
mattcary
A revert of this CL (patchset #5 id:260001) has been created in https://codereview.chromium.org/2832983003/ by mattcary@chromium.org. ...
1 week, 1 day ago (2017-04-21 08:55:06 UTC) #35
sgurun
2 days, 16 hours ago (2017-04-26 22:22:54 UTC) #37
Message was sent while issue was closed.
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46