Improve jni_generator sample documentation.

base/android/jni_generator/sample_for_tests.h

 // Copyright (c) 2013 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.
 
 #ifndef BASE_ANDROID_JNI_GENERATOR_SAMPLE_FOR_TESTS_H_
 #define BASE_ANDROID_JNI_GENERATOR_SAMPLE_FOR_TESTS_H_
 
 #include <jni.h>
 #include <map>
 #include <string>
 
 #include "base/android/jni_android.h"
 #include "base/basictypes.h"
 
 namespace base {
 namespace android {
 
 // This file is used to:
 // - document the best practices and guidelines on JNI usage.
 // - ensure sample_for_tests_jni.h compiles and the functions declared in it
 // as expected.
 //
-// All methods are called directly from Java. See more documentation in
-// SampleForTests.java.
+// Methods are called directly from Java (except RegisterJNI). More
+// documentation in SampleForTests.java
+//
+// For C++ to access Java methods:
+// - GYP Build must be configured to generate bindings:
+//  # ...
+//  'targets': [
+//    {
+//      # An example target that will rely on JNI:
+//      'target_name': 'base_jni_generator_sample',

[cjhopman, 2015/05/20 02:28:27]

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.

[cjhopman, 2015/05/20 02:28:27]

add a 'type': 'static_library' (or shared/component) to make it clear that this
is a native target

[scheib, 2015/05/20 22:12:06]

Done.

[scheib, 2015/05/20 22:12:06]

Done.

+//      # ... normal sources, defines, deps.
+//      # Add deps for JNI:
+//      'conditions': [
+//        ['OS == "android"', {
+//          'dependencies': [
+//            'base_jni_generator_sample_java',

[nyquist, 2015/05/14 00:31:13]

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.

[scheib, 2015/05/14 02:14:47]

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.

[nyquist, 2015/05/14 17:01:01]

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...

[scheib, 2015/05/14 17:11:27]

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.

+//            'base_jni_generator_sample_jni_headers',
+//          ],
+//        }],
+//      ],
+//    },
+//  ],
+//  # ...
+//  # Create targets for JNI:
+//  'conditions': [
+//    ['OS == "android"', {
+//      'targets': [
+//        {
+//          'target_name': 'base_jni_generator_sample_jni_headers',
+//          'type': 'none',
+//          'sources': [
+//            'java/src/org/chromium/example/jni_generator/SampleForTests.java',
+//          ],
+//          'variables': {
+//            'jni_gen_package': 'base_jni_generator_sample',
+//          },
+//          'includes': [ '../../../build/jni_generator.gypi' ],
+//        },
+//        {
+//          'target_name': 'base_jni_generator_sample_java',
+//          'type': 'none',
+//          'dependencies': [
+//            '../../../base/base.gyp:base',
+//          ],
+//          'variables': {
+//            'java_in_dir': 'java',
+//          },
+//          'includes': [ '../../../build/java.gypi' ],
+//        },
+//      ],
+//    }],
+//  ],
+//
+// - GN Build must be configured to generate bindings:
+//  # Add import at top of file:
+//  if (is_android) {
+//    import("//build/config/android/rules.gni")  # For generate_jni().
+//  }
+//  # ...
+//  # An example target that will rely on JNI:
+//  component("base_jni_generator_sample") {
+//    # ... normal sources, defines, deps.

[nyquist, 2015/05/14 00:31:13]

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).

[scheib, 2015/05/20 22:12:06]

Done.

+//    # Add a dep for JNI:
+//    if (is_android) {
+//      deps += [ ":base_jni_generator_sample_jni" ]
+//    }
+//  }
+//  # ...
+//  # Create target for JNI:
+//  if (is_android) {
+//    generate_jni("base_jni_generator_sample_jni") {
+//      sources = [
+//        "java/src/org/chromium/example/jni_generator/SampleForTests.java",
+//      ]
+//      jni_package = "base_jni_generator_sample"
+//    }

[cjhopman, 2015/05/20 02:28:27]

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",
  ]
}

[scheib, 2015/05/20 22:12:06]

Done.

+//  }
+//
+// For C++ methods to be exposed to Java:
+// - The generated RegisterNativesImpl method must be called, this is typically
+//   done by having a static RegisterJNI method in the C++ class.
+// - The RegisterJNI method is added to a module's collection of register
+//   methods, such as: example_jni_registrar.h/cc files which call
+//   base::android::RegisterNativeMethods.
+//   An example_jni_registstrar.cc:
+//
+//     namespace {
+//     const base::android::RegistrationMethod kRegisteredMethods[] = {
+//         // Initial string is for debugging only.
+//         { "ExampleName", base::ExampleNameAndroid::RegisterJNI },
+//         { "ExampleName2", base::ExampleName2Android::RegisterJNI },
+//     };
+//     }  // namespace
+//
+//     bool RegisterModuleNameJni(JNIEnv* env) {
+//       return RegisterNativeMethods(env, kRegisteredMethods,
+//                                    arraysize(kRegisteredMethods));
+//     }
+//
+//  - Each module's RegisterModuleNameJni is then called by a larger module,

[cjhopman, 2015/05/20 02:28:27]

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.

[scheib, 2015/05/20 22:12:06]

Done.

+//    or application during startup.
+//
 class CPPClass {
  public:
   CPPClass();
   ~CPPClass();
 
+  // Register C++ methods exposed to Java using JNI.
+  static bool RegisterJNI(JNIEnv* env);
+
+  // Java @CalledByNative methods implicitly available to C++ via the _jni.h
+  // file included in the .cc file.
+
   class InnerClass {
    public:
     jdouble MethodOtherP0(JNIEnv* env, jobject obj);
   };
 
   void Destroy(JNIEnv* env, jobject obj);
 
   jint Method(JNIEnv* env, jobject obj);
 
   void AddStructB(JNIEnv* env, jobject obj, jobject structb);
 
   void IterateAndDoSomethingWithStructB(JNIEnv* env, jobject obj);
 
   base::android::ScopedJavaLocalRef<jstring> ReturnAString(
       JNIEnv* env, jobject obj);
 
  private:
   std::map<long, std::string> map_;
 
   DISALLOW_COPY_AND_ASSIGN(CPPClass);
 };
 
 }  // namespace android
 }  // namespace base
 
 #endif  // BASE_ANDROID_JNI_GENERATOR_SAMPLE_FOR_TESTS_H_