Updating JNIAdditionalImport to allow more than one import.

Updating JNIAdditionalImport to allow more than one import.

R=bulach@chromium.org

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

[bulach, 2014-05-22 16:40:00]

+ross

thanks benjamin! small suggestions below:

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:173: print 'Adding ' + new_import
nit: remove

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:199:
r'@JNIAdditionalImport\({(?P<class_names>.*?)}\)')
shouldn't this be sufficient, i.e., remove 193-197 above?

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:202:
JniParams.AddAdditionalImport(class_name.strip()[:-6])
hmm, the -6 seems out of place here.
how about just pass class_name.strip() here, then in 165, ensure that
class_name.endswith('.class') and then remove it there?

also, please add a test for the new case

[qsr, 2014-05-22 16:47:55]

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:173: print 'Adding ' + new_import
On 2014/05/22 16:40:00, bulach wrote:
> nit: remove

Done.

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:199:
r'@JNIAdditionalImport\({(?P<class_names>.*?)}\)')
On 2014/05/22 16:40:00, bulach wrote:
> shouldn't this be sufficient, i.e., remove 193-197 above?

 Not necessarly, you can use @JNIAdditionalImport(String.class) when
JNIAdditionalImport expects an array.

https://codereview.chromium.org/298973005/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:202:
JniParams.AddAdditionalImport(class_name.strip()[:-6])
On 2014/05/22 16:40:00, bulach wrote:
> hmm, the -6 seems out of place here.
> how about just pass class_name.strip() here, then in 165, ensure that
> class_name.endswith('.class') and then remove it there?
> 
> also, please add a test for the new case

Done.

[bulach, 2014-05-22 17:13:44]

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:200:
r'@JNIAdditionalImport\({(?P<class_names>.*?)}\)')
so how about collapsing here using optional braces, i.e.

r'@JNIAdditionalImport\({?(?P<class_names>.*?)}?\)'

would still return a string, and even if it's a single name, the split(',')
would work fine..

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/jni_generator_tests.py:1050:
@JNIAdditionalImport({Bar2.class, Bar3.class})
probably best to split into a separate file.. 
the reason is that the generated code in the sample file is actually wrong,
which makes it hard to follow it as an example of what it should be doing...
ditto for the overloaded methods: it mangles the functions in the generated
code... the mangling is already covered by testMethodNameMangling above, so we
can keep this test just generating a clearer example..

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/testJNIAdditionalImport.golden (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/testJNIAdditionalImport.golden:68: env, g_Foo_clazz,
as above, this g_Foo_clazz is wrong, but that's just an artifact of the test
data... if we split the test, the generated code will be correct. similarly for
the "V_OCFBC_OCFBC" mangled suffix..

[qsr, 2014-05-23 08:11:37]

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:200:
r'@JNIAdditionalImport\({(?P<class_names>.*?)}\)')
On 2014/05/22 17:13:45, bulach wrote:
> so how about collapsing here using optional braces, i.e.
> 
> mailto:r
> 
> would still return a string, and even if it's a single name, the split(',')
> would work fine..

Done.

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/jni_generator_tests.py:1050:
@JNIAdditionalImport({Bar2.class, Bar3.class})
On 2014/05/22 17:13:45, bulach wrote:
> probably best to split into a separate file.. 
> the reason is that the generated code in the sample file is actually wrong,
> which makes it hard to follow it as an example of what it should be doing...
> ditto for the overloaded methods: it mangles the functions in the generated
> code... the mangling is already covered by testMethodNameMangling above, so we
> can keep this test just generating a clearer example..

Done.

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
File base/android/jni_generator/testJNIAdditionalImport.golden (right):

https://codereview.chromium.org/298973005/diff/20001/base/android/jni_generat...
base/android/jni_generator/testJNIAdditionalImport.golden:68: env, g_Foo_clazz,
On 2014/05/22 17:13:45, bulach wrote:
> as above, this g_Foo_clazz is wrong, but that's just an artifact of the test
> data... if we split the test, the generated code will be correct. similarly
for
> the "V_OCFBC_OCFBC" mangled suffix..

Done.

https://codereview.chromium.org/298973005/diff/60001/base/android/jni_generat...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/298973005/diff/60001/base/android/jni_generat...
base/android/jni_generator/jni_generator_tests.py:152:
'org/chromium/example/jni_generator/SampleForTests')
This is necessary because this test was wrong -> the class was the one of
whichever test was run before, which is wrong. It put the class that was used in
the golden file.

[bulach, 2014-05-23 12:08:07]

lgtm, thanks!

[qsr, 2014-05-23 13:07:27]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-23 13:07:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/298973005/60001

[qsr, 2014-05-23 14:44:41]

Committed patchset #4 manually as r272524 (presubmit successful).