Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by mcasas
Modified:
4 months, 4 weeks 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months ago (2017-04-19 18:51:04 UTC) #16
Reilly Grant (use Gerrit)
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 ...
5 months 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, ...
5 months 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 ...
5 months 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: ...
5 months 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 ...
5 months 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 ...
5 months 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 ...
5 months ago (2017-04-20 21:50:27 UTC) #25
aelias_OOO_until_Jul13
content/public/android lgtm
5 months ago (2017-04-20 22:29:48 UTC) #26
Reilly Grant (use Gerrit)
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()?
5 months 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: > ...
5 months 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
5 months 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
5 months 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. ...
5 months ago (2017-04-21 08:55:06 UTC) #35
sgurun-gerrit only
4 months, 4 weeks 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 b40b6558b