Android: adds ptr_type to jni_generator.

Android: adds ptr_type to jni_generator.

Allows ptr_type to be configurable.

BUG=

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

[bulach, 2013-10-28 22:40:31]

ptal (a few more patches to come... :)

[rmcilroy, 2013-10-29 11:12:00]

lgtm

Couple of nits - feel free to ignore.

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

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator.py:534: natives =
ExtractNatives(contents, options)
nit - Unless you think other fields in the "options" object will be required
later, it might be cleaner just to pass ptr_type to this method.

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator.py:1079: help='The type of "p0" to be
cast to a native '
nit - the reference to "p0" is a bit obscure.  How about "The type used to
represent native pointers in Java code."?

[Andrew Hayden (chromium.org), 2013-10-29 11:49:21]

Minor comments. LGTM.

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

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator.py:1079: help='The type of "p0" to be
cast to a native '
On 2013/10/29 11:12:00, rmcilroy wrote:
> nit - the reference to "p0" is a bit obscure.  How about "The type used to
> represent native pointers in Java code."?

Or even more explicitly:
"The type used to represent native pointers in Java code. For 32-bit, use int;
for 64-bit, use long."

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator_tests.py:2240: static const
JNINativeMethod kMethodsTestJni[] = {
It looks like this is the only part that we care about for the test. The rest -
specifically things like the copyright statement and comments - could change
with the template, and we wouldn't care. Perhaps just search for this substring
in the result instead of matching the entire text?

[bulach, 2013-10-29 18:22:20]

thanks both! comments addressed and one question to andrew. please let me know
if you're ok with the suggested approach!

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

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator.py:534: natives =
ExtractNatives(contents, options)
On 2013/10/29 11:12:00, rmcilroy wrote:
> nit - Unless you think other fields in the "options" object will be required
> later, it might be cleaner just to pass ptr_type to this method.

good point, done.

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator.py:1079: help='The type of "p0" to be
cast to a native '
On 2013/10/29 11:49:21, Andrew Hayden wrote:
> On 2013/10/29 11:12:00, rmcilroy wrote:
> > nit - the reference to "p0" is a bit obscure.  How about "The type used to
> > represent native pointers in Java code."?
> 
> Or even more explicitly:
> "The type used to represent native pointers in Java code. For 32-bit, use int;
> for 64-bit, use long."

Done.

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
base/android/jni_generator/jni_generator_tests.py:2240: static const
JNINativeMethod kMethodsTestJni[] = {
On 2013/10/29 11:49:21, Andrew Hayden wrote:
> It looks like this is the only part that we care about for the test. The rest
-
> specifically things like the copyright statement and comments - could change
> with the template, and we wouldn't care. Perhaps just search for this
substring
> in the result instead of matching the entire text?

there are two aspects:
1. first, I agree that this is testing _more_ than necessary.. I should probably
tidy up the entire file, this same pattern is applied throughout the other
tests.

2. however :) this is not the only thing: 2226-2231 are also the target of this
test (i.e., the method takes a jlong and does a cast to the object...)...

if that's ok, I'd like to keep as is and then orthogonally cleanup all these
tests as a whole (without also changing the implementation)?

[rmcilroy, 2013-10-30 11:27:02]

On 2013/10/29 18:22:20, bulach wrote:
> thanks both! comments addressed and one question to andrew. please let me know
> if you're ok with the suggested approach!
> 
>
https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
> File base/android/jni_generator/jni_generator.py (right):
> 
>
https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
> base/android/jni_generator/jni_generator.py:534: natives =
> ExtractNatives(contents, options)
> On 2013/10/29 11:12:00, rmcilroy wrote:
> > nit - Unless you think other fields in the "options" object will be required
> > later, it might be cleaner just to pass ptr_type to this method.
> 
> good point, done.
> 
>
https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
> base/android/jni_generator/jni_generator.py:1079: help='The type of "p0" to be
> cast to a native '
> On 2013/10/29 11:49:21, Andrew Hayden wrote:
> > On 2013/10/29 11:12:00, rmcilroy wrote:
> > > nit - the reference to "p0" is a bit obscure.  How about "The type used to
> > > represent native pointers in Java code."?
> > 
> > Or even more explicitly:
> > "The type used to represent native pointers in Java code. For 32-bit, use
int;
> > for 64-bit, use long."
> 
> Done.
> 
>
https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
> File base/android/jni_generator/jni_generator_tests.py (right):
> 
>
https://codereview.chromium.org/49913003/diff/1/base/android/jni_generator/jn...
> base/android/jni_generator/jni_generator_tests.py:2240: static const
> JNINativeMethod kMethodsTestJni[] = {
> On 2013/10/29 11:49:21, Andrew Hayden wrote:
> > It looks like this is the only part that we care about for the test. The
rest
> -
> > specifically things like the copyright statement and comments - could change
> > with the template, and we wouldn't care. Perhaps just search for this
> substring
> > in the result instead of matching the entire text?
> 
> there are two aspects:
> 1. first, I agree that this is testing _more_ than necessary.. I should
probably
> tidy up the entire file, this same pattern is applied throughout the other
> tests.
> 
> 2. however :) this is not the only thing: 2226-2231 are also the target of
this
> test (i.e., the method takes a jlong and does a cast to the object...)...
> 
> if that's ok, I'd like to keep as is and then orthogonally cleanup all these
> tests as a whole (without also changing the implementation)?

Still lgtm from me.

[Andrew Hayden (chromium.org), 2013-10-30 11:39:51]

On 2013/10/29 18:22:20, bulach wrote:
> there are two aspects:
> 1. first, I agree that this is testing _more_ than necessary.. I should
probably
> tidy up the entire file, this same pattern is applied throughout the other
> tests.
> 
> 2. however :) this is not the only thing: 2226-2231 are also the target of
this
> test (i.e., the method takes a jlong and does a cast to the object...)...
> 
> if that's ok, I'd like to keep as is and then orthogonally cleanup all these
> tests as a whole (without also changing the implementation)?

SGTM.

[Andrew Hayden (chromium.org), 2013-10-30 11:40:44]

Still LGTM. Do it!

[commit-bot: I haz the power, 2013-11-04 10:50:10]

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

[commit-bot: I haz the power, 2013-11-04 17:41:18]

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

[commit-bot: I haz the power, 2013-11-04 20:26:32]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-04 20:55:08]

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

[commit-bot: I haz the power, 2013-11-05 00:54:25]

Change committed as 232858