Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1123343005:
Improve jni_generator sample documentation. (Closed)
DescriptionImprove jni_generator sample documentation.
Add details of C++ registration, build file modifications.
Committed: https://crrev.com/d56b07e289ef5e9907587337108b6c8e5ff8c3c8
Cr-Commit-Position: refs/heads/master@{#330944}
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressed comments #
Total comments: 14
Patch Set 3 : cjhopman comments addressed #Patch Set 4 : merge TOT #
Messages
Total messages: 32 (9 generated)
scheib@chromium.org changed reviewers: + boliu@chromium.org
looks fine, it's just I don't actually own these files :p
boliu@chromium.org changed reviewers: + nyquist@chromium.org
On 2015/05/07 19:36:40, boliu wrote: > looks fine, it's just I don't actually own these files :p +nyquist
Thanks boliu@, I included you for initial review as Marcus mentioned you in https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/eBgBVu_1f0g
nyquist: PTAL
Thanks for doing this! I would appreciate a couple of extra clarifications while you're add it if you can spare the time. https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.cc (right): https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:10: #include "jni/SampleForTests_jni.h" // Generated, add based on Java class name. Could you also clarify that this file should only ever be included in the .cc file? https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.h:60: // And these two targets must be dependencies of modules using them. This is a little bit unclear to me (as a fake new reader). Could you clarify that the C++ target that has the .cc-file which includes the _jni.h file is the one that should depend on the _jni_headers target? And likewise just clarify that the _java target can be depended on by other Java targets like normal? https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.h:112: // Register C++ methods exposed to Java using JNI. What about Java methods that are marked with @CalledByNative? (i.e., the other direction)
Patchset #2 (id:20001) has been deleted
Thanks nyquist, PTAL. https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.cc (right): https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.cc:10: #include "jni/SampleForTests_jni.h" // Generated, add based on Java class name. On 2015/05/13 06:47:20, nyquist wrote: > Could you also clarify that this file should only ever be included in the .cc > file? Done. https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.h:60: // And these two targets must be dependencies of modules using them. On 2015/05/13 06:47:20, nyquist wrote: > This is a little bit unclear to me (as a fake new reader). > Could you clarify that the C++ target that has the .cc-file which includes the > _jni.h file is the one that should depend on the _jni_headers target? > And likewise just clarify that the _java target can be depended on by other Java > targets like normal? Done, I've included the deps added in the build file examples. I believe in GYP both targets should just be included by the C++ target, as they are both necessary for that target to function. I've a clean up patch to do this for bluetooth as well https://codereview.chromium.org/1127043006/ https://codereview.chromium.org/1123343005/diff/1/base/android/jni_generator/... base/android/jni_generator/sample_for_tests.h:112: // Register C++ methods exposed to Java using JNI. On 2015/05/13 06:47:20, nyquist wrote: > What about Java methods that are marked with @CalledByNative? (i.e., the other > direction) That doesn't require external C++ setup, it works given the generated headers. So, just include the header in the .cc and call the functions. I've added a comment to explain.
nyquist@chromium.org changed reviewers: + cjhopman@chromium.org
thanks for the updates. adding cjhopman@ in case he has comments to this. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:38: // 'base_jni_generator_sample_java', I don't think we should suggest depending on both _java and _jni_headers in the same target. I'd consider that discouraged. Could you instead have a _java target and a normal C++ target where the C++ target depends on the headers and the Java target depends on the sample java? The Java target might be very meta at that point though. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:84: // # ... normal sources, defines, deps. Mention that herein lies the one and only .cc file that in fact includes the header? (there can naturally be many such .cc files, but only one include per generated file).
https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:38: // 'base_jni_generator_sample_java', On 2015/05/14 00:31:13, nyquist wrote: > I don't think we should suggest depending on both _java and _jni_headers in the > same target. I'd consider that discouraged. > > Could you instead have a _java target and a normal C++ target where the C++ > target depends on the headers and the Java target depends on the sample java? > The Java target might be very meta at that point though. By demonstration, I have the device/bluetooth target that is a C++ target. There is no java target. For JNI bindings to work, the device/bluetooth target must depend on the _jni_headers and _java targets. perhaps it would be even simpler to have _jni_headers depend on _java, because there is a dependency there. https://codereview.chromium.org/1127043006/ demonstrates me cleaning it up, and it's easier to see there. When I remove the dep on the _java target the build breaks at runtime, the calls from C++ in the _jni_headers fail because the clazz isn't registered.
https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:38: // 'base_jni_generator_sample_java', On 2015/05/14 02:14:47, scheib wrote: > On 2015/05/14 00:31:13, nyquist wrote: > > I don't think we should suggest depending on both _java and _jni_headers in > the > > same target. I'd consider that discouraged. > > > > Could you instead have a _java target and a normal C++ target where the C++ > > target depends on the headers and the Java target depends on the sample java? > > The Java target might be very meta at that point though. > > By demonstration, I have the device/bluetooth target that is a C++ target. There > is no java target. For JNI bindings to work, the device/bluetooth target must > depend on the _jni_headers and _java targets. perhaps it would be even simpler > to have _jni_headers depend on _java, because there is a dependency there. > > https://codereview.chromium.org/1127043006/ demonstrates me cleaning it up, and > it's easier to see there. When I remove the dep on the _java target the build > breaks at runtime, the calls from C++ in the _jni_headers fail because the clazz > isn't registered. C++ target at compile time only need _jni_headers. At runtime, you need an APK that puts together both these targets. See chrome_shell_apk target: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_shel...
https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:38: // 'base_jni_generator_sample_java', On 2015/05/14 17:01:01, nyquist wrote: > On 2015/05/14 02:14:47, scheib wrote: > > On 2015/05/14 00:31:13, nyquist wrote: > > > I don't think we should suggest depending on both _java and _jni_headers in > > the > > > same target. I'd consider that discouraged. > > > > > > Could you instead have a _java target and a normal C++ target where the C++ > > > target depends on the headers and the Java target depends on the sample > java? > > > The Java target might be very meta at that point though. > > > > By demonstration, I have the device/bluetooth target that is a C++ target. > There > > is no java target. For JNI bindings to work, the device/bluetooth target must > > depend on the _jni_headers and _java targets. perhaps it would be even simpler > > to have _jni_headers depend on _java, because there is a dependency there. > > > > https://codereview.chromium.org/1127043006/ demonstrates me cleaning it up, > and > > it's easier to see there. When I remove the dep on the _java target the build > > breaks at runtime, the calls from C++ in the _jni_headers fail because the > clazz > > isn't registered. > > C++ target at compile time only need _jni_headers. > > At runtime, you need an APK that puts together both these targets. See > chrome_shell_apk target: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_shel... That seems consistent with "C++ target requires _jni_headers (or error at compile time) and _java targets (or error at runtime)", or simplifying: C++ target requires _jni_headers and _java. I'm pretty convinced at this point that the _java target should just be a dependency of the _jni_headers. Why try to avoid this and assume dependent targets will add the _java dependency? What benefit is there? I see only runtime failures and requiring multiple other targets to include a _java target.
On 2015/05/14 17:11:28, scheib wrote: > https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... > File base/android/jni_generator/sample_for_tests.h (right): > > https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... > base/android/jni_generator/sample_for_tests.h:38: // > 'base_jni_generator_sample_java', > On 2015/05/14 17:01:01, nyquist wrote: > > On 2015/05/14 02:14:47, scheib wrote: > > > On 2015/05/14 00:31:13, nyquist wrote: > > > > I don't think we should suggest depending on both _java and _jni_headers > in > > > the > > > > same target. I'd consider that discouraged. > > > > > > > > Could you instead have a _java target and a normal C++ target where the > C++ > > > > target depends on the headers and the Java target depends on the sample > > java? > > > > The Java target might be very meta at that point though. > > > > > > By demonstration, I have the device/bluetooth target that is a C++ target. > > There > > > is no java target. For JNI bindings to work, the device/bluetooth target > must > > > depend on the _jni_headers and _java targets. perhaps it would be even > simpler > > > to have _jni_headers depend on _java, because there is a dependency there. > > > > > > https://codereview.chromium.org/1127043006/ demonstrates me cleaning it up, > > and > > > it's easier to see there. When I remove the dep on the _java target the > build > > > breaks at runtime, the calls from C++ in the _jni_headers fail because the > > clazz > > > isn't registered. > > > > C++ target at compile time only need _jni_headers. > > > > At runtime, you need an APK that puts together both these targets. See > > chrome_shell_apk target: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_shel... > > That seems consistent with "C++ target requires _jni_headers (or error at > compile time) and _java targets (or error at runtime)", or simplifying: C++ > target requires _jni_headers and _java. > > I'm pretty convinced at this point that the _java target should just be a > dependency of the _jni_headers. Why try to avoid this and assume dependent > targets will add the _java dependency? What benefit is there? I see only runtime > failures and requiring multiple other targets to include a _java target. If more discussion is prudent here, I propose we do so in real time and summarize on code review.
ping
https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:32: // 'target_name': 'base_jni_generator_sample', add a 'type': 'static_library' (or shared/component) to make it clear that this is a native target https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:32: // 'target_name': 'base_jni_generator_sample', I'd probably do s/base_jni_generator/foo/ throughout these examples. I found it confusing that these referred to real files and the targets might not make sense in that case. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:98: // } add: android_library("base_jni_generator_sample_java") { java_files = [ "java/src/org/chromium/example/jni_generator/SampleForTests.java", "java/src/org/chromium/example/jni_generator/NonJniFile.java", ] } https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:122: // - Each module's RegisterModuleNameJni is then called by a larger module, I'd do s/is then called/must be called/ to make it clear that it is the larger module/application's responsibility to actually call it.
Patchset #3 (id:60001) has been deleted
PTAL https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... File base/android/jni_generator/sample_for_tests.h (right): https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:32: // 'target_name': 'base_jni_generator_sample', On 2015/05/20 02:28:27, cjhopman wrote: > I'd probably do s/base_jni_generator/foo/ throughout these examples. I found it > confusing that these referred to real files and the targets might not make sense > in that case. Done. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:32: // 'target_name': 'base_jni_generator_sample', On 2015/05/20 02:28:27, cjhopman wrote: > add a 'type': 'static_library' (or shared/component) to make it clear that this > is a native target Done. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:84: // # ... normal sources, defines, deps. On 2015/05/14 00:31:13, nyquist wrote: > Mention that herein lies the one and only .cc file that in fact includes the > header? (there can naturally be many such .cc files, but only one include per > generated file). Done. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:98: // } On 2015/05/20 02:28:27, cjhopman wrote: > add: > android_library("base_jni_generator_sample_java") { > java_files = [ > "java/src/org/chromium/example/jni_generator/SampleForTests.java", > "java/src/org/chromium/example/jni_generator/NonJniFile.java", > ] > } Done. https://codereview.chromium.org/1123343005/diff/40001/base/android/jni_genera... base/android/jni_generator/sample_for_tests.h:122: // - Each module's RegisterModuleNameJni is then called by a larger module, On 2015/05/20 02:28:27, cjhopman wrote: > I'd do s/is then called/must be called/ to make it clear that it is the larger > module/application's responsibility to actually call it. Done.
lgtm
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123343005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
nyquist, OWNERS stamp please.
lgtm
The CQ bit was checked by scheib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1123343005/#ps100001 (title: "merge TOT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123343005/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d56b07e289ef5e9907587337108b6c8e5ff8c3c8 Cr-Commit-Position: refs/heads/master@{#330944} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
