Android's paths and message loop implementation with JNI

Android's paths and message loop implementation with JNI


BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95085

[michaelbai, 2011-08-01 19:29:37]

This is upstream, please help to review it

[brettw, 2011-08-01 21:43:54]

LGTM

http://codereview.chromium.org/7518032/diff/17/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/7518032/diff/17/base/android/jni_android.h#new...
base/android/jni_android.h:30: jobject GetApplicationContext();
Can you add a comment for this?

http://codereview.chromium.org/7518032/diff/17/base/android/jni_android.h#new...
base/android/jni_android.h:44: }  // namespace base
Blank line after this.

[M-A Ruel, 2011-11-11 13:09:07]

While reviewing http://codereview.chromium.org/8509019/, I saw a few issues and
Steve pointed me to this review.

This CL doesn't even touch a single test.

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.cc#...
base/android/jni_android.cc:19: if (!g_jvm)
Having this function silently fail is a recipe for hard-to-find bugs.

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.h#n...
base/android/jni_android.h:32: // Get the method ID for a method. Will clear the
pending Java
Documentation style is inconsistent;

(To me) Having a query method clear the pending java exception is surprising. I
wouldn't guess that at call site.

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_array.cc
File base/android/jni_array.cc (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_array.cc#ne...
base/android/jni_array.cc:41: reinterpret_cast<const uint8*>(v[i].data()),
v[i].length()));
We never align at +8;

AutoJObject byte_array = AutoJObject::FromLocalRef(
    env,
    ToJavaByteArray(env, reinterpret_cast<const uint8*>(v[i].data()),
v[i].length()));
or use a local variable, mostly for the reinterpreted pointer.

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_string.cc
File base/android/jni_string.cc (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_string.cc#n...
base/android/jni_string.cc:15: // JNI's GetStringUTFChars() returns strings in
Java-modified UTF8, so we
This seems relatively inefficient in a potentially hot function, why aren't you
using the base::android::ConvertJavaStringToUTF8() function?

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_string.h
File base/android/jni_string.h (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_string.h#ne...
base/android/jni_string.h:16: // Convert a Java string to UTF8. Returns a std
string.
Verb tense usage is inconsistent in every files.

http://codereview.chromium.org/7518032/diff/5001/base/android/path_utils.cc
File base/android/path_utils.cc (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/path_utils.cc#n...
base/android/path_utils.cc:20: env, base::android::GetApplicationContext()));
Why specify base::android:: when you are already in the namespace?

http://codereview.chromium.org/7518032/diff/5001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/7518032/diff/5001/base/message_loop.h#newcode119
base/message_loop.h:119: // Using the given base::MessagePumpForUIFactory to
override the default
Can you use an action sentence instead?

[M-A Ruel, 2011-11-11 13:12:42]

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/7518032/diff/5001/base/android/jni_android.h#n...
base/android/jni_android.h:36: const char* const method,
Making the pointer itself const for function parameters is frown upon in
Chromium coding style. It just add 5 dead characters with no information. Please
remove.