Android: passes a list of qualified JNI parameters as a param to the generator.

Android: passes a list of qualified JNI parameters as a param to the generator.

Instead of hardcoding the list of qualified JNI parameters,
pass it as a parameter so that different modules can inject their
classes.
This is the first step in such decoupling; follow up will start splitting
the class_list.jni list closer to their modules.

BUG=158722
TEST=jni_generator_tests

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

[bulach, 2012-10-29 14:21:04]

small step towards decoupling the trees, ptal

[Yaron, 2012-10-29 17:49:18]

lgtm

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:25: APP_PARAM_LIST = []
Nit: don't shorten. Call it APPLICATION_PARAM_LIST (or alternatively,
EXTERNAL/PROVIDED_*)

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:965: exec(contents) in tmp_globals,
tmp_locals
I'm not really keen on exec'ing external files. Can't we just have a list of
classes and append them to APP_PARAM_LIST? I don't feel too strongly but would
prefer otherwise.

http://codereview.chromium.org/11339013/diff/4002/chrome/browser/android/clas...
File chrome/browser/android/class_list.jni (right):

http://codereview.chromium.org/11339013/diff/4002/chrome/browser/android/clas...
chrome/browser/android/class_list.jni:24: # TODO(bulach): are these dependencies
ok?
For now, yes. All Java code in content is public until we decide to try and
separate them.

[bulach, 2012-10-29 18:57:06]

thanks yaron! all comments addressed, another quick look please?

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:25: APP_PARAM_LIST = []
On 2012/10/29 17:49:18, Yaron wrote:
> Nit: don't shorten. Call it APPLICATION_PARAM_LIST (or alternatively,
> EXTERNAL/PROVIDED_*)

Done.

http://codereview.chromium.org/11339013/diff/4002/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:965: exec(contents) in tmp_globals,
tmp_locals
On 2012/10/29 17:49:18, Yaron wrote:
> I'm not really keen on exec'ing external files. Can't we just have a list of
> classes and append them to APP_PARAM_LIST? I don't feel too strongly but would
> prefer otherwise.

sounds reasonable, stripping out comments is not too bad anyways.. :) done.

http://codereview.chromium.org/11339013/diff/4002/chrome/browser/android/clas...
File chrome/browser/android/class_list.jni (right):

http://codereview.chromium.org/11339013/diff/4002/chrome/browser/android/clas...
chrome/browser/android/class_list.jni:24: # TODO(bulach): are these dependencies
ok?
On 2012/10/29 17:49:18, Yaron wrote:
> For now, yes. All Java code in content is public until we decide to try and
> separate them.

thanks! added a comment about that. how about chromium/ui, is that a valid
dependency?

[Yaron, 2012-10-29 20:28:17]

lgtm

chrome/DEPS has +ui and there are ui includes all over chrome. I think it's
fine.

[bulach, 2012-10-30 12:51:42]

+thakis for OWNERS in chrome/chrome_browser.gypi


thanks yaron! I'm preparing the follow ups on the other trees, will send
shortly..

[Yaron, 2012-10-30 19:42:36]

We should also update the jni generator wiki page to mention the new override
format

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:511: msg += '\nPlease, edit %s' %
sys.argv[0]
Actually, with this change, this will no longer be correct. Can you include the
external_param_list file in the error message.

[Nico, 2012-10-30 19:47:35]

It'd be cool if the CL description mentioned what this will be used for?

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
File base/android/jni_generator/class_list.jni (right):

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/class_list.jni:9: # directories.
File tracking bug, link to it here.

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:170: for qualified_name in
object_param_list + EXTERNAL_PARAM_LIST:
CAPS_NAMES are usually constants, which this isn't really. Can't you just pass
this in as a parameter anyway?

[bulach, 2012-10-31 10:17:02]

thanks all! another look please?

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
File base/android/jni_generator/class_list.jni (right):

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/class_list.jni:9: # directories.
On 2012/10/30 19:47:36, Nico wrote:
> File tracking bug, link to it here.

Done.

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:170: for qualified_name in
object_param_list + EXTERNAL_PARAM_LIST:
On 2012/10/30 19:47:36, Nico wrote:
> CAPS_NAMES are usually constants, which this isn't really. Can't you just pass
> this in as a parameter anyway?

good points, thanks nico!

I refactored this a bit and created a class JniParams for all of these functions
so that this global has a narrower scope.

http://codereview.chromium.org/11339013/diff/7008/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:511: msg += '\nPlease, edit %s' %
sys.argv[0]
On 2012/10/30 19:42:36, Yaron wrote:
> Actually, with this change, this will no longer be correct. Can you include
the
> external_param_list file in the error message.

Done.

[Nico, 2012-10-31 17:41:28]

lgtm

http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
base/android/jni_generator/jni_generator.py:113: _external_param_list = []
Meh, that's still global state. Why not make this a real object that gets passed
to where the static methods get called?

[Yaron, 2012-10-31 17:43:54]

lgtm

[bulach, 2012-10-31 17:53:10]

thanks nico! quick question below:

http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
base/android/jni_generator/jni_generator.py:113: _external_param_list = []
On 2012/10/31 17:41:28, Nico wrote:
> Meh, that's still global state. Why not make this a real object that gets
passed
> to where the static methods get called?

I agree, but that's a much more complex change :)
ok to do separately? this patch as it is moves towards that anyways..

[Nico, 2012-10-31 18:15:51]

On Wed, Oct 31, 2012 at 10:53 AM,  <bulach@chromium.org> wrote:
> thanks nico! quick question below:
>
>
>
>
http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
> File base/android/jni_generator/jni_generator.py (right):
>
>
http://codereview.chromium.org/11339013/diff/20001/base/android/jni_generator...
> base/android/jni_generator/jni_generator.py:113: _external_param_list =
> []
> On 2012/10/31 17:41:28, Nico wrote:
>>
>> Meh, that's still global state. Why not make this a real object that
>
> gets passed
>>
>> to where the static methods get called?
>
>
> I agree, but that's a much more complex change :)
> ok to do separately?

Yes.

>
> http://codereview.chromium.org/11339013/

[commit-bot: I haz the power, 2012-10-31 18:25:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11339013/20001

[commit-bot: I haz the power, 2012-10-31 22:58:17]

Change committed as 165250