Upstream Android's PathUtils implementation.

Upstream Android's PathUtils implementation.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126624

[Peter Beverloo, 2012-02-23 13:50:27]

This change is dependent on Marcus' CL:
http://codereview.chromium.org/9384011/

It includes the first non-test Java files, providing the functions required to
build the bindings which, in turn, are required for path_utils.cc and
base_paths_android.cc.

This will solve one of the two remaining DumpRenderTree linking errors too. I'll
run trybots as soon as 9384011 lands, as it'd be pointless right now.

[joth, 2012-02-23 14:07:14]

some comments in drive-by.

Do we have any plan/idea when the .java will be in the continuous build? (just
concerned about bit-rot between now and then, if it's not going to be any time
soon)

https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/Pa...
File base/android/java/PathUtils.java (right):

https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/Pa...
base/android/java/PathUtils.java:24: return appContext.getDir("chrome",
Context.MODE_PRIVATE).getPath();
base/ should not know about "chrome"

at least, this should have a TODO

[Peter Beverloo, 2012-02-23 14:39:21]

I believe jrg is working on adding the capability to compile Java, I'll defer to
him for answering that question. The two Java files included in this patch only
refer each other, the Android SDK and Java APIs, so should be fairly isolated.

https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/Pa...
File base/android/java/PathUtils.java (right):

https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/Pa...
base/android/java/PathUtils.java:24: return appContext.getDir("chrome",
Context.MODE_PRIVATE).getPath();
On 2012/02/23 14:07:14, joth wrote:
> base/ should not know about "chrome"
> 
> at least, this should have a TODO

Added a TODO and filed an issue downstream, as it should be fixed there first.

[yfriedman, 2012-02-23 17:44:29]

Last I heard, Armand will be working on compiling java.

On Thu, Feb 23, 2012 at 6:39 AM, <peter@chromium.org> wrote:

> I believe jrg is working on adding the capability to compile Java, I'll
> defer to
> him for answering that question. The two Java files included in this patch
> only
> refer each other, the Android SDK and Java APIs, so should be fairly
> isolated.
>
>
> https://chromiumcodereview.**appspot.com/9443018/diff/2001/**
>
base/android/java/PathUtils.**java<https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/PathUtils.java>
> File base/android/java/PathUtils.**java (right):
>
> https://chromiumcodereview.**appspot.com/9443018/diff/2001/**
>
base/android/java/PathUtils.**java#newcode24<https://chromiumcodereview.appspot.com/9443018/diff/2001/base/android/java/PathUtils.java#newcode24>
> base/android/java/PathUtils.**java:24: return appContext.getDir("chrome",
> Context.MODE_PRIVATE).getPath(**);
> On 2012/02/23 14:07:14, joth wrote:
>
>> base/ should not know about "chrome"
>>
>
>  at least, this should have a TODO
>>
>
> Added a TODO and filed an issue downstream, as it should be fixed there
> first.
>
>
https://chromiumcodereview.**appspot.com/9443018/<https://chromiumcodereview....
>

[bulach, 2012-02-23 18:18:41]

not an owner, but lgtm with joth's comment and one nit below..

a few related notes fyi:
1. I just added a reference to android's style guide to our docs:
http://dev.chromium.org/developers/coding-style
http://dev.chromium.org/developers/android-style-guide

2. there are quite a few pieces required to actually compile and test java. it's
a chicken and egg problem :) as we'll need _some_ java code to be able to do
that, but obviously we don't want it to rotten.. I think this file is a good
candidate, as it's fairly small, self contained, and has a straightforward JNI
layer.. I've started to coordinate the efforts required, we'll update once we
have some concrete activities outlined...

thoughts?

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
File base/android/path_utils.cc (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
base/android/path_utils.cc:43: 
nit: extra \n

[Mark Mentovai, 2012-02-23 18:43:33]

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
File base/android/path_utils.cc (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
base/android/path_utils.cc:43: 
Remove the extraneous blank line at the end of this file.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp#newcod...
base/base.gyp:118: 'target_name': 'base_jni_headers',
If we need to do this in many locations, we may want to provide a template to
deal with it.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp#newcod...
base/base.gyp:133: '-o',
Is this the right way to invoke jni_generator.py?

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp#newcod...
base/base.gyp:134: '<@(_inputs)',
This will expand to all of the inputs, including jni_generator.py. Do you want
that?

[John Grabowski, 2012-02-26 08:25:58]

I don't think we should land java without the ability to build it.  It sets a
bad precedent for upstreaming.

It may be necessary to do so for staging, but if so we should map out a staging
plan.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
File base/android/java/PathUtils.java (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
base/android/java/PathUtils.java:5: package org.chromium.chromeview.base;
I don't think org.chromium.chromeview makes sense as a prefix.
"chromeview" seems wrong.
I think debating this issue, aka on chromium-dev, should be a precursor.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
base/android/java/PathUtils.java:24: // TODO(beverloo) base/ should not know
about "chrome": http://b/6057342
I think we should try a little harder to not violate layering on the 1st java
CL.

[klobag.chromium, 2012-02-27 21:38:39]

On 2012/02/26 08:25:58, John Grabowski wrote:
> I don't think we should land java without the ability to build it.  It sets a
> bad precedent for upstreaming.
> 

I believe the reason to land Java files is to generate the jni headers. So even
we don't build Java directly yet, we use the generated files to build the rest
of the cc files. So they are not "dead" code.

> It may be necessary to do so for staging, but if so we should map out a
staging
> plan.
> 
>
https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
> File base/android/java/PathUtils.java (right):
> 
>
https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
> base/android/java/PathUtils.java:5: package org.chromium.chromeview.base;
> I don't think org.chromium.chromeview makes sense as a prefix.
> "chromeview" seems wrong.
> I think debating this issue, aka on chromium-dev, should be a precursor.
> 
>
https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
> base/android/java/PathUtils.java:24: // TODO(beverloo) base/ should not know
> about "chrome": http://b/6057342
> I think we should try a little harder to not violate layering on the 1st java
> CL.

[klobag.chromium, 2012-02-27 21:38:59]

http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils....
File base/android/java/PathUtils.java (right):

http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils....
base/android/java/PathUtils.java:24: // TODO(beverloo) base/ should not know
about "chrome": http://b/6057342
I actually don't think this violates the layering. This just gives a name to the
directory. It is similar as TempFileName() in file_util_posix. If we don't want
to always call "chrome", we can call "chromium" for the non GOOGLE_CHROME_BUILD
case.

On 2012/02/26 08:25:58, John Grabowski wrote:
> I think we should try a little harder to not violate layering on the 1st java
> CL.

[joth, 2012-02-28 11:20:20]

On 27 February 2012 21:38, <klobag@chromium.org> wrote:

>
> http://codereview.chromium.**org/9443018/diff/5002/base/**
>
android/java/PathUtils.java<http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils.java>
> File base/android/java/PathUtils.**java (right):
>
> http://codereview.chromium.**org/9443018/diff/5002/base/**
>
android/java/PathUtils.java#**newcode24<http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils.java#newcode24>
>
> base/android/java/PathUtils.**java:24: // TODO(beverloo) base/ should not
> know about "chrome": http://b/6057342
> I actually don't think this violates the layering. This just gives a
> name to the directory. It is similar as TempFileName() in
> file_util_posix. If we don't want to always call "chrome", we can call
> "chromium" for the non GOOGLE_CHROME_BUILD case.
>
>
It violates layer in the sense that other binaries that use base (um...
hypothetical e.g.: forwarder!) it wouldn't make sense to have the name of
the browser inside their data path.

But given the name used is arbitrary (it's already inside the app's private
data dir and can't conflict with another version of the app
installed), "chromium" would actually work fine for everyone... In effect,
this would mean: "the sub dir that code from chromium.org writes its app
specific data files into".

But this will make us an upgrade headache for existing beta users :-/




>
> On 2012/02/26 08:25:58, John Grabowski wrote:
>
>> I think we should try a little harder to not violate layering on the
>>
> 1st java
>
>> CL.
>>
>
>
http://codereview.chromium.**org/9443018/<http://codereview.chromium.org/9443...
>

[klobag.chromium, 2012-02-28 17:26:59]

If we can use OFFICIAL_BUILD or GOOGLE_CHROME_BUILD to control, we can still
keep "chrome" for the "Chrome for Android" in the market.

On 2012/02/28 11:20:20, joth wrote:
> On 27 February 2012 21:38, <mailto:klobag@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/9443018/diff/5002/base/**
> >
>
android/java/PathUtils.java<http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils.java>
> > File base/android/java/PathUtils.**java (right):
> >
> > http://codereview.chromium.**org/9443018/diff/5002/base/**
> >
>
android/java/PathUtils.java#**newcode24<http://codereview.chromium.org/9443018/diff/5002/base/android/java/PathUtils.java#newcode24>
> >
> > base/android/java/PathUtils.**java:24: // TODO(beverloo) base/ should not
> > know about "chrome": http://b/6057342
> > I actually don't think this violates the layering. This just gives a
> > name to the directory. It is similar as TempFileName() in
> > file_util_posix. If we don't want to always call "chrome", we can call
> > "chromium" for the non GOOGLE_CHROME_BUILD case.
> >
> >
> It violates layer in the sense that other binaries that use base (um...
> hypothetical e.g.: forwarder!) it wouldn't make sense to have the name of
> the browser inside their data path.
> 
> But given the name used is arbitrary (it's already inside the app's private
> data dir and can't conflict with another version of the app
> installed), "chromium" would actually work fine for everyone... In effect,
> this would mean: "the sub dir that code from http://chromium.org writes its
app
> specific data files into".
> 
> But this will make us an upgrade headache for existing beta users :-/
> 
> 
> 
> 
> >
> > On 2012/02/26 08:25:58, John Grabowski wrote:
> >
> >> I think we should try a little harder to not violate layering on the
> >>
> > 1st java
> >
> >> CL.
> >>
> >
> >
>
http://codereview.chromium.**org/9443018/%3Chttp://codereview.chromium.org/94...>
> >

[Peter Beverloo, 2012-02-29 15:40:22]

Thank you for the reviews!

This amended patch changes Armand's stub Java file (r124134) with the actual
PathUtils and CalledByNative files, which will now be build as well. There are
two open items remaining:

1) Should the possible layering violation in PathUtils.java be addressed in this
patch? If so, any suggestions how?
2) Since the generate JNI-header action will be used more often, a template
could indeed be better. I'll work on this next.

Before landing, we'll probably want to apply the changes downstream first.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
File base/android/java/PathUtils.java (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/java/Pa...
base/android/java/PathUtils.java:5: package org.chromium.chromeview.base;
On 2012/02/26 08:25:58, John Grabowski wrote:
> I don't think org.chromium.chromeview makes sense as a prefix.
> "chromeview" seems wrong.
> I think debating this issue, aka on chromium-dev, should be a precursor.

Done in the thread. It's org.chromium.base now.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
File base/android/path_utils.cc (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/android/path_ut...
base/android/path_utils.cc:43: 
On 2012/02/23 18:43:33, Mark Mentovai wrote:
> Remove the extraneous blank line at the end of this file.

Done.

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/9443018/diff/5002/base/base.gyp#newcod...
base/base.gyp:133: '-o',
On 2012/02/23 18:43:33, Mark Mentovai wrote:
> Is this the right way to invoke jni_generator.py?

Yes (per a chat with bulach@). This is used to prevent rebuilding headers when
it's not necessary, and the argument is trimmed off the output files as expected
on line 961 of jni_generator.py.

[Yaron, 2012-02-29 16:55:14]

I don't think we can land this until at least:
1) We actually get Java to compile successfully on the fyi bot
2) Install ant on all of our builders

https://chromiumcodereview.appspot.com/9443018/diff/15001/build/android/envse...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/9443018/diff/15001/build/android/envse...
build/android/envsetup.sh:50: export ANDROID_SDK_VERSION="15"
I think we might need to define a few more environment variables here. The bot
isn't able to compile Java yet:
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...

[navabi, 2012-02-29 18:23:52]

https://chromiumcodereview.appspot.com/9443018/diff/15001/build/android/envse...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/9443018/diff/15001/build/android/envse...
build/android/envsetup.sh:50: export ANDROID_SDK_VERSION="15"
On 2012/02/29 16:55:14, Yaron wrote:
> I think we might need to define a few more environment variables here. The bot
> isn't able to compile Java yet:
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Andr...

It looks like we need to define at least JAVA_HOME

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...

[John Grabowski, 2012-03-13 19:06:57]

LGTM

Please rehit try server and be sure java builds

[Peter Beverloo, 2012-03-13 19:09:45]

On 2012/03/13 19:06:57, John Grabowski wrote:
> LGTM
> 
> Please rehit try server and be sure java builds

Thanks!

Mark, could you take a look as base/ OWNER? I'll address the template suggestion
downstream first. The try-bots should be running and I'll verify with
WebKit(DRT) as well prior to landing.

[Mark Mentovai, 2012-03-13 20:16:21]

https://chromiumcodereview.appspot.com/9443018/diff/23001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/9443018/diff/23001/base/base.gyp#newco...
base/base.gyp:134: '<@(_inputs)',
Do you really want to pass jni_generator.py back into jni_generator.py?

[Peter Beverloo, 2012-03-13 20:18:50]

https://chromiumcodereview.appspot.com/9443018/diff/23001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/9443018/diff/23001/base/base.gyp#newco...
base/base.gyp:134: '<@(_inputs)',
On 2012/03/13 20:16:21, Mark Mentovai wrote:
> Do you really want to pass jni_generator.py back into jni_generator.py?

As answered in PS3, per bulach@ this indeed is the way to invoke the generator.
I agree that it's odd, and will address this while looking at using a template
instead of bare actions.

[Mark Mentovai, 2012-03-13 20:22:28]

That‘s very weird and should be fixed separately.

LGTM then.

[commit-bot: I haz the power, 2012-03-14 09:42:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/9443018/23001

[commit-bot: I haz the power, 2012-03-14 11:24:05]

Change committed as 126624

[Nico, 2012-04-03 18:51:05]

http://codereview.chromium.org/9443018/diff/23001/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9443018/diff/23001/base/base.gyp#newcode122
base/base.gyp:122: 'action_name': 'generate_jni_headers',
This isn't done conditionally, so a `make all` creates jni headers even when not
building for android. It seems to be fairly fast so maybe that's ok, but it's a
bit surprising.