Android JNI: Generate calls to RegisterNatives() [chromium/src : master]
Yipeng Wang (Gerrit)
Yipeng Wang uploaded patch set #11 to this change.
Android JNI: Generate calls to RegisterNatives()
Generate registration functions with unique names(package+class). Create a new template to
generate a header file which calls all registration functions together.
Design doc: https://docs.google.com/a/google.com/document/d/1GjdDAomu9AHFFAyGxRD8_cScsROP7fOwpMnHhnsiAGc/edit?usp=sharing
Bug: 683256
Change-Id: I31cbfe393c9088233f65f1249285d48ac5571f45
---
M android_webview/BUILD.gn
M base/android/jni_generator/SampleForTests_jni.golden
M base/android/jni_generator/jni_generator.py
M base/android/jni_generator/jni_generator_helper.h
A base/android/jni_generator/jni_registration_generator.py
M base/android/jni_generator/testInnerClassNatives.golden
M base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
M base/android/jni_generator/testInnerClassNativesMultiple.golden
M base/android/jni_generator/testMainDexFile.golden
M base/android/jni_generator/testMultipleJNIAdditionalImport.golden
M base/android/jni_generator/testNatives.golden
M base/android/jni_generator/testNativesLong.golden
M base/android/jni_generator/testNonMainDexFile.golden
M base/android/jni_generator/testSingleJNIAdditionalImport.golden
M build/android/gradle/generate_gradle.py
M build/android/gyp/write_build_config.py
M build/config/android/rules.gni
M chrome/BUILD.gn
M chrome/android/BUILD.gn
M chrome/browser/android/DEPS
M chrome/browser/android/chrome_entry_point.cc
21 files changed, 392 insertions(+), 6 deletions(-)
To view, visit change 527683. To unsubscribe, visit settings.
Andrew Grieve (Gerrit)
Andrew Grieve posted comments on this change.
Patch set 11:
Here are the rest.
Eric - maybe hold off on a thorough review, but I'd be curious if you had thoughts how to add tests?
(12 comments)
File base/android/jni_generator/jni_registration_generator.py:
This is the same string transformation that is used above. You should create a helper method for it.
Patch Set #8, Line 165:
AddDepfileOptionI don't see you writing a depfile anywhere, but you should be (use build_utils.WriteDepFile), and include all .java files in it.
nit: looke like this will fit on the previous line. (and a couple others below)
Patch Set #8, Line 175:
ptr_typeUnused. Remove. And seems the default should be 'long'
File build/config/android/rules.gni:
Patch Set #8, Line 337:
, and it is only used inI'd remove this note about where it's used.
Patch Set #8, Line 347:
sourcesnit: this is stale. The variable is called "target", and "output"
Patch Set #8, Line 355:
base_output_dirprefix local variables with _
Patch Set #8, Line 365:
_exception_java_filesDon't hardcode these in the template, but rather have them passed in as a variable.
Patch Set #8, Line 370:
rebase_exception_java_fileslocal variables should start with _ to follow conventions. I'd just inline this below though.
Patch Set #8, Line 396:
include_dirs = [ base_output_dir ]I don't think there's value in adding an include dir for this. We should just have the .cc file include via:
#include "chrome/android/chrome_jni_registration.h"The root_gen_dir is already in the default include_dirs.
Without the config, you can also drop the need for a group() wrapper, and just have action(target_name) above.
Patch Set #8, Line 694:
_toolchain)nit: making this "${target_gen_dir}/${target_name}.h" might make it a bit nicer.
File chrome/browser/android/chrome_entry_point.cc:
Patch Set #8, Line 38:
if (base::android::IsSelectiveJniRegistrationEnabled(env)) {Add TODO(crbug/XXXX) to delete this part (and comment that it's a no-op now)
To view, visit change 527683. To unsubscribe, visit settings.
Eric Stevenson (Gerrit)
Eric Stevenson posted comments on this change.
Patch set 11:
Patch Set 11:
(12 comments)
Here are the rest.
Eric - maybe hold off on a thorough review, but I'd be curious if you had thoughts how to add tests?
Sounds good.
I think jni_generator_tests.py already covers all of the functions jni_registration_generator.py uses from jni_generator.
Probably sufficient to have GenerateJNIHeader contain only the code to up to:
header_content = CreateFromDict(dict)
and then just add a bunch of tests with goldens for that method?
(1 comment)
File base/android/jni_generator/jni_registration_generator.py:
Patch Set #8, Line 143:
if self.maindex == 'true':Why not use a python boolean?
Previously used a string since we never needed to test the value, we just threw it into the C++ template but it makes sense to make it a boolean now.
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 12:Commit-Queue +1
nd
Yipeng Wang (Gerrit)
Yipeng Wang uploaded patch set #13 to this change.
Android JNI: Generate calls to RegisterNatives()
Generate registration functions with unique names(package+class). Create a new template to
generate a header file which calls all registration functions together.
Design doc: https://docs.google.com/document/d/1pYnceZMuxhpU9u3OAzWLYInV_nqtHKsBFROp927FDXM/edit?usp=sharing
Bug: 683256
Change-Id: I31cbfe393c9088233f65f1249285d48ac5571f45
---
M base/android/jni_generator/SampleForTests_jni.golden
M base/android/jni_generator/jni_generator.py
M base/android/jni_generator/jni_generator_helper.h
A base/android/jni_generator/jni_registration_generator.py
M base/android/jni_generator/testInnerClassNatives.golden
M base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
M base/android/jni_generator/testInnerClassNativesMultiple.golden
M base/android/jni_generator/testMainDexFile.golden
M base/android/jni_generator/testMultipleJNIAdditionalImport.golden
M base/android/jni_generator/testNatives.golden
M base/android/jni_generator/testNativesLong.golden
M base/android/jni_generator/testNonMainDexFile.golden
M base/android/jni_generator/testSingleJNIAdditionalImport.golden
M build/android/gradle/generate_gradle.py
M build/android/gyp/write_build_config.py
M build/config/android/rules.gni
M chrome/android/BUILD.gn
M chrome/browser/android/DEPS
M chrome/browser/android/chrome_entry_point.cc
19 files changed, 383 insertions(+), 5 deletions(-)
To view, visit change 527683. To unsubscribe, visit settings.
lpy (Gerrit)
lpy removed chrome-gr...@chromium.org from this change.
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 14:
(38 comments)
suggestion for title: "Android JNI: Generate calls to RegisterNatives()"
Done
Linking the design doc is a good idea, but you should move it to be hosted
Done
File base/android/jni_generator/jni_generator.py:
Patch Set #8, Line 784:
/ TODO(crbug/683256):nit: can you add in a // TODO(crbug/XXX) comment here about removing the em
Done
Patch Set #8, Line 892:
= 'JNI_I don't think it's worth reusing the template for this case since the argum
Done
Why replace _ with _1?
Done
Patch Set #8, Line 896:
'REGISTER_NATIVES_SIGNATURE': signature,Dont' use \ to escape a newline. Also preferred to use .format() over +:
Done
File base/android/jni_generator/jni_registration_generator.py:
drop the (c)
Done
nit: Similar to git commits, the first line should not wrap.
Done
optparse is deprecated. Use argparse instead (it's very similar)
Done
Against style guide to import a class from a module. Just import string and
Done
ditto
Done
nit: It's clearer to not accept |options| in helper functions. Instead, dir
We need the option to create the jni_generator.InlHeaderFileGenerator object.
Please add pydoc.
Done
Patch Set #8, Line 20:
def GenerateJNIHeader(java_file_paths, output_file, args):Is this used? Seems like it's reassigned on line 41.
It is passed to HeaderGenerator in each loop and get updated. Here I initialize the "dict" variable.
with open(path) as f:
Done
Patch Set #8, Line 30:
args: All input arguments.4 space indent
Done
Patch Set #8, Line 38:
path iHere and everywhere: main dex should be two words. main_dex, MainDex
Done
Patch Set #8, Line 44:
fully_qualified_class = jni_generator.ExtractFullyQualifiedJavaClassName('#include "{}"'.format(options.includes)
Done
Patch Set #8, Line 49:
jni_namespace = jni_generator.ExtractJNINamespace(contents)nit: Prefer to just not catch the exception rather than catching, printing
Done
Patch Set #8, Line 50:
natives = jni_generator.ExtractNatives(contents, 'long')Looks like from here down is copy/pasted with logic from jni_generator.py.
We don't need optimize_generation here, so just write to output_file.
Patch Set #8, Line 51:
if len(natives) == 0:nit: use a variable to store os.path.dirname(os.path.abspath(output_file))
Done
Patch Set #8, Line 54:
header_generator = HeaderGenerator(jni_namespace, fully_qualified_class,Use open(), not file(). No need to specify 'r'. It's the default.
Done
non-main dex
Done
use a regular if statement if the single-line one is going to cause wrappin
Done
Patch Set #8, Line 129:
def AddForwardDeclaration(self):indent
Done
Patch Set #8, Line 143:
self.inl_header_file_generator.GetCloseNamespaceString(),
Previously used a string since we never needed to test the value, we just t
Done
This is the same string transformation that is used above. You should creat
Done
Patch Set #8, Line 165:
gisterNatives()
I don't see you writing a depfile anywhere, but you should be (use build_ut
Done
nit: looke like this will fit on the previous line. (and a couple others be
Done
Patch Set #8, Line 175:
' file
Unused. Remove. And seems the default should be 'long'
I'd remove this note about where it's used.
Done
nit: this is stale. The variable is called "target", and "output"
Done
Patch Set #8, Line 355:
_build_config
prefix local variables with _
Done
Don't hardcode these in the template, but rather have them passed in as a v
Done
Patch Set #8, Line 370:
invoker.output,
local variables should start with _ to follow conventions. I'd just inline
Done
Patch Set #8, Line 396:
# This target will create a single .srcjar. Adding this target to an
I don't think there's value in adding an include dir for this. We should ju
nit: making this "${target_gen_dir}/${target_name}.h" might make it a bit
Done
File chrome/browser/android/chrome_entry_point.cc:
Patch Set #8, Line 38:
// TODO(crbug/683256) delete this block, this is a no-op now.
Add TODO(crbug/XXXX) to delete this part (and comment that it's a no-op now
Done
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 14:Commit-Queue +1
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 14:
(1 comment)
File base/android/jni_generator/jni_generator.py:
Done
Because that's how Java binary names are generated; _ is the escape character. I'm not sure if that matters in this specific context, but in any context where you're generating a symbol the VM is going to look up you need this replacement, so it might be best to be consistent anyway? (since the actual JNI methods will have _1 in them)
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 15:Commit-Queue +1
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 16:Commit-Queue +1
Yipeng Wang (Gerrit)
Hi,
jbudorick please review build/android/gyp/write_build_config.py
yfriedman please review files under chrome/browser/android/
dpranke please review testing/test.gni
torne feel free to give any suggestions.
Thanks,
Yipeng
Yipeng Wang (Gerrit)
Yipeng Wang would like Richard Coles, John Budorick, Yaron Friedman and Dirk Pranke to review this change.
Android JNI: Generate calls to RegisterNatives()
Generate registration functions with unique names(package+class). Create a new template to
generate a header file which calls all registration functions together.
Design doc: https://docs.google.com/document/d/1pYnceZMuxhpU9u3OAzWLYInV_nqtHKsBFROp927FDXM/edit?usp=sharing
Bug: 683256
Change-Id: I31cbfe393c9088233f65f1249285d48ac5571f45
---
M base/android/jni_generator/SampleForTests_jni.golden
M base/android/jni_generator/jni_generator.py
M base/android/jni_generator/jni_generator_helper.h
M base/android/jni_generator/jni_generator_tests.py
A base/android/jni_generator/jni_registration_generator.py
M base/android/jni_generator/testCalledByNatives.golden
M base/android/jni_generator/testConstantsFromJavaP.golden
M base/android/jni_generator/testFromJavaP.golden
M base/android/jni_generator/testFromJavaPGenerics.golden
M base/android/jni_generator/testInnerClassNatives.golden
M base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
M base/android/jni_generator/testInnerClassNativesMultiple.golden
M base/android/jni_generator/testMainDexFile.golden
M base/android/jni_generator/testMultipleJNIAdditionalImport.golden
M base/android/jni_generator/testNativeExportsOnlyOption.golden
M base/android/jni_generator/testNatives.golden
M base/android/jni_generator/testNativesLong.golden
M base/android/jni_generator/testNonMainDexFile.golden
M base/android/jni_generator/testSingleJNIAdditionalImport.golden
M build/android/gradle/generate_gradle.py
M build/android/gyp/write_build_config.py
M build/config/android/rules.gni
M chrome/android/BUILD.gn
M chrome/browser/android/DEPS
M chrome/browser/android/chrome_entry_point.cc
A chrome/browser/android/chrome_sync_shell_entry_point.cc
M testing/test.gni
27 files changed, 473 insertions(+), 71 deletions(-)
diff --git a/base/android/jni_generator/SampleForTests_jni.golden b/base/android/jni_generator/SampleForTests_jni.golden
index 7b38425..e82abe0 100644
--- a/base/android/jni_generator/SampleForTests_jni.golden
+++ b/base/android/jni_generator/SampleForTests_jni.golden
@@ -482,9 +482,14 @@
},
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool
+ RegisterNativeOrgChromiumExampleJni_GeneratorSamplefortests(JNIEnv* env) {
const int kMethodsInnerClassSize = arraysize(kMethodsInnerClass);
diff --git a/base/android/jni_generator/jni_generator.py b/base/android/jni_generator/jni_generator.py
index d4c3523..a58af87 100755
--- a/base/android/jni_generator/jni_generator.py
+++ b/base/android/jni_generator/jni_generator.py
@@ -184,6 +184,14 @@
_implicit_imports = []
@staticmethod
+ def ResetStates():
+ JniParams._imports = []
+ JniParams._fully_qualified_class = ''
+ JniParams._package = ''
+ JniParams._inner_classes = []
+ JniParams._implicit_imports = []
+
+ @staticmethod
def SetFullyQualifiedClass(fully_qualified_class):
JniParams._fully_qualified_class = 'L' + fully_qualified_class
JniParams._package = '/'.join(fully_qualified_class.split('/')[:-1])
@@ -413,7 +421,7 @@
def IsMainDexJavaClass(contents):
- """Returns "true" if the class is annotated with "@MainDex", "false" if not.
+ """Returns True if the class is annotated with "@MainDex", False if not.
JNI registration doesn't always need to be completed for non-browser processes
since most Java code is only used by the browser process. Classes that are
@@ -422,7 +430,7 @@
"""
re_maindex = re.compile(r'@MainDex[\s\S]*class({|[\s\S]*{)')
found = re.search(re_maindex, contents)
- return 'true' if found else 'false'
+ return True if found else False
def GetStaticCastForReturnType(return_type):
@@ -724,7 +732,7 @@
"""Generates an inline header file for JNI integration."""
def __init__(self, namespace, fully_qualified_class, natives,
- called_by_natives, constant_fields, options, maindex='false'):
+ called_by_natives, constant_fields, options, maindex=False):
self.namespace = namespace
self.fully_qualified_class = fully_qualified_class
self.class_name = self.fully_qualified_class.split('/')[-1]
@@ -773,6 +781,9 @@
// Step 3: RegisterNatives.
$JNI_NATIVE_METHODS
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+$REGISTER_NATIVES_EMPTY
$REGISTER_NATIVES
$CLOSE_NAMESPACE
@@ -786,7 +797,8 @@
'METHOD_STUBS': self.GetMethodStubsString(),
'OPEN_NAMESPACE': self.GetOpenNamespaceString(),
'JNI_NATIVE_METHODS': self.GetJNINativeMethodsString(),
- 'REGISTER_NATIVES': self.GetRegisterNativesString(),
+ 'REGISTER_NATIVES_EMPTY': self.GetRegisterNativesString(True),
+ 'REGISTER_NATIVES': self.GetRegisterNativesString(False),
'CLOSE_NAMESPACE': self.GetCloseNamespaceString(),
'HEADER_GUARD': self.header_guard,
'INCLUDES': self.GetIncludesString(),
@@ -860,7 +872,7 @@
""")
return self.SubstituteNativeMethods(template)
- def GetRegisterNativesString(self):
+ def GetRegisterNativesString(self, is_empty_register):
"""Returns the code for RegisterNatives."""
natives = self.GetRegisterNativesImplString()
if not natives:
@@ -868,26 +880,29 @@
template = Template("""\
${REGISTER_NATIVES_SIGNATURE} {
-${EARLY_EXIT}
${NATIVES}
return true;
}
""")
- signature = 'static bool RegisterNativesImpl(JNIEnv* env)'
- early_exit = ''
- if self.options.native_exports_optional:
- early_exit = """\
- if (jni_generator::ShouldSkipJniRegistration(%s))
- return true;
-""" % self.maindex
+ if is_empty_register:
+ return """static bool RegisterNativesImpl(JNIEnv* env) {
+ return true;
+}
+"""
+ signature = 'JNI_REGISTRATION_EXPORT bool {}(JNIEnv* env)'.format(
+ self.GetRegisterName())
- values = {'REGISTER_NATIVES_SIGNATURE': signature,
- 'EARLY_EXIT': early_exit,
- 'NATIVES': natives,
- }
+ values = {
+ 'REGISTER_NATIVES_SIGNATURE': signature,
+ 'NATIVES': natives
+ }
return template.substitute(values)
+ def GetRegisterName(self):
+ java_name = self.fully_qualified_class.title().replace('/', '')
+ return 'RegisterNative{}'.format(java_name)
+
def GetRegisterNativesImplString(self):
"""Returns the shared implementation for RegisterNatives."""
if not self.options.native_exports_optional:
@@ -1321,19 +1336,19 @@
print e
sys.exit(1)
if output_file:
- if not os.path.exists(os.path.dirname(os.path.abspath(output_file))):
- os.makedirs(os.path.dirname(os.path.abspath(output_file)))
+ output_file_path = os.path.dirname(os.path.abspath(output_file))
+ if not os.path.exists(output_file_path):
+ os.makedirs(output_file_path)
if options.optimize_generation and os.path.exists(output_file):
- with file(output_file, 'r') as f:
+ with open(output_file) as f:
existing_content = f.read()
if existing_content == content:
return
- with file(output_file, 'w') as f:
+ with open(output_file, 'w') as f:
f.write(content)
else:
print content
-
def GetScriptName():
script_components = os.path.abspath(sys.argv[0]).split(os.path.sep)
base_index = 0
diff --git a/base/android/jni_generator/jni_generator_helper.h b/base/android/jni_generator/jni_generator_helper.h
index 3062806..84be86f 100644
--- a/base/android/jni_generator/jni_generator_helper.h
+++ b/base/android/jni_generator/jni_generator_helper.h
@@ -30,6 +30,9 @@
#define JNI_GENERATOR_EXPORT extern "C" __attribute__((visibility("default")))
#endif
+// Used to export JNI registration functions.
+#define JNI_REGISTRATION_EXPORT __attribute__((visibility("default")))
+
namespace jni_generator {
inline void HandleRegistrationError(JNIEnv* env,
diff --git a/base/android/jni_generator/jni_generator_tests.py b/base/android/jni_generator/jni_generator_tests.py
index feac90a..e792642 100755
--- a/base/android/jni_generator/jni_generator_tests.py
+++ b/base/android/jni_generator/jni_generator_tests.py
@@ -954,33 +954,6 @@
natives, [], [], test_options)
self.assertGoldenTextEquals(h.GetContent())
- def testMainDexFile(self):
- test_data = """
- package org.chromium.example.jni_generator;
-
- @MainDex
- class Test {
- private static native int nativeStaticMethod(long nativeTest, int arg1);
- }
- """
- options = TestOptions()
- jni_from_java = jni_generator.JNIFromJavaSource(
- test_data, 'org/chromium/foo/Bar', options)
- self.assertGoldenTextEquals(jni_from_java.GetContent())
-
- def testNonMainDexFile(self):
- test_data = """
- package org.chromium.example.jni_generator;
-
- class Test {
- private static native int nativeStaticMethod(long nativeTest, int arg1);
- }
- """
- options = TestOptions()
- jni_from_java = jni_generator.JNIFromJavaSource(
- test_data, 'org/chromium/foo/Bar', options)
- self.assertGoldenTextEquals(jni_from_java.GetContent())
-
def testMainDexAnnotation(self):
mainDexEntries = [
'@MainDex public class Test {',
@@ -1008,7 +981,7 @@
'@MainDex public class Test extends Testable<java.io.Serializable> {',
]
for entry in mainDexEntries:
- self.assertEquals("true", IsMainDexJavaClass(entry))
+ self.assertEquals(True, IsMainDexJavaClass(entry))
def testNoMainDexAnnotation(self):
noMainDexEntries = [
@@ -1018,7 +991,7 @@
'public class Test extends BaseTest {'
]
for entry in noMainDexEntries:
- self.assertEquals("false", IsMainDexJavaClass(entry))
+ self.assertEquals(False, IsMainDexJavaClass(entry))
def testNativeExportsOnlyOption(self):
test_data = """
diff --git a/base/android/jni_generator/jni_registration_generator.py b/base/android/jni_generator/jni_registration_generator.py
new file mode 100755
index 0000000..619b60e
--- /dev/null
+++ b/base/android/jni_generator/jni_registration_generator.py
@@ -0,0 +1,207 @@
+#!/usr/bin/env python
+# Copyright 2017 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.
+
+"""Generate JNI registration entry points
+
+Creates a header file with two static functions: RegisterMainDexNatives() and
+RegisterNonMainDexNatives(). Together, these will use manual JNI registration
+to register all native methods that exist within an application."""
+
+import os
+import sys
+import argparse
+import string
+
+import jni_generator
+from util import build_utils
+
+def GenerateJNIHeader(java_file_paths, output_file, args):
+ """Generate a header file including two registration functions.
+
+ Forward declares all JNI registration functions created by jni_generator.py.
+ Calls the functions in RegisterMainDexNatives() if they are main dex. And
+ calls them in RegisterNonMainDexNatives() if they are non-main dex.
+
+ Args:
+ java_file_paths: A list of java file paths.
+ output_file: A relative path to output file.
+ args: All input arguments.
+ """
+ dict = {
+ 'FORWARD_DECLARATIONS': '',
+ 'REGISTER_MAIN_DEX_NATIVES': '',
+ 'REGISTER_NON_MAIN_DEX_NATIVES': ''
+ }
+ for path in java_file_paths:
+ if path in args.no_register_java:
+ continue
+ with open(path) as f:
+ contents = f.read()
+
+ # Get the package name and class name.
+ fully_qualified_class = jni_generator.ExtractFullyQualifiedJavaClassName(
+ path, contents)
+ jni_generator.JniParams.ResetStates()
+ jni_generator.JniParams.SetFullyQualifiedClass(fully_qualified_class)
+ jni_generator.JniParams.ExtractImportsAndInnerClasses(contents)
+ jni_namespace = jni_generator.ExtractJNINamespace(contents)
+ natives = jni_generator.ExtractNatives(contents, 'long')
+ if len(natives) == 0:
+ continue
+ main_dex = jni_generator.IsMainDexJavaClass(contents)
+ header_generator = HeaderGenerator(jni_namespace, fully_qualified_class,
+ natives, args, dict, main_dex)
+ dict = header_generator.GetContent()
+
+ include_file = ''
+ if args.includes:
+ include_file = '#include "{}"'.format(args.includes)
+ dict['INCLUDES'] = include_file
+ header_content = CreateFromDict(dict)
+
+ if output_file:
+ output_file_path = os.path.dirname(os.path.abspath(output_file))
+ if not os.path.exists(output_file_path):
+ os.makedirs(output_file_path)
+ with open(output_file, 'w') as f:
+ f.write(header_content)
+ else:
+ print header_content
+
+def CreateFromDict(dict):
+ """Returns the content of the header file."""
+
+ template = string.Template("""\
+// Copyright 2017 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.
+
+
+// This file is autogenerated by
+// base/android/jni_generator/jni_registration_generator.py
+// Please do not change its content.
+
+#ifndef HEADER_GUARD
+#define HEADER_GUARD
+
+#include <jni.h>
+
+${INCLUDES}
+
+#include "base/android/jni_int_wrapper.h"
+
+// Step 1: Forward declaration.
+${FORWARD_DECLARATIONS}
+
+// Step 2: Main dex and non-main dex registration functions.
+
+bool RegisterMainDexNatives(JNIEnv* env) {
+${REGISTER_MAIN_DEX_NATIVES}
+
+ return true;
+}
+
+bool RegisterNonMainDexNatives(JNIEnv* env) {
+${REGISTER_NON_MAIN_DEX_NATIVES}
+
+ return true;
+}
+
+#endif // HEADER_GUARD
+""")
+ if len(dict['FORWARD_DECLARATIONS']) == 0:
+ return ''
+ return jni_generator.WrapOutput(template.substitute(dict))
+
+
+class HeaderGenerator(object):
+ """Generates an inline header file for JNI registration."""
+
+ def __init__(self, namespace, fully_qualified_class, natives,
+ args, dict, main_dex):
+ self.namespace = namespace
+ self.fully_qualified_class = fully_qualified_class
+ self.class_name = self.fully_qualified_class.split('/')[-1]
+ self.natives = natives
+ self.args = args
+ self.dict = dict
+ self.main_dex = main_dex
+ self.inl_header_file_generator = jni_generator.InlHeaderFileGenerator(
+ self.namespace, self.fully_qualified_class, self.natives, [], [],
+ self.args)
+
+ def AddForwardDeclaration(self):
+ """Add the content of the forward declaration to the dictionary."""
+ template = string.Template("""\
+$OPEN_NAMESPACE
+$METHOD_SIGNATURE
+$CLOSE_NAMESPACE
+""")
+ signature = 'JNI_REGISTRATION_EXPORT bool {}(JNIEnv* env);'.format(
+ self.inl_header_file_generator.GetRegisterName())
+ values = {
+ 'OPEN_NAMESPACE':
+ self.inl_header_file_generator.GetOpenNamespaceString(),
+ 'METHOD_SIGNATURE': signature,
+ 'CLOSE_NAMESPACE':
+ self.inl_header_file_generator.GetCloseNamespaceString(),
+ }
+ self.dict['FORWARD_DECLARATIONS'] += template.substitute(values) + '\n\n'
+
+ def AddRegisterNatives(self):
+ """Add the body of the RegisterNativesImpl method to the dictionary."""
+ register_function = '{}(env)'.format(
+ self.inl_header_file_generator.GetRegisterName())
+ if self.namespace:
+ register_function = self.namespace + '::' + register_function
+ register_function_block = """\
+ if (!%s)
+ return false;
+
+""" % register_function
+ if self.main_dex:
+ self.dict['REGISTER_MAIN_DEX_NATIVES'] += register_function_block
+ else:
+ self.dict['REGISTER_NON_MAIN_DEX_NATIVES'] += register_function_block
+
+ def GetContent(self):
+ self.AddForwardDeclaration()
+ self.AddRegisterNatives()
+ return self.dict
+
+
+def main(argv):
+ arg_parser = argparse.ArgumentParser()
+ build_utils.AddDepfileOption(arg_parser)
+
+ arg_parser.add_argument('--sources_files',
+ help='A list of .sources files which contain Java'
+ ' file paths. Must be used with --output.')
+ arg_parser.add_argument('--output',
+ help='The output file path.')
+ arg_parser.add_argument('--includes',
+ help='Helper file for the generated header file')
+ arg_parser.add_argument('--no_register_java',
+ help='A list of Java files which should be ignored '
+ 'by the parser.')
+ args = arg_parser.parse_args(build_utils.ExpandFileArgs(argv[1:]))
+ args.sources_files = build_utils.ParseGnList(args.sources_files)
+
+ if args.sources_files:
+ java_file_paths = []
+ for f in args.sources_files:
+ # java_file_paths stores each Java file path as a string.
+ java_file_paths += build_utils.ReadSourcesList(f)
+ else:
+ print '\nError: Must specify --sources_files.'
+ return 1
+ output_file = args.output
+ GenerateJNIHeader(java_file_paths, output_file, args)
+
+ if args.depfile:
+ build_utils.WriteDepfile(args.depfile, output_file)
+
+if __name__ == '__main__':
+ sys.exit(main(sys.argv))
diff --git a/base/android/jni_generator/testCalledByNatives.golden b/base/android/jni_generator/testCalledByNatives.golden
index ac86b2e..9307e42 100644
--- a/base/android/jni_generator/testCalledByNatives.golden
+++ b/base/android/jni_generator/testCalledByNatives.golden
@@ -494,4 +494,7 @@
// Step 3: RegisterNatives.
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+
#endif // org_chromium_TestJni_JNI
diff --git a/base/android/jni_generator/testConstantsFromJavaP.golden b/base/android/jni_generator/testConstantsFromJavaP.golden
index b16956f..177170f 100644
--- a/base/android/jni_generator/testConstantsFromJavaP.golden
+++ b/base/android/jni_generator/testConstantsFromJavaP.golden
@@ -2190,6 +2190,9 @@
// Step 3: RegisterNatives.
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+
} // namespace JNI_MotionEvent
#endif // android_view_MotionEvent_JNI
diff --git a/base/android/jni_generator/testFromJavaP.golden b/base/android/jni_generator/testFromJavaP.golden
index 18a9430..3e39213 100644
--- a/base/android/jni_generator/testFromJavaP.golden
+++ b/base/android/jni_generator/testFromJavaP.golden
@@ -255,6 +255,9 @@
// Step 3: RegisterNatives.
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+
} // namespace JNI_InputStream
#endif // java_io_InputStream_JNI
diff --git a/base/android/jni_generator/testFromJavaPGenerics.golden b/base/android/jni_generator/testFromJavaPGenerics.golden
index c076c39c..2b09cfe 100644
--- a/base/android/jni_generator/testFromJavaPGenerics.golden
+++ b/base/android/jni_generator/testFromJavaPGenerics.golden
@@ -51,6 +51,9 @@
// Step 3: RegisterNatives.
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+
} // namespace JNI_HashSet
#endif // java_util_HashSet_JNI
diff --git a/base/android/jni_generator/testInnerClassNatives.golden b/base/android/jni_generator/testInnerClassNatives.golden
index 20b8830..eccbafa 100644
--- a/base/android/jni_generator/testInnerClassNatives.golden
+++ b/base/android/jni_generator/testInnerClassNatives.golden
@@ -51,9 +51,13 @@
},
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumTestjni(JNIEnv* env) {
const int kMethodsMyInnerClassSize = arraysize(kMethodsMyInnerClass);
diff --git a/base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden b/base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
index 67352e7..42cb6ee 100644
--- a/base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
+++ b/base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
@@ -67,9 +67,13 @@
"I", reinterpret_cast<void*>(Java_org_chromium_TestJni_nativeInit) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumTestjni(JNIEnv* env) {
const int kMethodsMyOtherInnerClassSize =
arraysize(kMethodsMyOtherInnerClass);
diff --git a/base/android/jni_generator/testInnerClassNativesMultiple.golden b/base/android/jni_generator/testInnerClassNativesMultiple.golden
index 7807efa..94dec03 100644
--- a/base/android/jni_generator/testInnerClassNativesMultiple.golden
+++ b/base/android/jni_generator/testInnerClassNativesMultiple.golden
@@ -74,9 +74,13 @@
},
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumTestjni(JNIEnv* env) {
const int kMethodsMyOtherInnerClassSize =
arraysize(kMethodsMyOtherInnerClass);
diff --git a/base/android/jni_generator/testMainDexFile.golden b/base/android/jni_generator/testMainDexFile.golden
index cbb2a7d..7d3abe1 100644
--- a/base/android/jni_generator/testMainDexFile.golden
+++ b/base/android/jni_generator/testMainDexFile.golden
@@ -47,7 +47,13 @@
"I", reinterpret_cast<void*>(Java_org_chromium_foo_Bar_nativeStaticMethod) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumFooBar(JNIEnv* env) {
if (jni_generator::ShouldSkipJniRegistration(true))
return true;
diff --git a/base/android/jni_generator/testMultipleJNIAdditionalImport.golden b/base/android/jni_generator/testMultipleJNIAdditionalImport.golden
index 0eecb5a..7b86727 100644
--- a/base/android/jni_generator/testMultipleJNIAdditionalImport.golden
+++ b/base/android/jni_generator/testMultipleJNIAdditionalImport.golden
@@ -75,9 +75,13 @@
"V", reinterpret_cast<void*>(Java_org_chromium_foo_Foo_nativeDoSomething) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumFooFoo(JNIEnv* env) {
const int kMethodsFooSize = arraysize(kMethodsFoo);
diff --git a/base/android/jni_generator/testNativeExportsOnlyOption.golden b/base/android/jni_generator/testNativeExportsOnlyOption.golden
index 6a50619..582838f 100644
--- a/base/android/jni_generator/testNativeExportsOnlyOption.golden
+++ b/base/android/jni_generator/testNativeExportsOnlyOption.golden
@@ -182,4 +182,7 @@
// Step 3: RegisterNatives.
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
+
#endif // org_chromium_example_jni_generator_SampleForTests_JNI
diff --git a/base/android/jni_generator/testNatives.golden b/base/android/jni_generator/testNatives.golden
index 3362c92..526c88a 100644
--- a/base/android/jni_generator/testNatives.golden
+++ b/base/android/jni_generator/testNatives.golden
@@ -320,9 +320,13 @@
},
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumTestjni(JNIEnv* env) {
const int kMethodsTestJniSize = arraysize(kMethodsTestJni);
diff --git a/base/android/jni_generator/testNativesLong.golden b/base/android/jni_generator/testNativesLong.golden
index ec029ce..18b469f 100644
--- a/base/android/jni_generator/testNativesLong.golden
+++ b/base/android/jni_generator/testNativesLong.golden
@@ -46,9 +46,13 @@
"V", reinterpret_cast<void*>(Java_org_chromium_TestJni_nativeDestroy) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumTestjni(JNIEnv* env) {
const int kMethodsTestJniSize = arraysize(kMethodsTestJni);
diff --git a/base/android/jni_generator/testNonMainDexFile.golden b/base/android/jni_generator/testNonMainDexFile.golden
index 533241e..b8d8797 100644
--- a/base/android/jni_generator/testNonMainDexFile.golden
+++ b/base/android/jni_generator/testNonMainDexFile.golden
@@ -47,7 +47,13 @@
"I", reinterpret_cast<void*>(Java_org_chromium_foo_Bar_nativeStaticMethod) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumFooBar(JNIEnv* env) {
if (jni_generator::ShouldSkipJniRegistration(false))
return true;
diff --git a/base/android/jni_generator/testSingleJNIAdditionalImport.golden b/base/android/jni_generator/testSingleJNIAdditionalImport.golden
index ef618da8..4b2578a 100644
--- a/base/android/jni_generator/testSingleJNIAdditionalImport.golden
+++ b/base/android/jni_generator/testSingleJNIAdditionalImport.golden
@@ -69,9 +69,13 @@
"V", reinterpret_cast<void*>(Java_org_chromium_foo_Foo_nativeDoSomething) },
};
+// TODO(crbug/683256): Remove these empty registration functions and functions
+// calling them.
static bool RegisterNativesImpl(JNIEnv* env) {
- if (jni_generator::ShouldSkipJniRegistration(false))
- return true;
+ return true;
+}
+
+JNI_REGISTRATION_EXPORT bool RegisterNativeOrgChromiumFooFoo(JNIEnv* env) {
const int kMethodsFooSize = arraysize(kMethodsFoo);
diff --git a/build/android/gradle/generate_gradle.py b/build/android/gradle/generate_gradle.py
index 6269bfb..112e85f 100755
--- a/build/android/gradle/generate_gradle.py
+++ b/build/android/gradle/generate_gradle.py
@@ -210,7 +210,7 @@
def JavaFiles(self):
if self._java_files is None:
- java_sources_file = self.Gradle().get('java_sources_file')
+ java_sources_file = self.DepsInfo().get('java_sources_file')
java_files = []
if java_sources_file:
java_sources_file = _RebasePath(java_sources_file)
diff --git a/build/android/gyp/write_build_config.py b/build/android/gyp/write_build_config.py
index 2da62f8..d58dc7e 100755
--- a/build/android/gyp/write_build_config.py
+++ b/build/android/gyp/write_build_config.py
@@ -413,7 +413,7 @@
gradle['android_manifest'] = options.android_manifest
if options.type in ('java_binary', 'java_library', 'android_apk'):
if options.java_sources_file:
- gradle['java_sources_file'] = options.java_sources_file
+ deps_info['java_sources_file'] = options.java_sources_file
if options.bundled_srcjars:
gradle['bundled_srcjars'] = (
build_utils.ParseGnList(options.bundled_srcjars))
@@ -434,6 +434,12 @@
gradle['dependent_java_projects'].append(c['path'])
+ if options.type == 'android_apk':
+ config['jni'] = {}
+ all_java_sources = [c['java_sources_file'] for c in all_library_deps
+ if 'java_sources_file' in c]
+ config['jni']['all_source'] = all_java_sources
+
if (options.type in ('java_binary', 'java_library')):
deps_info['requires_android'] = options.requires_android
deps_info['supports_android'] = options.supports_android
diff --git a/build/config/android/rules.gni b/build/config/android/rules.gni
index b3785d6..a44a578 100644
--- a/build/config/android/rules.gni
+++ b/build/config/android/rules.gni
@@ -334,6 +334,57 @@
}
}
+ # Declare a jni registration target.
+ #
+ # This target generates registration functions for all .java files calling
+ # native methods.
+ #
+ # See base/android/jni_generator/jni_registration_generator.py for more info
+ # about the format of the registration functions.
+ #
+ # Variables
+ # target: chrome_public_apk.build_config
+ #
+ # Example
+ # generate_jni_registration("chrome_jni_registration") {
+ # target = ":chrome_public_apk"
+ # output = "${target_gen_dir}/${target_name}/chrome_jni_registration.h"
+ # }
+ template("generate_jni_registration") {
+ action(target_name) {
+ forward_variables_from(invoker, [ "testonly" ])
+ _build_config = get_label_info(invoker.target, "target_gen_dir") + "/" +
+ get_label_info(invoker.target, "name") + ".build_config"
+ _rebased_build_config = rebase_path(_build_config, root_build_dir)
+
+ _rebase_exception_java_files =
+ rebase_path(invoker.exception_files, root_build_dir)
+
+ script = "//base/android/jni_generator/jni_registration_generator.py"
+ deps = [
+ "${invoker.target}__build_config",
+ ]
+ inputs = [
+ _build_config,
+ ]
+ outputs = [
+ invoker.output,
+ ]
+ _jni_generator_include =
+ "//base/android/jni_generator/jni_generator_helper.h"
+
+ args = [
+ # This is a list of .sources files.
+ "--sources_files=@FileArg($_rebased_build_config:jni:all_source)",
+ "--output",
+ rebase_path(invoker.output, root_build_dir),
+ "--includes",
+ rebase_path(_jni_generator_include, root_build_dir),
+ "--no_register_java=$_rebase_exception_java_files",
+ ]
+ }
+ }
+
# Declare a target for c-preprocessor-generated java files
#
# NOTE: For generating Java conterparts to enums prefer using the java_cpp_enum
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
index 429b2d5..25196b6 100644
--- a/chrome/android/BUILD.gn
+++ b/chrome/android/BUILD.gn
@@ -668,6 +668,7 @@
"../browser/android/chrome_entry_point.cc",
]
deps = [
+ ":chrome_jni_registration($default_toolchain)",
"//build/config:exe_and_shlib_deps",
"//chrome:chrome_android_core",
]
@@ -694,6 +695,27 @@
# Ensure that .pak files are built only once (build them in the default
# toolchain).
if (current_toolchain == default_toolchain) {
+ generate_jni_registration("chrome_jni_registration") {
+ target = ":chrome_public_apk"
+ output = "$root_gen_dir/chrome/browser/android/${target_name}.h"
+ exception_files = [
+ "//base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java",
+ "//base/android/java/src/org/chromium/base/library_loader/Linker.java",
+ "//base/android/java/src/org/chromium/base/library_loader/ModernLinker.java",
+ ]
+ }
+
+ generate_jni_registration("chrome_sync_shell_jni_registration") {
+ testonly = true
+ target = ":chrome_sync_shell_apk"
+ output = "$root_gen_dir/chrome/browser/android/${target_name}.h"
+ exception_files = [
+ "//base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java",
+ "//base/android/java/src/org/chromium/base/library_loader/Linker.java",
+ "//base/android/java/src/org/chromium/base/library_loader/ModernLinker.java",
+ ]
+ }
+
if (enable_resource_whitelist_generation) {
generate_resource_whitelist("monochrome_resource_whitelist") {
# Always use the 32-bit library's whitelist since the 64-bit one is
@@ -789,12 +811,13 @@
shared_library("chrome_sync_shell") {
testonly = true
sources = [
- "../browser/android/chrome_entry_point.cc",
+ "../browser/android/chrome_sync_shell_entry_point.cc",
"../browser/android/chrome_sync_shell_main_delegate.cc",
"../browser/android/chrome_sync_shell_main_delegate.h",
"../browser/android/chrome_sync_shell_main_delegate_initializer.cc",
]
deps = [
+ ":chrome_sync_shell_jni_registration($default_toolchain)",
"//build/config:exe_and_shlib_deps",
"//chrome:chrome_android_core",
"//components/sync",
diff --git a/chrome/browser/android/DEPS b/chrome/browser/android/DEPS
index bb55a8c..8f032b7 100644
--- a/chrome/browser/android/DEPS
+++ b/chrome/browser/android/DEPS
@@ -2,6 +2,7 @@
"-components/devtools_bridge",
"+cc/layers/layer.h",
"+cc/output/context_provider.h",
+ "+chrome_jni_registration/chrome_jni_registration.h",
"+components/doodle",
"+components/ntp_snippets",
"+components/spellcheck/browser",
diff --git a/chrome/browser/android/chrome_entry_point.cc b/chrome/browser/android/chrome_entry_point.cc
index ee46cd6..9fd00cf 100644
--- a/chrome/browser/android/chrome_entry_point.cc
+++ b/chrome/browser/android/chrome_entry_point.cc
@@ -7,6 +7,7 @@
#include "base/android/library_loader/library_loader_hooks.h"
#include "base/bind.h"
#include "chrome/app/android/chrome_jni_onload.h"
+#include "chrome/browser/android/chrome_jni_registration.h"
namespace {
@@ -23,6 +24,18 @@
// Java side and only register a subset of JNI methods.
base::android::InitVM(vm);
JNIEnv* env = base::android::AttachCurrentThread();
+
+ if (!base::android::IsSelectiveJniRegistrationEnabled(env)) {
+ if (!RegisterNonMainDexNatives(env)) {
+ return -1;
+ }
+ }
+
+ if (!RegisterMainDexNatives(env)) {
+ return -1;
+ }
+
+ // TODO(crbug/683256) delete this block, this is a no-op now.
if (base::android::IsSelectiveJniRegistrationEnabled(env)) {
base::android::SetJniRegistrationType(
base::android::SELECTIVE_JNI_REGISTRATION);
diff --git a/chrome/browser/android/chrome_sync_shell_entry_point.cc b/chrome/browser/android/chrome_sync_shell_entry_point.cc
new file mode 100644
index 0000000..3a12377
--- /dev/null
+++ b/chrome/browser/android/chrome_sync_shell_entry_point.cc
@@ -0,0 +1,48 @@
+// Copyright 2017 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.
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_utils.h"
+#include "base/android/library_loader/library_loader_hooks.h"
+#include "base/bind.h"
+#include "chrome/app/android/chrome_jni_onload.h"
+#include "chrome/browser/android/chrome_sync_shell_jni_registration.h"
+
+namespace {
+
+bool NativeInit() {
+ return android::OnJNIOnLoadInit();
+}
+
+} // namespace
+
+// This is called by the VM when the shared library is first loaded.
+JNI_EXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
+ // By default, all JNI methods are registered. However, since render processes
+ // don't need very much Java code, we enable selective JNI registration on the
+ // Java side and only register a subset of JNI methods.
+ base::android::InitVM(vm);
+ JNIEnv* env = base::android::AttachCurrentThread();
+
+ if (!base::android::IsSelectiveJniRegistrationEnabled(env)) {
+ if (!RegisterNonMainDexNatives(env)) {
+ return -1;
+ }
+ }
+
+ if (!RegisterMainDexNatives(env)) {
+ return -1;
+ }
+
+ // TODO(crbug/683256) delete this block, this is a no-op now.
+ if (base::android::IsSelectiveJniRegistrationEnabled(env)) {
+ base::android::SetJniRegistrationType(
+ base::android::SELECTIVE_JNI_REGISTRATION);
+ }
+ if (!android::OnJNIOnLoadRegisterJNI(env)) {
+ return -1;
+ }
+ base::android::SetNativeInitializationHook(NativeInit);
+ return JNI_VERSION_1_4;
+}
diff --git a/testing/test.gni b/testing/test.gni
index 36e0d91..64619b3 100644
--- a/testing/test.gni
+++ b/testing/test.gni
@@ -63,7 +63,7 @@
# the default shared_library configs rather than executable configs.
configs -= [
"//build/config:shared_library_config",
- "//build/config/android:hide_all_but_jni_onload",
+ "//build/config/android:hide_all_but_jni",
]
configs += [ "//build/config:executable_config" ]
@@ -321,6 +321,8 @@
set_defaults("test") {
if (is_android) {
configs = default_shared_library_configs
+ configs -= [ "//build/config/android:hide_all_but_jni_onload" ]
+ configs += [ "//build/config/android:hide_all_but_jni" ]
} else {
configs = default_executable_configs
}
To view, visit change 527683. To unsubscribe, visit settings.
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 16:
Haven't looked at this in detail yet but there are definitely other JNI entry points you need to call this from: cronet is the first one that comes to mind? Aren't there also other test entry points? Have you looked at all the places that call the current manual registration functions?
Richard Coles (Gerrit)
The general approach here looks good.
(18 comments)
File base/android/jni_generator/jni_generator.py:
Patch Set #16, Line 187:
def ResetStates():Adding a reset method here to get around the script expecting to only be run for one class is a bit gross; just making this non-static seems like it'd be better.
Patch Set #16, Line 735:
maindexIt doesn't look like the generator actually uses this any more? If not, just remove it.
Patch Set #16, Line 784:
crbugnit: preferable to write crbug.com as the crbug/ redirector only exists inside google
Also, the comment gets generated even when $REGISTER_NATIVES_EMPTY is empty; you can make it part of that string literal instead of the template?Patch Set #16, Line 875:
is_empty_registerThe empty case doesn't really seem to have anything in common with the real case. Just do it separately, I think the function would be simpler.
Patch Set #16, Line 892:
formatUse Template to generate this string like the rest of the file. We aren't using str.format anywhere else and its placeholder {} is confusing in the context of C++ code.
Patch Set #16, Line 903:
java_name = self.fully_qualified_class.title().replace('/', '')I think you should use the actual binary name of the class here instead of making up a new name mangling convention - you could pull the logic from GetStubName out into a separate function and share it.
Patch Set #16, Line 1339:
output_file_path = os.path.dirname(os.path.abspath(output_file))It's usually better to split this kind of unrelated cleanup into a separate CL when you're already doing something quite complicated.
File base/android/jni_generator/jni_generator_helper.h:
Patch Set #16, Line 34:
#define JNI_REGISTRATION_EXPORT __attribute__((visibility("default")))I assume you're exporting this because otherwise they aren't visible from outside the component in a component build? That seems unfortunate to have to do it that way. Limit this to component builds only at least?
Patch Set #16, Line 48:
inline bool ShouldSkipJniRegistration(bool is_maindex_class) {Can we delete this now?
File base/android/jni_generator/jni_registration_generator.py:
Patch Set #16, Line 37:
for path in java_file_paths:Is there guaranteed to be any particular order of these inputs? I don't think so?
Can you sort the list so that the output of the script is deterministic?Patch Set #16, Line 48:
jni_generator.JniParams.ExtractImportsAndInnerClasses(contents)It seems like you're having to do a lot of things that this script doesn't really care about just to satisfy the existing API. I'd suggest just refactoring the existing script a bit more to make the actual things you care about more standalone.
It looks like all you need to be able to do here is:
1) Check whether the class has any natives or not
2) Check whether it's in the main dex
3) Get the name of the registration function for the class
4) Get the C++ namespace the class uses (which could be avoided if you just generated the registration function outside the namespace to begin with, actually).This seems like it could be done with a much simpler API that wouldn't require creating an instance of InlHeaderFileGenerator which needs all these calls to gather its parameters.
Patch Set #16, Line 60:
formatsame comment about str.format in this file too :)
Patch Set #16, Line 154:
"""Add the body of the RegisterNativesImpl method to the dictionary."""This just adds a call to it, not the body, right?
Patch Set #16, Line 186:
arg_parser.add_argument('--no_register_java',What is this needed for?
File base/android/jni_generator/testMainDexFile.golden:
You removed the test that uses this file; you should be able to just delete it?
File base/android/jni_generator/testNonMainDexFile.golden:
You removed the test that uses this file; you should be able to just delete it?
File chrome/browser/android/chrome_sync_shell_entry_point.cc:
Why do we have to duplicate this?
Patch Set #16, Line 66:
"//build/config/android:hide_all_but_jni",I guess you're switching this to allow regular JVM lookup so you don't have to update all the test entry points and have all the tests generate a registration function?
I'm not sure I like that; having tests rely on the same mechanism that the shipping binary (or at least the most restrictive one) uses seems like a good way to make sure the mechanism is working properly. Even though this change makes the chance of incorrect registration much lower...
To view, visit change 527683. To unsubscribe, visit settings.
Andrew Grieve (Gerrit)
Andrew Grieve posted comments on this change.
Patch set 16:
Thanks for the review torne! Replied just to a couple comments and will let Yipeng address the rest.
(2 comments)
Patch Set #16, Line 37:
for path in java_file_paths:Is there guaranteed to be any particular order of these inputs? I don't thi
Had a look, and I think it will be deterministic in that it's the order that the files are specified within BUILD.gn files. Agree sorting takes any doubt away though :)
Patch Set #16, Line 66:
"//build/config/android:hide_all_but_jni",I guess you're switching this to allow regular JVM lookup so you don't have
One snag we hit with this new approach, is that the shared_library() target must have a direct dependency on all component()s that contain JNI. Otherwise, you get a linker error.
This came up for libchrome.so here:
https://chromium-review.googlesource.com/c/527678/4/chrome/BUILD.gnI don't want to introduce this onus on our tests when it's easily avoidable anyways. If we did want to introduce it, then I think we should also enable relocation packing for our test apks, but I just don't think the value is there.
To view, visit change 527683. To unsubscribe, visit settings.
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 16:
I had a look through the entry points declared in the project and I think you need to also update:
- content shell
- remoting client
- chromecast shell
- cronet
and possibly some others (most of the rest look like tests which are covered by the test symbol policy change here).
Many of these things are not actually executed on trybots (some not even on the main waterfall possibly?) and so the commit queue being happy with your change isn't enough here - you need to check all the places that currently do manual JNI registration, and make sure that all of them (other than webview and monochrome, and targets affected by your test change) get the new generated logic instead.
(1 comment)
Patch Set #16, Line 66:
"//build/config/android:hide_all_but_jni",
One snag we hit with this new approach, is that the shared_library() target
Ah, that's kind of a pain; I hadn't thought of that. Yeah, it's easier not to make that an issue for the tests too.
To view, visit change 527683. To unsubscribe, visit settings.
John Budorick (Gerrit)
John Budorick posted comments on this change.
Patch set 16:
Patch Set 16:
Hi,
jbudorick please review build/android/gyp/write_build_config.py
build/android/ lgtm. Didn't look at rules.gni but don't mind doing so if you'd like.
yfriedman please review files under chrome/browser/android/
dpranke please review testing/test.gni
torne feel free to give any suggestions.Thanks,
Yipeng
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 17:
(19 comments)
File base/android/jni_generator/jni_generator.py:
Patch Set #16, Line 187:
self._implicit_imports = []
Adding a reset method here to get around the script expecting to only be ru
Refactor this class in CL 538141
It doesn't look like the generator actually uses this any more? If not, jus
Done
Patch Set #16, Line 784:
'CONS
nit: preferable to write crbug.com as the crbug/ redirector only exists ins
Done
The empty case doesn't really seem to have anything in common with the real
Done
Use Template to generate this string like the rest of the file. We aren't u
Use str.Template everywhere now.
Patch Set #16, Line 903:
if not self.options.native_exports_optional:
I think you should use the actual binary name of the class here instead of
Done
Patch Set #16, Line 1339:
def WriteOutput(output_file, content):
It's usually better to split this kind of unrelated cleanup into a separate
I just removed this part since ninja generates missing directory automatically.
File base/android/jni_generator/jni_generator_helper.h:
Patch Set #16, Line 34:
#if defined(COMPONENT_BUILD)
I assume you're exporting this because otherwise they aren't visible from o
Done
Can we delete this now?
This function is also called by some manual registration functions such as "third_party/gvr-android-sdk/gvr_api_jni.h"
File base/android/jni_generator/jni_registration_generator.py:
Patch Set #16, Line 37:
# Sort the file list to make sure the order is deterministic.
Had a look, and I think it will be deterministic in that it's the order tha
Done
Patch Set #16, Line 48:
path, contents)
It seems like you're having to do a lot of things that this script doesn't
Now the InlHeaderFileGenerator class is no longer used in this script.
We generate the new registration function outside of the namespace now.
same comment about str.format in this file too :)
Done
same comment about str.format in this file too :)
Done
Patch Set #16, Line 154:
build_utils.AddDepfileOption(arg_parser)
This just adds a call to it, not the body, right?
Done
What is this needed for?
We use this arg to exclude the linker files at
"//base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java",
"//base/android/java/src/org/chromium/base/library_loader/Linker.java",
"//base/android/java/src/org/chromium/base/library_loader/ModernLinker.java".
File base/android/jni_generator/testMainDexFile.golden:
You removed the test that uses this file; you should be able to just delete
You removed the test that uses this file; you should be able to just delete
Why do we have to duplicate this?
The chrome_sync_shell_apk includes different JNI functions than chrome_apk, so we generate another header file for it called "chrome_sync_shell_jni_registration.h" and create a new entry_point file for it.
Patch Set #16, Line 66:
"//build/config/android:hide_all_but_jni",
Ah, that's kind of a pain; I hadn't thought of that. Yeah, it's easier not
Done
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 17:Commit-Queue +1
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 18:Commit-Queue +1
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 18:
(9 comments)
Patch Set #16, Line 187:
self._implicit_imports = []
Refactor this class in CL 538141
Ah, it'd be clearer if you based this CL on the other one, instead of including the changes here (though since the other one is in the CQ now, at this point it's easier to just wait for the other one to land and then rebase this on top)
Patch Set #16, Line 1388:
help='The comma-separated list of header files to 'Why was this removed?
File base/android/jni_generator/jni_generator.py:
Patch Set #18, Line 415:
GetRegisterNameGetRegistrationFunctionName ?
Patch Set #18, Line 417:
template = Template('register_native_${JAVA_NAME}')Could keep this as RegisterNative instead of lower case maybe? It's a weirdly formatted name either way because of the Java binary name but InitialCaps is more like our coding style.
Also for really trivial cases like this you can just do string concatenation instead of creating a Template :)
Patch Set #18, Line 420:
fully_qualified_class.replace('_', '_1').replace('/', '_')I meant you could share this logic itself between the generation of the registration function name and the other place in this file, instead of duplicating it..
Patch Set #18, Line 869:
crbugPatch Set #18, Line 877:
crbug
File base/android/jni_generator/jni_generator_helper.h:
Patch Set #16, Line 48:
inline void CheckException(JNIEnv* env) {
This function is also called by some manual registration functions such as
Hm, that's awkward :/
The VR SDK seems to use a really weird way to handle JNI registration - it shouldn't have a generated, edited header checked in. Can we put a TODO on this to remove it, and talk with the VR owners about how to fix registration in the VR SDK?
File chrome/browser/android/chrome_sync_shell_entry_point.cc:
The chrome_sync_shell_apk includes different JNI functions than chrome_apk,
Hrm, having to duplicate all this just to include a different header file seems a bit awkward :/
Maybe this could just get #ifdef'ed?
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 20:Commit-Queue +1
Yipeng Wang (Gerrit)
(7 comments)
Patch Set #16, Line 187:
self._implicit_imports = []
Ah, it'd be clearer if you based this CL on the other one, instead of inclu
Done
Patch Set #16, Line 1388:
option_parser.add_option('--includes',Why was this removed?
I change it to a default setting.
File base/android/jni_generator/jni_generator.py:
Patch Set #18, Line 415:
GetPkgAndClassNGetRegistrationFunctionName ?
Done
Patch Set #18, Line 417:
return fully_qualified_class.replace('_', '_1').replace('/', '_')
Could keep this as RegisterNative instead of lower case maybe? It's a weird
Done
Patch Set #18, Line 420:
"""Returns the register name with a given class."""
I meant you could share this logic itself between the generation of the reg
Done
Patch Set #18, Line 869:
://crcrbug.com
Done
crbug.com
Done
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 22:Commit-Queue +1
Eric Stevenson (Gerrit)
Eric Stevenson posted comments on this change.
Patch set 22:
Looking great! This definitely ended up being more involved than expected (as usual). Mostly just nits.
(8 comments)
File base/android/jni_generator/jni_generator.py:
Patch Set #20, Line 415:
GetPkgAndClassNamenit: two empty lines between top level function defs.
Patch Set #20, Line 419:
GetRegistrationFunctionNamenit: two empty lines between top level function defs.
File base/android/jni_generator/jni_generator_helper.h:
Patch Set #16, Line 48:
inline void CheckException(JNIEnv* env) {
Hm, that's awkward :/
There's an existing bug for this: crbug.com/664306. Maybe add a TODO mentioning the crbug and we can fix that in a follow up?
File base/android/jni_generator/jni_registration_generator.py:
import sys
import argparse
import stringnit: order these alphabetically.
Patch Set #20, Line 20:
GenerateJNIHeadernit: two empty lines between top level function defs.
super nit: dict() is a built-in function, use another name (registration_dict?).
Or, better yet might be to move dict to be internal to HeaderGenerator and then make HeaderGenerator.GetContent() return the fully formed header file string. Your call.
nit: move all of these back a space.
File chrome/browser/android/chrome_entry_point.cc:
Patch Set #22, Line 43:
OnJNIOnLoadRegisterJNII guess we can't really remove these yet because that would require removing all manual registration functions? Does that mean we're calling all registration functions twice for the time being? If so can we add a temporary early exit?
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 22:
(7 comments)
Patch Set #20, Line 415:
GetPkgAndClassNamenit: two empty lines between top level function defs.
Done
Patch Set #20, Line 419:
GetRegistrationFunctionNamenit: two empty lines between top level function defs.
import sys
import argparse
import stringnit: order these alphabetically.
Done
Patch Set #20, Line 20:
GenerateJNIHeadernit: two empty lines between top level function defs.
Done
super nit: dict() is a built-in function, use another name (registration_di
Done
nit: move all of these back a space.
Patch Set #22, Line 43:
OnJNIOnLoadRegisterJNII guess we can't really remove these yet because that would require removin
The old register functions (the ones created by previous jni_generator.py) now have empty bodies and always return true. We created a separate register function which sits at the same header file as the previous one but has a concrete body. This is the function called by our central header file. Since the previous ones are no-ops, we only register natives once.
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 23:Commit-Queue +1
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 24:Commit-Queue +1
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 24:
So in this version of the CL we've switched all but six targets in the entire tree to use lazy JNI function lookup instead of registration, by my count. The only remaining targets are:
1) chrome
2) chrome sync shell
3) cast
4) cronet
5) content shell
6) remoting client
It would probably be a benefit to cast, cronet and remoting to also switch to lazy JNI function lookup, as long as they never run in a configuration where they use the crazy linker (which I don't think is the case). I've previously spoken to the cronet team about this and I think they're looking into it, and we could also talk to cronet and remoting.
Content shell and the sync shell are just for testing I think; do they use the crazy linker ever? If not then they could be switched as well.
The only reason to have any target other than chrome itself using JNI registration is to provide test coverage for the JNI registration code. Now that we're autogenerating it, it's somewhat less likely that it will be wrong (though there might still be bugs in the generation logic). If we're okay with this CL removing the JNI registration code from almost all test binaries then I suspect we may as well go all the way and remove it from everywhere but the one target that actually requires it. :)
If we're going to do that then we should also just flip the default configuration, so that instead of all these separate places opting *out* of having JNI lookup symbols stripped, chrome just opts into it.
Other than that, and the few nits I put on specific lines, I'm pretty happy with how this looks :)
(5 comments)
File base/android/jni_generator/jni_generator.py:
Patch Set #24, Line 416:
GetPkgAndClassNamenit: call this GetBinaryClassName, since that's what the JNI spec calls it and it's why we're using this particular name mangling.
nit: probably 'RegisterNative_' since the binary name is likely to start with a lowercase character :)
Patch Set #24, Line 840:
"""Substitutes JAVA_CLASS and KMETHODS in the provided template."""This should probably mention NAMESPACE as well now.
Patch Set #24, Line 847:
namespace_template = Template('${NAMESPACE}::')This could just be a string concatenation instead of a template; more readable.
Patch Set #24, Line 698:
# Ensure that .pak files are built only once (build them in the defaultUpdate this comment to mention the JNI headers as this is unintuitive: they seem like part of the native code (and so should get generated twice) but they're actually generated from the java code and so generating them once is more correct :)
To view, visit change 527683. To unsubscribe, visit settings.
Andrew Grieve (Gerrit)
Andrew Grieve posted comments on this change.
Patch set 24:
Interesting point about changing the default opt-in/opt-out. I like the idea, but doing so will require updating some sub-repos, so I think we save it for a follow-up.
I think it makes sense to leave the explicit registration for content shell, since that's supposed to mirror Chrome as much as possible. Cronet, Cast, and Remoting are shipping products, so I think it also makes sense to change this for them in follow-ups.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 25:
(5 comments)
File base/android/jni_generator/jni_generator.py:
Patch Set #24, Line 416:
GetBinaryClassName
nit: call this GetBinaryClassName, since that's what the JNI spec calls it
Done
nit: probably 'RegisterNative_' since the binary name is likely to start wi
Done
Patch Set #24, Line 840:
"""Substitutes NAMESPACE, JAVA_CLASS and KMETHODS in the provided
This should probably mention NAMESPACE as well now.
Done
Patch Set #24, Line 847:
namespace_str = ''
This could just be a string concatenation instead of a template; more reada
Update this comment to mention the JNI headers as this is unintuitive: they
Done
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 25:Commit-Queue +1
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 25:
Patch Set 24:
Interesting point about changing the default opt-in/opt-out. I like the idea, but doing so will require updating some sub-repos, so I think we save it for a follow-up.
Sure.
I think it makes sense to leave the explicit registration for content shell, since that's supposed to mirror Chrome as much as possible.
Does content shell actually get tested on the waterfall?
Cronet, Cast, and Remoting are shipping products, so I think it also makes sense to change this for them in follow-ups.
Yeah, doing it separately is fine, I was just wondering as it'd make this CL smaller :)
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 25:Code-Review +1
OK, this version looks good! Thanks a lot for doing all this.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 26:Commit-Queue +1
John Budorick (Gerrit)
John Budorick posted comments on this change.
Patch set 26:Code-Review +1
Patch Set 16:
Patch Set 16:
Hi,
jbudorick please review build/android/gyp/write_build_config.py
build/android/ lgtm. Didn't look at rules.gni but don't mind doing so if you'd like.
yfriedman please review files under chrome/browser/android/
dpranke please review testing/test.gni
torne feel free to give any suggestions.Thanks,
Yipeng
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 26:
Misha, please review the code under components/cronet, and net/android/BUILD.gn
Scott, please review mojo/android/BUILD.gn
Luke, please review the code under chromecast/
Pawel, please review testing/test.gni
Thank all of you!
Yipeng
Yipeng Wang (Gerrit)
Sergey: please review the code under chrome/browser/media/android
Thanks!
Scott Violet (Gerrit)
Scott Violet posted comments on this change.
Patch set 26:
(1 comment)
Patch Set #26, Line 144:
= [ "//buiCould you please elaborate on why the - and then + are necessary here?
Yipeng Wang (Gerrit)
Yipeng Wang uploaded patch set #27 to this change.
Android JNI: Generate calls to RegisterNatives()
Generate registration functions with unique names(package+class). Create a new template to
generate a header file which calls all registration functions together.
This CL also switches the test targets from using explicit JNI registration (via RegisterNatives()),
to using implicit JNI registration (just export the symbols and let dalvik look them up lazily).
This switch simplifies things a great deal, as the only reason for using explicit registration is
to work around a deficiency in the crazy linker, which most test targets don't use.
Design doc: https://docs.google.com/document/d/1pYnceZMuxhpU9u3OAzWLYInV_nqtHKsBFROp927FDXM/edit?usp=sharing
Bug: 683256
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I31cbfe393c9088233f65f1249285d48ac5571f45
---
M android_webview/test/embedded_test_server/BUILD.gn
M base/android/jni_generator/SampleForTests_jni.golden
M base/android/jni_generator/jni_generator.py
M base/android/jni_generator/jni_generator_helper.h
M base/android/jni_generator/jni_generator_tests.py
A base/android/jni_generator/jni_registration_generator.py
M base/android/jni_generator/testInnerClassNatives.golden
M base/android/jni_generator/testInnerClassNativesBothInnerAndOuter.golden
M base/android/jni_generator/testInnerClassNativesMultiple.golden
D base/android/jni_generator/testMainDexFile.golden
M base/android/jni_generator/testMultipleJNIAdditionalImport.golden
M base/android/jni_generator/testNatives.golden
M base/android/jni_generator/testNativesLong.golden
D base/android/jni_generator/testNonMainDexFile.golden
M base/android/jni_generator/testSingleJNIAdditionalImport.golden
M build/android/gradle/generate_gradle.py
M build/android/gyp/write_build_config.py
M build/config/android/rules.gni
M chrome/android/BUILD.gn
M chrome/android/java_sources.gni
M chrome/browser/android/DEPS
M chrome/browser/android/chrome_entry_point.cc
A chrome/browser/android/chrome_sync_shell_entry_point.cc
M chrome/browser/media/android/remote/remote_media_player_bridge.cc
M chrome/browser/media/android/router/media_router_android_bridge.cc
M chromecast/BUILD.gn
M chromecast/android/BUILD.gn
M chromecast/app/android/cast_jni_loader.cc
M components/cronet/android/BUILD.gn
M components/cronet/android/cronet_library_loader.cc
M content/shell/android/BUILD.gn
M content/shell/android/linker_test_apk/chromium_linker_test_android.cc
M content/shell/android/shell_library_loader.cc
M mojo/android/BUILD.gn
M net/android/BUILD.gn
M remoting/android/BUILD.gn
M remoting/android/client_java_tmpl.gni
M remoting/client/jni/BUILD.gn
M remoting/client/jni/remoting_jni_onload.cc
M testing/test.gni
M ui/android/BUILD.gn
41 files changed, 630 insertions(+), 281 deletions(-)
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 27:
(1 comment)
Patch Set #26, Line 144:
= [ "//buiCould you please elaborate on why the - and then + are necessary here?
This switches the target from using explicit JNI registration (via RegisterNatives()), to using implicit JNI registration (just export the symbols and let dalvik look them up lazily).
This switch simplifies things a great deal, as the only reason for using explicit registration is to work around a deficiency in the crazy linker, which most targets don't use.
To view, visit change 527683. To unsubscribe, visit settings.
Andrew Grieve (Gerrit)
Andrew Grieve posted comments on this change.
Patch set 27:Code-Review +1
a few more nits
(4 comments)
File base/android/jni_generator/jni_generator.py:
nit: shorten this to:
return bool(re.search(re_maindex, contents))
File base/android/jni_generator/jni_registration_generator.py:
nit: two lines in between top-level methods
File build/config/android/rules.gni:
Patch Set #26, Line 344:
chrome_public_apkthis example is wrong. Should be:
target: The Apk target to generate registrations for.
output: Path to the generated .h file
exception_files: List of .java files that should be ignored when searching for native methods (optional)
nit: say: "This exists here rather than in chrome_sync_shell_test_apk...
Yaron Friedman (Gerrit)
Yaron Friedman posted comments on this change.
Patch set 27:Code-Review +1
lgtm - i assume there's a CL for internal repo as well?
Scott Violet (Gerrit)
Scott Violet posted comments on this change.
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 27:
(4 comments)
nit: shorten this to:
nit: two lines in between top-level methods
Patch Set #26, Line 344:
chrome_public_apkthis example is wrong. Should be:
nit: say: "This exists here rather than in chrome_sync_shell_test_apk...
Done
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 28:
Joe, please review the code under remoting/ for me. Thanks!
Misha Efimov (Gerrit)
Misha Efimov posted comments on this change.
Patch set 28:
This is very interesting, and I need to understand whether and how this will work for Cronet.
Please ping me if I don't reply by Tuesday.
(2 comments)
File components/cronet/android/BUILD.gn:
Patch Set #28, Line 29:
cronet_sample_apkWhat does target = ":cronet_sample_apk" mean?
It seems strange that library JNI registration depends on a sample app.Patch Set #28, Line 544:
android:hide_all_but_jni_onloadWhy do we need to change this config? I think Cronet currently does use RegisterJNI instead of exporting all JNI symbols.
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 28:
mef@, see also my earlier comments on the CL about cronet maybe just moving to exporting all the JNI symbols instead.
(2 comments)
Patch Set #28, Line 29:
cronet_sample_apkWhat does target = ":cronet_sample_apk" mean?
The generation relies on using an APK target to find all the java sources - cronet itself doesn't define an APK, so I guess this is the only one that depends on the right stuff?
Patch Set #28, Line 544:
android:hide_all_but_jni_onloadWhy do we need to change this config? I think Cronet currently does use Reg
To simplify this process we've moved all test targets to just exporting all JNI symbols, to avoid having to generate the registration code and adjust the dep tree to make it work.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 28:
(2 comments)
Patch Set #28, Line 29:
cronet_sample_apk
The generation relies on using an APK target to find all the java sources -
Agreed
Patch Set #28, Line 544:
android:hide_all_but_jni_onload
To simplify this process we've moved all test targets to just exporting all
Agreed, I also explained that in the commit message of this CL.
Luke Halliwell (Gerrit)
Paweł Hajdan Jr. (Gerrit)
Paweł Hajdan Jr. posted comments on this change.
Yipeng Wang (Gerrit)
Yipeng Wang removed Dirk Pranke from this change.
To view, visit change 527683. To unsubscribe, visit settings.
Joe Downing (Gerrit)
Joe Downing posted comments on this change.
Patch set 28:Code-Review +1
lgtm for remoting
(1 comment)
File remoting/client/jni/remoting_jni_onload.cc:
TODOs should include the username of the person who wrote the TODO (it need not be the same person who will address it). Can you add your alias here?
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 29:
(1 comment)
TODOs should include the username of the person who wrote the TODO (it need
Done
To view, visit change 527683. To unsubscribe, visit settings.
Yipeng Wang (Gerrit)
Sergey Ulanov (Gerrit)
Sergey Ulanov posted comments on this change.
Patch set 29:Code-Review +1
chrome/browser/media/android lgtm
Misha Efimov (Gerrit)
Misha Efimov posted comments on this change.
Patch set 29:
Thanks for explanations! I've verified locally that all cronet tests pass.
Do I understand correctly that exporting JNI methods from actual cronet library will be done in a separate CL?
(1 comment)
File components/cronet/android/BUILD.gn:
Patch Set #29, Line 545:
configIs it expected to be applied to libcronet_tests.so only and not to libcronet.so?
Richard Coles (Gerrit)
Richard Coles posted comments on this change.
Patch set 29:
Patch Set 29:
(1 comment)
Thanks for explanations! I've verified locally that all cronet tests pass.
Do I understand correctly that exporting JNI methods from actual cronet library will be done in a separate CL?
Yeah, this CL wasn't intending to change the behaviour for non-test code, which seems sensible. If you'd like to try out dropping registration in cronet entirely then we can help you test that out after this lands? Probably easest..
Yipeng Wang (Gerrit)
Yipeng Wang posted comments on this change.
Patch set 29:
(1 comment)
Patch Set #29, Line 545:
configIs it expected to be applied to libcronet_tests.so only and not to libcrone
Yes, we only try to apply the dynamic lookup to libcronet_tests.so for now.
Misha Efimov (Gerrit)
Misha Efimov posted comments on this change.
Patch set 29:Code-Review +1
Thanks again for your explanation!
Yipeng Wang (Gerrit)
Commit Bot (Gerrit)
Commit Bot merged this change.
Android JNI: Generate calls to RegisterNatives()
Generate registration functions with unique names(package+class). Create a new template to
generate a header file which calls all registration functions together.
This CL also switches the test targets from using explicit JNI registration (via RegisterNatives()),
to using implicit JNI registration (just export the symbols and let dalvik look them up lazily).
This switch simplifies things a great deal, as the only reason for using explicit registration is
to work around a deficiency in the crazy linker, which most test targets don't use.
Design doc: https://docs.google.com/document/d/1pYnceZMuxhpU9u3OAzWLYInV_nqtHKsBFROp927FDXM/edit?usp=sharing
Bug: 683256
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I31cbfe393c9088233f65f1249285d48ac5571f45
Reviewed-on: https://chromium-review.googlesource.com/527683
Reviewed-by: Sergey Ulanov <ser...@chromium.org>
Reviewed-by: Misha Efimov <m...@chromium.org>
Reviewed-by: Luke Halliwell <hall...@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajd...@chromium.org>
Reviewed-by: Joe Downing <joe...@chromium.org>
Reviewed-by: Andrew Grieve <agr...@chromium.org>
Reviewed-by: Yaron Friedman <yfri...@chromium.org>
Reviewed-by: Scott Violet <s...@chromium.org>
Reviewed-by: John Budorick <jbud...@chromium.org>
Reviewed-by: Richard Coles <to...@chromium.org>
Commit-Queue: Yipeng Wang <yip...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482615}
M remoting/client/jni/BUILD.gn
M remoting/client/jni/remoting_jni_onload.cc
M testing/test.gni
39 files changed, 627 insertions(+), 279 deletions(-)