Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10996063:
Android: adds Get(Static)MethodIDOrNULL. (Closed)
|
Created:
8 years, 2 months ago by bulach Modified:
8 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, erikwright+watch_chromium.org, browser-components-watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: adds Get(Static)MethodIDOrNULL.
When generating JNI bindings for system classes, we try to get the ids
for methods based in the .class file in the SDK.
However, this may trigger run-time errors when trying to run on an older
SDK.
For these system classes, the JNI generator will bind using the "OrNULL"
variation.
BUG=152987
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159769
Patch Set 1 #Patch Set 2 : OrNULL #
Total comments: 4
Patch Set 3 : Comments #
Total comments: 9
Patch Set 4 : Sami/Joth comments #
Total comments: 12
Patch Set 5 : Nyquist comments / joth's template #
Total comments: 2
Patch Set 6 : CHECK consistency #
Total comments: 4
Patch Set 7 : Removes spurious ClearException #
Messages
Total messages: 18 (0 generated)
http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.cc... base/android/jni_android.cc:181: ClearException(env); This causes some log spam which could be misleading if the caller is prepared to handle the missing method. Should we only log if this is called from GetMethodID? http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.h#... base/android/jni_android.h:75: jmethodID GetMethodIDOrNULL(JNIEnv* env, Nit: GetMethodIDOrNull would be more consistent with existing naming. Or: TryGetMethodID -- either way :)
thanks sami! comments addressed, another look please? http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.cc... base/android/jni_android.cc:181: ClearException(env); On 2012/10/01 11:32:15, Sami wrote: > This causes some log spam which could be misleading if the caller is prepared to > handle the missing method. Should we only log if this is called from > GetMethodID? good point! done.. http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/10996063/diff/2001/base/android/jni_android.h#... base/android/jni_android.h:75: jmethodID GetMethodIDOrNULL(JNIEnv* env, On 2012/10/01 11:32:15, Sami wrote: > Nit: GetMethodIDOrNull would be more consistent with existing naming. Or: > TryGetMethodID -- either way :) Done.
lgtm, thanks for refactoring. http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... File chrome/browser/history/android/sqlite_cursor.cc (right): http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... chrome/browser/history/android/sqlite_cursor.cc:19: using base::android::GetMethodIDOrNULL; Looks like NULL is still upper case here.
http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc... base/android/jni_android.cc:115: ExceptionCheck exception_check) { for bonus points these enums could be template args. http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc... base/android/jni_android.cc:122: (method_type == METHODTYPE_STATIC ? " static " : " ") << uber-nit: you don't need the space before 'static' or in the else clause. http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... File chrome/browser/history/android/sqlite_cursor.cc (right): http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... chrome/browser/history/android/sqlite_cursor.cc:66: if (!method_id) { can we comment why it's OK for this not to exist (i.e. we don't CHECK), yet we do log an error. THe class has existed since API level 1.0, so we shouldn't be hitting failures here.
http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc... base/android/jni_android.cc:115: ExceptionCheck exception_check) { On 2012/10/01 16:15:27, joth wrote: > for bonus points these enums could be template args. would that make any of this, or the caller code, clearer? :) http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc... base/android/jni_android.cc:122: (method_type == METHODTYPE_STATIC ? " static " : " ") << On 2012/10/01 16:15:27, joth wrote: > uber-nit: you don't need the space before 'static' or in the else clause. Done. http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... File chrome/browser/history/android/sqlite_cursor.cc (right): http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... chrome/browser/history/android/sqlite_cursor.cc:19: using base::android::GetMethodIDOrNULL; On 2012/10/01 15:46:00, Sami wrote: > Looks like NULL is still upper case here. Done. http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... chrome/browser/history/android/sqlite_cursor.cc:66: if (!method_id) { On 2012/10/01 16:15:27, joth wrote: > can we comment why it's OK for this not to exist (i.e. we don't CHECK), yet we > do log an error. > > THe class has existed since API level 1.0, so we shouldn't be hitting failures > here. ugh, good point.. I think it's one of those "just in case" checks for hand-written JNI.. using GetMethodID instead, but adding leandro to double check..
lgtm http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/3009/base/android/jni_android.cc... base/android/jni_android.cc:115: ExceptionCheck exception_check) { On 2012/10/01 17:25:00, bulach wrote: > On 2012/10/01 16:15:27, joth wrote: > > for bonus points these enums could be template args. > > would that make any of this, or the caller code, clearer? :) Possibly. It clearly communicates which params we expect (require) to be compile-time constant; at the moment it's vague and I need to read-around the code more to work out why we have this. (reading this My brain thought at first "why do we need 'is static' to be run-time definable? surely we always know for a given method if it's static or not, this can't change at runtime")
This also affects crbug/153024. See downstream: https://gerrit-int.chromium.org/#/c/26330/ http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:106: EXCEPTIIONCHECK_YES, Remove one I from the enum value from these two EXCEPTIIONCHECK_YES -> EXCEPTIONCHECK_YES EXCEPTIIONCHECK_NO -> EXCEPTIONCHECK_NO http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:117: env->GetMethodID(clazz, method_name, jni_signature) : These two lines should be swapped. If method_type == METHODTYPE_STATIC we should try to get the static method, not opposite... http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:120: CHECK(method_id && base::android::ClearException(env)) << Can we use || instead of && here? Otherwise we might be hiding an exception, and also, we would never get the exception printed if method_id is 0. http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:122: (method_type == METHODTYPE_STATIC ? "static" : "") << Add space after static: "static " : "" http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... File chrome/browser/history/android/sqlite_cursor.cc (left): http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... chrome/browser/history/android/sqlite_cursor.cc:65: if (!HasMethod(env, sclass, "<init>", "(I)V")) { See RHS of review http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... File chrome/browser/history/android/sqlite_cursor.cc (right): http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... chrome/browser/history/android/sqlite_cursor.cc:65: jmethodID method_id = GetMethodID(env, sclass, "<init>", "(I)V"); Could we do GetMethodIDOrNull(...) here, and then do add the old lines 66-67: LOG(ERROR) << "Can not find " << kSQLiteCursorClassPath << " Constructor"; return ScopedJavaLocalRef<jobject>(); That requires adding the JavaRef<jclass> version of base::android::GetMethodIDOrNull though. But at least we will then not change any behavior.
thanks tommy, joth! I addressed all your comments in the latest patch, ptal! http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:106: EXCEPTIIONCHECK_YES, On 2012/10/02 02:59:17, nyquist wrote: > Remove one I from the enum value from these two > EXCEPTIIONCHECK_YES -> EXCEPTIONCHECK_YES > EXCEPTIIONCHECK_NO -> EXCEPTIONCHECK_NO Done. http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:117: env->GetMethodID(clazz, method_name, jni_signature) : On 2012/10/02 02:59:17, nyquist wrote: > These two lines should be swapped. If method_type == METHODTYPE_STATIC we should > try to get the static method, not opposite... Done. http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:120: CHECK(method_id && base::android::ClearException(env)) << On 2012/10/02 02:59:17, nyquist wrote: > Can we use || instead of && here? Otherwise we might be hiding an exception, and > also, we would never get the exception printed if method_id is 0. Done. http://codereview.chromium.org/10996063/diff/12002/base/android/jni_android.c... base/android/jni_android.cc:122: (method_type == METHODTYPE_STATIC ? "static" : "") << On 2012/10/02 02:59:17, nyquist wrote: > Add space after static: "static " : "" Done. http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... File chrome/browser/history/android/sqlite_cursor.cc (left): http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... chrome/browser/history/android/sqlite_cursor.cc:65: if (!HasMethod(env, sclass, "<init>", "(I)V")) { On 2012/10/02 02:59:17, nyquist wrote: > See RHS of review Done. http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... File chrome/browser/history/android/sqlite_cursor.cc (right): http://codereview.chromium.org/10996063/diff/12002/chrome/browser/history/and... chrome/browser/history/android/sqlite_cursor.cc:65: jmethodID method_id = GetMethodID(env, sclass, "<init>", "(I)V"); On 2012/10/02 02:59:17, nyquist wrote: > Could we do GetMethodIDOrNull(...) here, and then do add the old lines 66-67: > > LOG(ERROR) << "Can not find " << kSQLiteCursorClassPath << " Constructor"; > return ScopedJavaLocalRef<jobject>(); > > That requires adding the JavaRef<jclass> version of > base::android::GetMethodIDOrNull though. But at least we will then not change > any behavior. joth points out that this constructor has always been available on all API levels.. I think this check was one of those "just in case" checks we sprinkled ages ago.. his comments: http://codereview.chromium.org/10996063/diff/3009/chrome/browser/history/andr... but tbh, since this patch no longer removes the HasMethod, I'll revert this file and then will address this later on.
lgtm % one consistency suggestion... http://codereview.chromium.org/10996063/diff/21001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/21001/base/android/jni_android.c... base/android/jni_android.cc:119: CHECK(method_id || base::android::ClearException(env)) << every where else in this file we have CHECK(some_id && !ClearException()) can we keep this consistent? to avoid short-circuit causing us to miss exception (even though method_id != NULL, which really should never happen) lets swap the order CHECK(!ClearException() && method_id) ... (ditto everywhere in the file)
thanks joth! addressed.. tommy: I'll put these through the trybots now, if you're happy with it, would you mind hitting CQ please? http://codereview.chromium.org/10996063/diff/21001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/21001/base/android/jni_android.c... base/android/jni_android.cc:119: CHECK(method_id || base::android::ClearException(env)) << On 2012/10/02 17:12:56, joth wrote: > every where else in this file we have > CHECK(some_id && !ClearException()) > > can we keep this consistent? > > to avoid short-circuit causing us to miss exception (even though method_id != > NULL, which really should never happen) lets swap the order > > CHECK(!ClearException() && method_id) ... (ditto everywhere in the file) sounds reasonable. done.
lgtm (one benign error, can fix in a follow up...) http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.c... base/android/jni_android.cc:256: bool error = ClearException(env); so we can remove this ClearException now... http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.c... base/android/jni_android.cc:283: bool error = ClearException(env); ditto
all done, thanks joth! http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.c... base/android/jni_android.cc:256: bool error = ClearException(env); On 2012/10/02 17:51:40, joth wrote: > so we can remove this ClearException now... Done. http://codereview.chromium.org/10996063/diff/21003/base/android/jni_android.c... base/android/jni_android.cc:283: bool error = ClearException(env); On 2012/10/02 17:51:40, joth wrote: > ditto Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/10996063/16003
Change committed as 159769 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
