Android: uses "import" section and inner classes for obtaining qualified JNI parameters.

Android: uses "import" section and inner classes for obtaining qualified JNI parameters.

Rather than explicitly listing all the parameters
(see crbug.com/158722), infer almost all of them from the import section and inner classes.
Assumes other classes are in the same package as a fallback.

BUG=159397
TEST=base/android/jni_generator/jni_generator_tests.py


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

[joth, 2012-11-05 14:08:14]

lgtm


On 5 November 2012 13:30, <bulach@chromium.org> wrote:

> Reviewers: John Knottenbelt, joth,
>
> Description:
> Android: uses "import" section for obtaining qualified JNI parameters.
>
> Rather than explicitly listing all the parameters
> (see crbug.com/158722), we can infer almost all of them from the import
> section.
> Exceptions being inner classes or others in the same package.
>
> BUG=159397
> TEST=base/android/jni_**generator/jni_generator_tests.**py
>
>
> Please review this at
http://codereview.chromium.**org/11363079/<http://codereview.chromium.org/113...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M base/android/jni_generator/**jni_generator.py
>   M base/android/jni_generator/**jni_generator_tests.py
>
>
> Index: base/android/jni_generator/**jni_generator.py
> diff --git a/base/android/jni_generator/**jni_generator.py
> b/base/android/jni_generator/**jni_generator.py
> index 20595dd8cbe1e9923f45b10e8159e1**53fc43fb3f..**
> 6ace02100a0f3d26a9baeec85d3937**5f967fac6a 100755
> --- a/base/android/jni_generator/**jni_generator.py
> +++ b/base/android/jni_generator/**jni_generator.py
> @@ -109,10 +109,18 @@ def JavaDataTypeToC(java_type):
>
>  class JniParams(object):
>    _UNKNOWN_JAVA_TYPE_PREFIX = 'UNKNOWN_JAVA_TYPE: '
> +  _imports = []
>    _external_param_files = set()
>    _external_param_list = []
>
>    @staticmethod
> +  def ExtractImports(contents):
> +    contents = contents.replace('\n', '')
> +    re_import = re.compile(r'import (?P<class>.*?);')
> +    for match in re.finditer(re_import, contents):
> +      JniParams._imports += ['L' + match.group('class').replace('**.',
> '/')]
> +
> +  @staticmethod
>    def ReadExternalParamList(**external_param_files):
>      if not external_param_files:
>        return
> @@ -180,7 +188,9 @@ class JniParams(object):
>      if '/' in param:
>        # Coming from javap, use the fully qualified param directly.
>        return 'L' + param + ';'
> -    for qualified_name in object_param_list + JniParams._external_param_*
> *list:
> +    for qualified_name in (object_param_list +
> +                           JniParams._imports +
> +                           JniParams._external_param_**list):
>        if (qualified_name.endswith('/' + param) or
>            qualified_name.endswith('$' + param.replace('.', '$')) or
>            qualified_name == 'L' + param):
> @@ -479,6 +489,7 @@ class JNIFromJavaSource(object):
>
>    def __init__(self, contents, fully_qualified_class):
>      contents = self._RemoveComments(contents)
> +    JniParams.ExtractImports(**contents)
>      jni_namespace = ExtractJNINamespace(contents)
>      natives = ExtractNatives(contents)
>      called_by_natives = ExtractCalledByNatives(**contents)
> Index: base/android/jni_generator/**jni_generator_tests.py
> diff --git a/base/android/jni_generator/**jni_generator_tests.py
> b/base/android/jni_generator/**jni_generator_tests.py
> index 3dd9097df816efa9c46485e05affa2**7cff5a995c..**
> 4537acb8954f4ba3c096008ebff5e3**dc320c34c0 100755
> --- a/base/android/jni_generator/**jni_generator_tests.py
> +++ b/base/android/jni_generator/**jni_generator_tests.py
> @@ -1537,5 +1537,40 @@ static bool RegisterNativesImpl(JNIEnv* env) {
>                        jni_generator.JniParams.**ReadExternalParamList,
>                        external_param_list)
>
> +  def testImports(self):
> +    import_header = """
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +package org.chromium.content.app;
> +
> +import android.app.Service;
> +import android.content.Context;
> +import android.content.Intent;
> +import android.graphics.**SurfaceTexture;
> +import android.os.Bundle;
> +import android.os.IBinder;
> +import android.os.**ParcelFileDescriptor;
> +import android.os.Process;
> +import android.os.RemoteException;
> +import android.util.Log;
> +import android.view.Surface;
> +
> +import java.util.ArrayList;
> +
> +import org.chromium.base.**CalledByNative;
> +import org.chromium.base.**JNINamespace;
> +import org.chromium.content.app.**ContentMain;
> +import org.chromium.content.browser.**SandboxedProcessConnection;
> +import org.chromium.content.common.**ISandboxedProcessCallback;
> +import org.chromium.content.common.**ISandboxedProcessService;
> +import org.chromium.content.common.**SurfaceCallback;
> +    """
> +    jni_generator.JniParams.**ExtractImports(import_header)
> +   
self.assertTrue('Lorg/**chromium/content/common/**ISandboxedProcessService'
> in
> +                    jni_generator.JniParams._**imports)
> +
> +
>  if __name__ == '__main__':
>    unittest.main()
>
>
>

[John Knottenbelt, 2012-11-05 14:26:31]

LGTM. Thanks, Marcus!

[bulach, 2012-11-05 19:32:56]

sorry about the churn! thanks to joth and jknotten's ideas, this is now _much_
simpler:
rather than having any hardcoded list of parameters (except POD and java/lang),
the jni generator now derives the types from:
1. "import" section
2. inner classes
3. fallback to use the same package.

it works fine for all ~90 files we're generating, but let me know if you think
there are any corner cases.

if you all think it's fine, I'll abandon the other patches with the
class_list.jni.

[Yaron, 2012-11-06 00:16:38]

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

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:123: def
ExtractReferences(contents):
"References" is too vague.. I had no idea what this did. Does
ExtractImportsAndInnerClasses offend you?

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:125: re_import = re.compile(r'import
(?P<class>.*?);')
It doesn't look like static imports are disallowed by the style guide and would
probably fail

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:127: JniParams._imports += ['L' +
match.group('class').replace('.', '/')]
Not necessarily for this change but there's a lot of "." to "/" and prepending
of 'L' in this code. It might be better to have simple types that support
conversion rather than using strings everywhere.

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:178: return prefix + 'L' +
JniParams._package + '/' + param + ';'
And a comment: 
# Type not found, falling back to same package as this class.

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:448:
JniParams.ExtractReferences(contents)
Nico's comment about making this non-static is becoming more relevant :)

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:963:
option_parser.add_option('--external_param_list',
Since you've removed this from jni_generator.gypi you can just remove the option
entirely.

http://codereview.chromium.org/11363079/diff/5001/build/jni_generator.gypi
File build/jni_generator.gypi (left):

http://codereview.chromium.org/11363079/diff/5001/build/jni_generator.gypi#ol...
build/jni_generator.gypi:32: 'jni_generator':
'<(DEPTH)/base/android/jni_generator/jni_generator.py',
chrome/chrome_browser.gypi references this variable already (i.e. and should be
removed)

[bulach, 2012-11-06 10:27:42]

thanks yaron! all comments addressed, ptal

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

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:123: def
ExtractReferences(contents):
On 2012/11/06 00:16:38, Yaron wrote:
> "References" is too vague.. I had no idea what this did. Does
> ExtractImportsAndInnerClasses offend you?

done (considering that an interface is a class..)

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:125: re_import = re.compile(r'import
(?P<class>.*?);')
On 2012/11/06 00:16:38, Yaron wrote:
> It doesn't look like static imports are disallowed by the style guide and
would
> probably fail

hmm.. this is necessary for extracting _types_ that will be used as JNI
parameters... afaict, static imports are just syntactic sugar for importing
members / constants, no? I mean, is it possible to "import static foo.bar;" and
then have either a @CalledByNative or a "native" using "bar" as param / return
type?

anyways... changed so that the regexp will match the last non-whitespace part
prior to ; and also added tests.

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:127: JniParams._imports += ['L' +
match.group('class').replace('.', '/')]
On 2012/11/06 00:16:38, Yaron wrote:
> Not necessarily for this change but there's a lot of "." to "/" and prepending
> of 'L' in this code. It might be better to have simple types that support
> conversion rather than using strings everywhere.

fully agree.. hopefully this JniParams class will eventually become a real class
(as opposed to currently just a namespace) and will deal with all these
conversions..

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:178: return prefix + 'L' +
JniParams._package + '/' + param + ';'
On 2012/11/06 00:16:38, Yaron wrote:
> And a comment: 
> # Type not found, falling back to same package as this class.

Done.

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:448:
JniParams.ExtractReferences(contents)
On 2012/11/06 00:16:38, Yaron wrote:
> Nico's comment about making this non-static is becoming more relevant :)

certainly! :) I'm planning a follow up soon once the implementation stabilizes a
bit..

http://codereview.chromium.org/11363079/diff/5001/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:963:
option_parser.add_option('--external_param_list',
On 2012/11/06 00:16:38, Yaron wrote:
> Since you've removed this from jni_generator.gypi you can just remove the
option
> entirely.

oh, yes, good point! the downstream tree uses via the gypi entry point, not this
one directly as I was thinking.. removed, thanks!

http://codereview.chromium.org/11363079/diff/5001/build/jni_generator.gypi
File build/jni_generator.gypi (left):

http://codereview.chromium.org/11363079/diff/5001/build/jni_generator.gypi#ol...
build/jni_generator.gypi:32: 'jni_generator':
'<(DEPTH)/base/android/jni_generator/jni_generator.py',
On 2012/11/06 00:16:38, Yaron wrote:
> chrome/chrome_browser.gypi references this variable already (i.e. and should
be
> removed)

Done.

[John Knottenbelt, 2012-11-06 12:08:07]

LGTM. You should update the description as the new code does deal with inner
classes etc :)

[bulach, 2012-11-06 12:25:25]

thanks john, good point! updated the title and description..

[Yaron, 2012-11-06 18:07:20]

lgtm

[commit-bot: I haz the power, 2012-11-07 09:38:07]

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

[commit-bot: I haz the power, 2012-11-07 09:38:11]

Presubmit check for 11363079-5 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[bulach, 2012-11-07 09:43:41]

+thakis for chrome/chrome_browser.gypi

[Nico, 2012-11-07 17:02:53]

lgtm

[commit-bot: I haz the power, 2012-11-07 17:03:05]

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

[commit-bot: I haz the power, 2012-11-07 17:03:28]

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

[commit-bot: I haz the power, 2012-11-07 19:06:53]

Change committed as 166482