Android: lazy initialization for method id.

base/android/jni_method_id.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_ANDROID_JNI_METHOD_ID_ANDROID_H_
+#define BASE_ANDROID_JNI_METHOD_ID_ANDROID_H_
+
+#include <jni.h>
+#include <sys/types.h>
+
+#include "base/android/jni_android.h"
+#include "base/atomicops.h"
+#include "base/logging.h"
+
+namespace base {
+namespace android {
+
+// This class is a wrapper for JNIEnv Get(Static)MethodID.

[joth, 2012/10/03 17:31:52]

call out this isn't a class at all, just a namespace. (Why isn't it a namespace.
But given we're in double depth namespace already, we hardly need a third
layer...)

+// It's mostly used internally by the autogenerated JNI bindings.
+class MethodID {
+ public:
+  enum MethodType {
+    METHODTYPE_STATIC,
+    METHODTYPE_NORMAL,
+  };
+
+  // Wether or not to CHECK if an exception happens when
+  // trying to obtain a method id.
+  enum ExceptionCheck {
+    EXCEPTIONCHECK_NO,
+    EXCEPTIONCHECK_YES,
+  };
+
+  template<MethodType method_type,
+           ExceptionCheck exception_check>
+  static jmethodID Get(JNIEnv* env,
+                       jclass clazz,
+                       const char* method_name,
+                       const char* jni_signature) {
+    jmethodID id = method_type == METHODTYPE_STATIC ?
+        env->GetStaticMethodID(clazz, method_name, jni_signature) :
+        env->GetMethodID(clazz, method_name, jni_signature);
+    if (exception_check == EXCEPTIONCHECK_YES) {
+      CHECK(base::android::ClearException(env) || id) <<
+        "Failed to find " <<
+        (method_type == METHODTYPE_STATIC ? "static " : "") <<
+        "method " << method_name << " " << jni_signature;
+    } else if (base::android::HasException(env)) {
+      env->ExceptionClear();
+    }
+    return id;

[joth, 2012/10/03 17:31:52]

I'd still prefer these method bodies into a .cc file, and keep the header just
for function declarations (templated in class or otherwise -- that I don't care
so much on).

Splitting the stuff across multiple headers (jni_android.h vs jni_method_id.h)
is nice in theory, but doesn't give us much if in practice every user has to
include both anyway. Also as it stands it's a bit random that classes and fields
and one way of getting methods is in the former, but 2 other ways of getting
methods is in the latter. Cleaning that up is going to be a bundle of churn that
doesn't gain much now and just risks more breakages while we still have forks...
so on balance IMO it's easier to just keep the single header file at this point.

+  }
+
+  // The caller is responsible to zero-initialize |atomic_method_id|.
+  // If it's set, it'll return immediately. Otherwise, it'll call into
+  // ::Get() above. If there's a race, it's ok since the values are the same
+  // (and the duplicated effort will happen only once).
+  template<MethodType method_type,
+           ExceptionCheck exception_check>
+  static jmethodID LazyGet(JNIEnv* env,
+                           jclass clazz,
+                           const char* method_name,
+                           const char* jni_signature,
+                           base::subtle::AtomicWord* atomic_method_id) {
+    COMPILE_ASSERT(sizeof(subtle::AtomicWord) >= sizeof(jmethodID),
+                   AtomicWord_SmallerThan_jMethodID);
+    subtle::AtomicWord value = base::subtle::Acquire_Load(atomic_method_id);

[joth, 2012/10/03 17:31:52]

FWIW, I realized this doesn't need to be an 'acquire'. It can be straight
no-memory barrier load.
(It slightly increases the risk of race causing double init, but at trade off of
each subsequent call being a bit faster)

+    if (value)
+      return reinterpret_cast<jmethodID>(value);
+    jmethodID id = MethodID::Get<method_type, exception_check>(
+        env, clazz, method_name, jni_signature);
+    base::subtle::Acquire_Store(

[joth, 2012/10/03 17:31:52]

nit: C++ style is to align params with open paren where possible

+        atomic_method_id, reinterpret_cast<subtle::AtomicWord>(id));
+    return id;
+  }
+};

[joth, 2012/10/03 17:31:52]

if you leave as a class, DISALLOW_IMPLICIT_CTOR and hide the DTOR too.

+
+}  // namespace android
+}  // namespace base
+
+#endif  // BASE_ANDROID_JNI_METHOD_ID_ANDROID_H_