Upstream Android Geolocation

Upstream Geolocation for Android.

TEST=content_unittests: GeolocationProvider.* LocationArbitrator.* 

There are also some integration tests downstream for the Java specific part.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=136019

[John Knottenbelt, 2012-04-24 17:02:13]

Hi Yaron, 

This isn't working yet -- jni headers not being generated yet. Can you take a
look and let me know if anything stands out as being obviously wrong?

Thanks

John

[Yaron, 2012-04-24 17:10:23]

I don't see anything obvious. I'd try to build content_unittests and also
re-gyp/touch the java file

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
File content/content_browser.gypi (right):

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
content/content_browser.gypi:820: }, { # OS!="android"
This is unnecessary as files with _android are omitted from other builds. See
build/filename_rules.gypi

[John Knottenbelt, 2012-04-25 15:25:37]

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
File content/content_browser.gypi (right):

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
content/content_browser.gypi:820: }, { # OS!="android"
Right you are -- although filename_rules.gypi won't cover
browser/geolocation/android_location_api_adapter.cc because it's not in android/
and it doesn't end in _android. 

Do you think that moving them to android/ makes most sense?

On 2012/04/24 17:10:23, Yaron wrote:
> This is unnecessary as files with _android are omitted from other builds. See
> build/filename_rules.gypi

[John Knottenbelt, 2012-04-25 17:14:08]

content/android/OWNERS copied from base/android/OWNERS

bulach, please review content/browser/geolocation

jrg, please can you take a look before I add content OWNERS and content/browser
OWNERS?

[jam, 2012-04-25 17:23:45]

see where the android files went per http://codereview.chromium.org/10035034/

[Yaron, 2012-04-25 17:26:09]

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
File content/content_browser.gypi (right):

https://chromiumcodereview.appspot.com/10209003/diff/1/content/content_browse...
content/content_browser.gypi:820: }, { # OS!="android"
On 2012/04/25 15:25:37, John Knottenbelt wrote:
> Right you are -- although filename_rules.gypi won't cover
> browser/geolocation/android_location_api_adapter.cc because it's not in
android/
> and it doesn't end in _android. 
> 
> Do you think that moving them to android/ makes most sense?
> 
> On 2012/04/24 17:10:23, Yaron wrote:
> > This is unnecessary as files with _android are omitted from other builds.
See
> > build/filename_rules.gypi
> 

I don't think it's necessary. Either rename it location_api_adapter_android or
add "android_*" to android-only builds.

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/android/OWNERS
File content/android/OWNERS (right):

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/android/OWN...
content/android/OWNERS:2: mark@chromium.org
I don't think this is the right set of owners. For one: It shouldn't be set
noparent. Two, you might seed with content/browser/OWNERS as this is very
different from base. Finally, it should include some Chrome on Android engineers
(e.g jrg, satish)

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/android/jav...
File content/android/java/content.xml (right):

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/android/jav...
content/android/java/content.xml:1: <project name="content" default="dist"
basedir=".">
Again, please rebase on 10035034 which added this in
content/public/android/java/content.xml

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/browser/and...
File content/browser/android/jni_helper.h (right):

https://chromiumcodereview.appspot.com/10209003/diff/5002/content/browser/and...
content/browser/android/jni_helper.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
See 10035034. The critical parts of this have been upstreamed. Do you need the
rest?

[John Knottenbelt, 2012-04-26 15:14:50]

http://codereview.chromium.org/10209003/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10209003/diff/1/content/content_browser.gypi#n...
content/content_browser.gypi:820: }, { # OS!="android"
Renamed to location_api_adapter_android

On 2012/04/25 17:26:09, Yaron wrote:
> On 2012/04/25 15:25:37, John Knottenbelt wrote:
> > Right you are -- although filename_rules.gypi won't cover
> > browser/geolocation/android_location_api_adapter.cc because it's not in
> android/
> > and it doesn't end in _android. 
> > 
> > Do you think that moving them to android/ makes most sense?
> > 
> > On 2012/04/24 17:10:23, Yaron wrote:
> > > This is unnecessary as files with _android are omitted from other builds.
> See
> > > build/filename_rules.gypi
> > 
> 
> I don't think it's necessary. Either rename it location_api_adapter_android or
> add "android_*" to android-only builds.

http://codereview.chromium.org/10209003/diff/5002/content/android/java/conten...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10209003/diff/5002/content/android/java/conten...
content/android/java/content.xml:1: <project name="content" default="dist"
basedir=".">
On 2012/04/25 17:26:09, Yaron wrote:
> Again, please rebase on 10035034 which added this in
> content/public/android/java/content.xml

These Java files are not part of the embedder API, but rather they are necessary
for providing Android-specific functionality to content. 

Don't we therefore need also a copy in content/android/java for the 'private'
portion of content/ ?

http://codereview.chromium.org/10209003/diff/5002/content/browser/android/jni...
File content/browser/android/jni_helper.h (right):

http://codereview.chromium.org/10209003/diff/5002/content/browser/android/jni...
content/browser/android/jni_helper.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
Nope, I'll get rid of this file.

On 2012/04/25 17:26:09, Yaron wrote:
> See 10035034. The critical parts of this have been upstreamed. Do you need the
> rest?

[John Knottenbelt, 2012-04-26 15:31:21]

On 2012/04/25 17:23:45, John Abd-El-Malek wrote:
> see where the android files went per http://codereview.chromium.org/10035034/

I had a look at that CL, but I don't think it's appropriate to add these Java
files in content/public/android/java because these java files are not providing
chrome-callable API. Rather, they are providing platform-specific implementation
(linking up with Android's geolocation system).

[John Knottenbelt, 2012-04-26 15:33:56]

Thanks for reviews guys. If content/android/java is not the correct place for
these files (I think it is), please can you suggest where they should go instead
-- as I said above I don't think content/public/android/java is the right place
since these java files shouldn't form part of the public API of content.

Thanks

John

[John Knottenbelt, 2012-04-26 15:35:33]

http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
File content/public/android/java/content.xml (left):

http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
content/public/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I renamed this to content_public, so that we don't trash the .jar with 'private'
content. Does this make sense?

[Yaron, 2012-04-26 19:03:54]

On 2012/04/26 15:35:33, John Knottenbelt wrote:
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> File content/public/android/java/content.xml (left):
> 
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> content/public/android/java/content.xml:1: <!-- Copyright (c) 2012 The
Chromium
> Authors. All rights reserved.
> I renamed this to content_public, so that we don't trash the .jar with
'private'
> content. Does this make sense?

I commented on the team thread that I don't like this approach. I actually think
that we shouldn't have content/public/android/java and should instead have
content/android/java and explicitly whitelist it so chrome can depend on it. The
reason I say this is because unlike C++, Java has language-features
(public/package-protected/etc) to support these restrictions within the
language. It doesn't seem like our build setup should require us to build two
different jars when the language handles it.

jam: what do you think?

[jam, 2012-04-26 19:29:40]

On 2012/04/26 19:03:54, Yaron wrote:
> On 2012/04/26 15:35:33, John Knottenbelt wrote:
> >
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> > File content/public/android/java/content.xml (left):
> > 
> >
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> > content/public/android/java/content.xml:1: <!-- Copyright (c) 2012 The
> Chromium
> > Authors. All rights reserved.
> > I renamed this to content_public, so that we don't trash the .jar with
> 'private'
> > content. Does this make sense?
> 
> I commented on the team thread that I don't like this approach. I actually
think
> that we shouldn't have content/public/android/java and should instead have
> content/android/java and explicitly whitelist it so chrome can depend on it.
The
> reason I say this is because unlike C++, Java has language-features
> (public/package-protected/etc) to support these restrictions within the
> language. It doesn't seem like our build setup should require us to build two
> different jars when the language handles it.
> 
> jam: what do you think?

it sounds like these jar files are needed for content implementation, so they
belong in content/browser/android like the other implementation files for
content

I feel strongly that chrome and other content embedders should only include
files from content/public. Using the argument above, one can say that we can add
internal content implementation functions in the content/public/ directories but
instead have them be under "private:" and friend other implementation classes in
chrome.

[klobag.chromium, 2012-04-27 00:45:58]

Even the classes are private to the package, the .jar needs to be included by
the embedder if we make a separate jar. So I suggest to keep all the Java
classes together and only export one jar as the static Java library. For each
Java class, use public/package/private to define whether it needs to be exposed
to the outside world.

On 2012/04/26 19:29:40, John Abd-El-Malek wrote:
> On 2012/04/26 19:03:54, Yaron wrote:
> > On 2012/04/26 15:35:33, John Knottenbelt wrote:
> > >
> >
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> > > File content/public/android/java/content.xml (left):
> > > 
> > >
> >
>
http://codereview.chromium.org/10209003/diff/9002/content/public/android/java...
> > > content/public/android/java/content.xml:1: <!-- Copyright (c) 2012 The
> > Chromium
> > > Authors. All rights reserved.
> > > I renamed this to content_public, so that we don't trash the .jar with
> > 'private'
> > > content. Does this make sense?
> > 
> > I commented on the team thread that I don't like this approach. I actually
> think
> > that we shouldn't have content/public/android/java and should instead have
> > content/android/java and explicitly whitelist it so chrome can depend on it.
> The
> > reason I say this is because unlike C++, Java has language-features
> > (public/package-protected/etc) to support these restrictions within the
> > language. It doesn't seem like our build setup should require us to build
two
> > different jars when the language handles it.
> > 
> > jam: what do you think?
> 
> it sounds like these jar files are needed for content implementation, so they
> belong in content/browser/android like the other implementation files for
> content
> 
> I feel strongly that chrome and other content embedders should only include
> files from content/public. Using the argument above, one can say that we can
add
> internal content implementation functions in the content/public/ directories
but
> instead have them be under "private:" and friend other implementation classes
in
> chrome.

[John Knottenbelt, 2012-04-27 13:00:00]

Thanks for your patience, I've move the java files into
content/public/android/java and ThreadUtils to base.

[Yaron, 2012-04-27 17:29:27]

gyp and java lgtm with the one change

http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gy...
content/content_browser.gypi:819: # A main loop would be handled in Java.
I don't think you want this comment or the first two exclusions.

[John Knottenbelt, 2012-04-30 10:51:23]

Thanks for review guys.

John A-M - please could you review the gyp and gypis for OWNERS approval? 

Bulach - could you please review content/browser/geolocation

Question - do we need a separate owners file for content/public/android?

http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gy...
content/content_browser.gypi:819: # A main loop would be handled in Java.
On 2012/04/27 17:29:27, Yaron wrote:
> I don't think you want this comment or the first two exclusions.

Done.

[jam, 2012-04-30 16:12:13]

On 2012/04/30 10:51:23, John Knottenbelt wrote:
> Thanks for review guys.
> 
> John A-M - please could you review the gyp and gypis for OWNERS approval? 

lgtm

> 
> Bulach - could you please review content/browser/geolocation
> 
> Question - do we need a separate owners file for content/public/android?

yes, definitely :)

> 
>
http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gypi
> File content/content_browser.gypi (right):
> 
>
http://codereview.chromium.org/10209003/diff/23001/content/content_browser.gy...
> content/content_browser.gypi:819: # A main loop would be handled in Java.
> On 2012/04/27 17:29:27, Yaron wrote:
> > I don't think you want this comment or the first two exclusions.
> 
> Done.

[John Knottenbelt, 2012-05-04 09:47:59]

As discussed with bulach, going to submit this TBR=bulach (for Geolocation).
There are no changes from downstream in the Geolocation part.

[commit-bot: I haz the power, 2012-05-04 09:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/10209003/31002

[commit-bot: I haz the power, 2012-05-04 09:59:03]

Try job failure for 10209003-31002 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-04 11:27:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/10209003/40002

[commit-bot: I haz the power, 2012-05-04 12:18:01]

Try job failure for 10209003-40002 (retry) on mac_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[John Knottenbelt, 2012-05-04 12:51:49]

@jam, could you please review my change to content/DEPS ? Should I move this
down into content/browser?

Thanks

John

[bulach, 2012-05-08 09:11:04]

lgtm

thanks john! sorry about the delay.. just some nits below, the first suggestion
below is probably best done downstream in a separate CL on its own.

http://codereview.chromium.org/10209003/diff/46002/base/android/java/org/chro...
File base/android/java/org/chromium/base/ThreadUtils.java (right):

http://codereview.chromium.org/10209003/diff/46002/base/android/java/org/chro...
base/android/java/org/chromium/base/ThreadUtils.java:62: public static <T> T
runOnUiThreadBlocking(Callable<T> c) throws ExecutionException {
nit: it may be better to move this overload up to 38, make the c param final if
possible, and make the one in 25 also throws ExecutionException..
perhaps it'd be nicer to rename this to "callOn" rather than "runOn" so that the
distinction between the Callable and the Runnable params..

http://codereview.chromium.org/10209003/diff/46002/base/android/java/org/chro...
base/android/java/org/chromium/base/ThreadUtils.java:129: * @return true iff the
current thread is the main (UI) thread.
nit: a reviewer (brettw?) once told me to not use "iff"..

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_api_adapter_android.cc (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_api_adapter_android.cc:29: has_heading,
heading, has_speed, speed);
nit: indent+2 here and above

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_api_adapter_android.h (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_api_adapter_android.h:20: class
LocationProviderAndroid;
nit: add a \n

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_provider_android.h (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_provider_android.h:24: // LocationProvider.
nit: LocationProviderBase

[John Knottenbelt, 2012-05-08 13:43:16]

Will do the ThreadUtils.java changes downstream first, as suggested.

@jam, DEPS change moved to content/browser/DEPS to be more specific. I'm going
to try and put this through the commit queue now as I think you're OK with this,
but if not, I will be happy to change it.

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_api_adapter_android.cc (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_api_adapter_android.cc:29: has_heading,
heading, has_speed, speed);
On 2012/05/08 09:11:05, bulach wrote:
> nit: indent+2 here and above

Done.

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_api_adapter_android.h (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_api_adapter_android.h:20: class
LocationProviderAndroid;
On 2012/05/08 09:11:05, bulach wrote:
> nit: add a \n

Done.

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
File content/browser/geolocation/location_provider_android.h (right):

http://codereview.chromium.org/10209003/diff/46002/content/browser/geolocatio...
content/browser/geolocation/location_provider_android.h:24: // LocationProvider.
On 2012/05/08 09:11:05, bulach wrote:
> nit: LocationProviderBase

Done.

[commit-bot: I haz the power, 2012-05-08 13:43:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/10209003/40003

[commit-bot: I haz the power, 2012-05-08 16:35:34]

Try job failure for 10209003-40003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-08 17:15:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/10209003/40003

[commit-bot: I haz the power, 2012-05-08 19:39:12]

Try job failure for 10209003-40003 (retry) on win_rel for step "update".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-09 09:12:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/10209003/40003

[commit-bot: I haz the power, 2012-05-09 09:22:32]

Try job failure for 10209003-40003 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.