Android: lazy initialization for method id.

chrome/browser/history/android/sqlite_cursor.cc

 // 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.
 
 #include "chrome/browser/history/android/sqlite_cursor.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "chrome/browser/history/android/android_history_types.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/SQLiteCursor_jni.h"
 #include "sql/statement.h"
 
 using base::android::ConvertUTF8ToJavaString;
 using base::android::GetClass;
 using base::android::HasClass;
-using base::android::HasMethod;
-using base::android::GetMethodID;
+using base::android::MethodID;
 using base::android::ScopedJavaLocalRef;
 using content::BrowserThread;
 
 namespace {
 
 SQLiteCursor::JavaColumnType ToJavaColumnType(sql::ColType type) {
   switch (type) {
     case sql::COLUMN_TYPE_INTEGER:
       return SQLiteCursor::NUMERIC;
     case sql::COLUMN_TYPE_FLOAT:
@@ skipping 24 matching lines @@
     const std::vector<std::string>& column_names,
     history::AndroidStatement* statement,
     AndroidHistoryProviderService* service,
     FaviconService* favicon_service) {
   if (!HasClass(env, kSQLiteCursorClassPath)) {
     LOG(ERROR) << "Can not find " << kSQLiteCursorClassPath;
     return ScopedJavaLocalRef<jobject>();
   }
 
   ScopedJavaLocalRef<jclass> sclass = GetClass(env, kSQLiteCursorClassPath);
-  if (!HasMethod(env, sclass, "<init>", "(I)V")) {
+  jmethodID method_id = MethodID::Get<
+      MethodID::METHODTYPE_NORMAL, MethodID::EXCEPTIONCHECK_NO>(

[joth, 2012/10/04 17:59:59]

For the non-lazy API I'm ambivalent about switching to template API. I see the
consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant noise, and in
the odd place where it's important like here I completely missed the fact this
method may be chucking a NULL pointer back at us. (And I even was going to ask
whether the if(!method_id) below is spurious)

TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the difference
between _YES vs _NO in a param I trained myself to ignore.

[bulach, 2012/10/04 18:58:17]

well, a few steps back :)
aren't your eyes ignoring just because this patch is massive?

I mean, going forward, both reviewers and writers should:
- not write custom JNI in the first place! :)
(I do intend to get rid of these at some point..)

- if they do, they should be really careful.

I'd rather keep a single consistent API, otherwise we'll end up with that
multitude of methods with ambiguous documentation quite quickly... wdyt?

[nyquist, 2012/10/04 19:05:48]

So, I kind of agree with joth that it might be hard to remember that by passing
the EXCEPTIONCHECK_NO template argument, the method can return null. Maybe the
argument should be clearer?
For example, change it to something like:
ALLOW_NULL_RESULT / DISALLOW_NULL_RESULT ?
Or use any type of good English language tricks to clarify the meaning of the
enum value, and what implications it has?

+          env, sclass.obj(), "<init>", "(I)V");
+  if (!method_id) {
     LOG(ERROR) << "Can not find " << kSQLiteCursorClassPath << " Constructor";
     return ScopedJavaLocalRef<jobject>();
   }
 
-  jmethodID method_id = GetMethodID(env, sclass, "<init>", "(I)V");
   SQLiteCursor* cursor = new SQLiteCursor(column_names, statement, service,
                                           favicon_service);
   ScopedJavaLocalRef<jobject> obj(env,
       env->NewObject(sclass.obj(), method_id, reinterpret_cast<jint>(cursor)));
   if (obj.is_null()) {
     delete cursor;
     return ScopedJavaLocalRef<jobject>();
   }
   return obj;
 }
@@ skipping 193 matching lines @@
     return SQLiteCursor::BLOB;
 
   return ToJavaColumnType(statement_->statement()->ColumnType(column));
 }
 
 void SQLiteCursor::RunMoveStatementOnUIThread(int pos) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   service_->MoveStatement(statement_, position_, pos, &consumer_,
                 base::Bind(&SQLiteCursor::OnMoved, base::Unretained(this)));
 }