Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8509019:
Add JavaBoundObject (Closed)
|
Created:
9 years, 1 month ago by Steve Block Modified:
9 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, dpranke-watch+content_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd JavaBoundObject
This represents a Java object for use in the Java bridge. It handles all of the
JNI required to interrogate the Java object and call methods on it.
Also adds some JNI utility methods to simpify the new code.
BUG=96703
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109645
Patch Set 1 #Patch Set 2 : Removed a superfluous string::reserve() #
Total comments: 5
Patch Set 3 : Moved new files to java/ #
Total comments: 11
Patch Set 4 : Fixed nits #
Total comments: 30
Patch Set 5 : Fixed style #
Total comments: 11
Patch Set 6 : Fixed style #Patch Set 7 : Use mutable #
Messages
Total messages: 18 (0 generated)
Ready for review
can we move all the java files under browser/renderer_host to browser/renderer_host/java? http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... File content/browser/renderer_host/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... content/browser/renderer_host/java_bound_object.cc:5: #if defined(ENABLE_JAVA_BRIDGE) ditto http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... File content/browser/renderer_host/java_bound_object.h (right): http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... content/browser/renderer_host/java_bound_object.h:8: #if defined(ENABLE_JAVA_BRIDGE) is this really needed? if ENABLE_JAVA_BRIDGE is not defined, then even if this file is included, no functions of it should be used http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... File content/browser/renderer_host/java_method.cc (right): http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... content/browser/renderer_host/java_method.cc:5: #if defined(ENABLE_JAVA_BRIDGE) ditto http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... File content/browser/renderer_host/java_method.h (right): http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... content/browser/renderer_host/java_method.h:8: #if defined(ENABLE_JAVA_BRIDGE) ditto
> can we move all the java files under browser/renderer_host > to browser/renderer_host/java? Sure. Is there any need to move the renderer files too? http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... File content/browser/renderer_host/java_bound_object.h (right): http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... content/browser/renderer_host/java_bound_object.h:8: #if defined(ENABLE_JAVA_BRIDGE) I guess it's not strictly required, because the gypi excludes these files when JAVA_BRIDGE is not defined. The reason I used a guard is that these files use JNI and Android-specific headers. There shouldn't be a problem, because Android should be the only platform to enable JAVA_BRIDGE, but is this a reasonable pattern?
On 2011/11/10 18:17:55, Steve Block wrote: > > can we move all the java files under browser/renderer_host > > to browser/renderer_host/java? > Sure. Is there any need to move the renderer files too? sure, for consistency > > http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... > File content/browser/renderer_host/java_bound_object.h (right): > > http://codereview.chromium.org/8509019/diff/3001/content/browser/renderer_hos... > content/browser/renderer_host/java_bound_object.h:8: #if > defined(ENABLE_JAVA_BRIDGE) > I guess it's not strictly required, because the gypi excludes these files when > JAVA_BRIDGE is not defined. > > The reason I used a guard is that these files use JNI and Android-specific > headers. There shouldn't be a problem, because Android should be the only > platform to enable JAVA_BRIDGE, but is this a reasonable pattern? sgtm
Moving all Java Bridge code to new java/ directory in http://codereview.chromium.org/8525016
Will rebase onto http://codereview.chromium.org/8525016 before landing
lgtm from owner, but i don't know any of this java code, so someone familiar with it should actually review the code before this is committed
Thanks John. Ben, Joth and Marcus, you guys reviewed reviewed this code locally before I uploaded it - care to comment?
a few (more) small nits.... but basically looks fine to me :) http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.cc... base/android/jni_android.cc:55: return GetMethodID(env, clazz.obj(), method, jni_signature); this caught me out as I thought you were calling the global c-like GetMethodID, not the one in this namespace (below), which should probably have been called GetMethodIDChecked or something. maybe comment that we call the other one as it handles the exception. http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.h#... base/android/jni_android.h:11: #include "base/android/scoped_java_ref.h" not needed http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:536: // object. is this comment suggesting that this class is created / used / destroyed on different threads? In that case, I'd suggest keeping the global ref java_object_ in a naked jobect rather than JavaGlobalRef, as the latter explicitly states it's only good for single threaded use. (will need a comment in the class to explain why it uses non-scoped jobject) Alternatively if this is only a delete issue we could post a message to delete it, but seems unfortunately to pull all that in just for this one case. http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:578: parameters[i] = CoerceJavaScriptValueToJavaValue(args[i], nit: if this is single thread and we keep the scoped jobject, we could pass java_object_.env() through here to avoid the calls to AttachCurrentThread() http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:145: JNIEnv* env = AttachCurrentThread(); again can use java_method_.env() rather than numerous AttachCurrentThread calls in here. http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:39: declare a d'tor and define it in the .cc
http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.cc... base/android/jni_android.cc:55: return GetMethodID(env, clazz.obj(), method, jni_signature); On 2011/11/11 11:40:38, joth wrote: > this caught me out as I thought you were calling the global c-like GetMethodID, > not the one in this namespace (below), which should probably have been called > GetMethodIDChecked or something. > > maybe comment that we call the other one as it handles the exception. Done. http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/10001/base/android/jni_android.h#... base/android/jni_android.h:11: #include "base/android/scoped_java_ref.h" On 2011/11/11 11:40:38, joth wrote: > not needed Done. http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:536: // object. Yes, currently objects of this class are created on one thread, then used and destroyed on another. I agree that using a naked jobject makes sense and will add a comment. http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:145: JNIEnv* env = AttachCurrentThread(); On 2011/11/11 11:40:38, joth wrote: > again can use java_method_.env() rather than numerous AttachCurrentThread calls > in here. Done. http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:39: On 2011/11/11 11:40:38, joth wrote: > declare a d'tor and define it in the .cc Done.
+ maruel for base/ approval
http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.cc... base/android/jni_android.cc:80: const char* const field, const char* field, making the pointer itself const has no impact. The coding style in chromium never uses that. I commented on the original CL but for now, don't do that for new functions. http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.h#... base/android/jni_android.h:35: // Get the method ID for a method, from the class name. Will clear the pending In general we use the imperative (?) tense for verbs, "Gets", "Returns", etc. This file is sadly inconsistent. Still use the right tense for new code. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:168: base::android::ConvertJavaStringToUTF8(env, java_string.obj()); See line 15. Please be coherent one way or another. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:489: static jmethodID g_object_get_class_id = NULL; Keep constants at the top of the file, otherwise it's hard to find them. This feels like a Singleton<> ? http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:500: static jmethodID g_class_get_methods_id = NULL; Same. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:562: // Take the first method with the correct number of arguments. What about function overloading with different types of arguments? http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.h (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:17: // Represents a Java object for use in the Java bridge. Holds a global ref to I'd add a first line summary: // Wrapper around jobject // To me, these 3 words are way more explanative that the whole paragraph. :) http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:44: // A global ref to the underlying Java object. We use a naked jobject, rather // Global ref .. 'A' doesn't convey information here. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:48: bool are_methods_set_up_; Isn't it redundant? I mean, is there a moment where are_methods_set_up_ != methods_.empty() ? http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:50: // A map containing the public methods of this Java object. We map from Similarly; // Public methods map from method name to Method instance. Multiple entries // will be present for overloaded methods. Note that ... http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:21: JavaType::Type BinaryNameToType(const std::string& binary_name) { Is this function hot? If so, a switch case on the first letter could be interesting. It's not worth if this function is not hot. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:81: static jmethodID g_method_get_parameter_types_id = NULL; Everywhere, move all globals to top of file. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:157: std::string JavaMethod::name() const { Functions like that can be inline. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:14: struct JavaType { Why define an empty struct? http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:31: }; Did you mean to have an instance of this enum in the struct? http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:34: // A wrapper around java.lang.reflect.Method. This class is not threadsafe. It java.lang.reflect.Method wrapper. This class must used on a single thread only. (Which implies it's not thread safe) http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:41: std::string name() const; can it be const std::string& ?
http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.cc File base/android/jni_android.cc (right): http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.cc... base/android/jni_android.cc:80: const char* const field, On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > const char* field, > > making the pointer itself const has no impact. The coding style in chromium > never uses that. > > I commented on the original CL but for now, don't do that for new functions. Done. http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/12003/base/android/jni_android.h#... base/android/jni_android.h:35: // Get the method ID for a method, from the class name. Will clear the pending On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > In general we use the imperative (?) tense for verbs, "Gets", "Returns", etc. > This file is sadly inconsistent. Still use the right tense for new code. Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:489: static jmethodID g_object_get_class_id = NULL; On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > Keep constants at the top of the file, otherwise it's hard to find them. > > This feels like a Singleton<> ? Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:562: // Take the first method with the correct number of arguments. We don't attempt to find the 'closest' match based on argument type. We always coerce the JavaScript value to the required type. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.h (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:17: // Represents a Java object for use in the Java bridge. Holds a global ref to On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > I'd add a first line summary: > > // Wrapper around jobject > // > > To me, these 3 words are way more explanative that the whole paragraph. :) Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:44: // A global ref to the underlying Java object. We use a naked jobject, rather On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > // Global ref .. > > 'A' doesn't convey information here. Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:48: bool are_methods_set_up_; I guess so, as I don't think it's possible to create a Java object with no methods. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.h:50: // A map containing the public methods of this Java object. We map from On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > Similarly; > > // Public methods map from method name to Method instance. Multiple entries > // will be present for overloaded methods. Note that ... Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:21: JavaType::Type BinaryNameToType(const std::string& binary_name) { No, it's not hot, as it's only used when setting up the methods for an injected object, not when actually calling them. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:157: std::string JavaMethod::name() const { On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > Functions like that can be inline. Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:14: struct JavaType { Switched to namespace http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:34: // A wrapper around java.lang.reflect.Method. This class is not threadsafe. It On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > java.lang.reflect.Method wrapper. This class must used on a single thread only. > > (Which implies it's not thread safe) Done. http://codereview.chromium.org/8509019/diff/12003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:41: std::string name() const; On 2011/11/11 13:31:13, Marc-Antoine Ruel wrote: > can it be const std::string& ? Done.
Only the const_cast<> bothers me, the rest is optional. http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h#... base/android/jni_android.h:61: // returns 0 if the field is not found. technically, isn't it NULL instead of 0? http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:77: JavaNPObject* obj = new JavaNPObject; optional: in general we always put (), e.g. new JavaNPObject(); http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:171: const_cast<JavaMethod*>(this)->EnsureNumParametersIsSetUp(); Why make the member function const if it isn't? I'd vote for removing the const_cast<>. http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:32: } } // namespace JavaType
http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h#... base/android/jni_android.h:61: // returns 0 if the field is not found. On 2011/11/11 16:22:28, Marc-Antoine Ruel wrote: > technically, isn't it NULL instead of 0? Done. http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_bound_object.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_bound_object.cc:77: JavaNPObject* obj = new JavaNPObject; On 2011/11/11 16:22:28, Marc-Antoine Ruel wrote: > optional: in general we always put (), e.g. new JavaNPObject(); Done. http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:171: const_cast<JavaMethod*>(this)->EnsureNumParametersIsSetUp(); I think that conceptually, the method is const - we just happen to do stuff lazily. Isn't there value in leaving it const to signal that? http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.h (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.h:32: } On 2011/11/11 16:22:28, Marc-Antoine Ruel wrote: > } // namespace JavaType Done.
lgtm http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h File base/android/jni_android.h (right): http://codereview.chromium.org/8509019/diff/14003/base/android/jni_android.h#... base/android/jni_android.h:61: // returns 0 if the field is not found. On 2011/11/11 16:31:34, Steve Block wrote: > On 2011/11/11 16:22:28, Marc-Antoine Ruel wrote: > > technically, isn't it NULL instead of 0? > > Done. You forgot to upload again? http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:171: const_cast<JavaMethod*>(this)->EnsureNumParametersIsSetUp(); On 2011/11/11 16:31:34, Steve Block wrote: > I think that conceptually, the method is const - we just happen to do stuff > lazily. Isn't there value in leaving it const to signal that? Then you would probably better to put your member a mutable. I prefer correctness over false advertisement. :) That said, I don't mind enough to block the CL.
http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... File content/browser/renderer_host/java/java_method.cc (right): http://codereview.chromium.org/8509019/diff/14003/content/browser/renderer_ho... content/browser/renderer_host/java/java_method.cc:171: const_cast<JavaMethod*>(this)->EnsureNumParametersIsSetUp(); The reason I preferred the const_cast to using mutable was that it made clear where the members need to be modified. But I guess you're right about false advertising. Changed |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
