Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7480016:
Add AutoJObject and AutoGlobalJObject (Closed)
|
Created:
9 years, 5 months ago by steveblock Modified:
9 years, 4 months ago CC:
chromium-reviews, brettw-cc_chromium.org Visibility:
Public. |
DescriptionAdd AutoJObject and AutoGlobalJObject
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95821
Patch Set 1 #Patch Set 2 : Updated base.gypi to exclude android directory for any non-Android OS #
Total comments: 4
Patch Set 3 : Fixed nitts #
Total comments: 11
Patch Set 4 : Removed FromLocalRef() #
Total comments: 3
Patch Set 5 : Uses templated ScopedJava(Global)Reference #Patch Set 6 : Adds use of static_cast #Patch Set 7 : Correct upload error #
Total comments: 12
Patch Set 8 : Fixed nits #
Messages
Total messages: 18 (0 generated)
Marcus, should these files be guarded with OS_ANDROID and/or excluded from the build for non-Android platforms?
Michael, Marcus mentioned that you're working on Android-specific code in base/. Is this patch in keeping with your plans? Do you have an opinion about how to guard it?
http://codereview.chromium.org/7480016/diff/3001/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/7480016/diff/3001/base/base.gypi#newcode25 base/base.gypi:25: 'android/auto_jobject.h', Above 4 lines might be in line 15.
http://codereview.chromium.org/7480016/diff/3001/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/7480016/diff/3001/base/base.gypi#newcode25 base/base.gypi:25: 'android/auto_jobject.h', On 2011/07/26 17:34:15, michaelbai wrote: > Above 4 lines might be in line 15. Sorry, you might correct.
(I'm not in the owners, but LGTM) just a couple of nits: http://codereview.chromium.org/7480016/diff/3001/base/android/auto_jobject.cc File base/android/auto_jobject.cc (right): http://codereview.chromium.org/7480016/diff/3001/base/android/auto_jobject.cc... base/android/auto_jobject.cc:41: obj_ = 0; nit: NULL http://codereview.chromium.org/7480016/diff/3001/base/android/auto_jobject.cc... base/android/auto_jobject.cc:47: nit: extra \n
http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... File base/android/auto_global_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:20: static AutoGlobalJObject FromLocalRef(JNIEnv* env, jobject obj); There's not a defined place for this in the header, but the typedefs, constructors & destructors should be first, so put this later. http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:23: AutoGlobalJObject(const AutoGlobalJObject& other); Do you need an implicit copy constructor? These are generally discouraged. http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:29: jobject obj() const; This should be inline. If it can't be, it should be named with CamelCase. http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h File base/android/auto_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h#... base/android/auto_jobject.h:13: // A helper class that automatically deletes the local reference to the jobject. Basically the same comments from the other class.
http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... File base/android/auto_global_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:20: static AutoGlobalJObject FromLocalRef(JNIEnv* env, jobject obj); On 2011/08/01 16:03:54, brettw wrote: > There's not a defined place for this in the header, but the typedefs, > constructors & destructors should be first, so put this later. Done. http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:23: AutoGlobalJObject(const AutoGlobalJObject& other); We don't need one for AutoGlobalJObject (other than for AutoGlobalJObject::FromLocalRef(), which can be removed), but we do need one for AutoJObject, as we want to be able to return objects of this type by value, such as from JavaObjectWeakGlobalRef::get() http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:29: jobject obj() const; On 2011/08/01 16:03:54, brettw wrote: > This should be inline. If it can't be, it should be named with CamelCase. Done. http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h File base/android/auto_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h#... base/android/auto_jobject.h:17: static AutoJObject FromLocalRef(JNIEnv* env, jobject obj); We could simplify the API by removing this method and making a new public constructor AutoJObject(JNIEnv, jobject). Marcus, Sami, do you have a preference?
reply inline: http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h File base/android/auto_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h#... base/android/auto_jobject.h:17: static AutoJObject FromLocalRef(JNIEnv* env, jobject obj); On 2011/08/02 15:05:46, Steve Block wrote: > We could simplify the API by removing this method and making a new public > constructor AutoJObject(JNIEnv, jobject). Marcus, Sami, do you have a > preference? no particular preference, happy to go with the simpler API (I think the original idea was to have named factory functions rather than different ctors, but I don't think we needed it in the end..)
http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h File base/android/auto_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_jobject.h#... base/android/auto_jobject.h:17: static AutoJObject FromLocalRef(JNIEnv* env, jobject obj); I'd be fine with having a new constructor for AutoJObject. Just make sure to document it well :)
Great, I'll upload a new patch without FromLocalRef(). I think this makes sense, as this method is the only reason for having a copy constructor for AutoGlobalJObject. We want to avoid this, so should remove FromLocalRef() from AutoJObject too for consistency.
http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... File base/android/auto_global_jobject.h (right): http://codereview.chromium.org/7480016/diff/8001/base/android/auto_global_job... base/android/auto_global_jobject.h:23: AutoGlobalJObject(const AutoGlobalJObject& other); If you're going to be returning these by value, it seems weird you wouldn't also support operator=. Otherwise users of this will likely get confused. http://codereview.chromium.org/7480016/diff/14002/base/android/auto_global_jo... base/android/auto_global_jobject.h:32: // Not permitted. This should be last. http://codereview.chromium.org/7480016/diff/14002/base/android/auto_jobject.h File base/android/auto_jobject.h (right): http://codereview.chromium.org/7480016/diff/14002/base/android/auto_jobject.h... base/android/auto_jobject.h:26: inline JNIEnv* env() const { return env_; } We wouldn't normally write "inline" here. C++ says all functions in headers will be inline anyway. http://codereview.chromium.org/7480016/diff/14002/base/android/auto_jobject.h... base/android/auto_jobject.h:33: AutoJObject(); // Not permitted. If you're returning by value, not permitting the default constructor also seems like a mistake since it permits things like this: AutoJObject foo; if (bar) { for = DoSomething(); } Basically, if you need your object to do any of these things (which is discouraged), it's usually best to have it do all of them to match user's expectations.
> we might consider use template in AutoJObject. Sounds reasonable. I'll rename to ScopedJava(Global)Reference and try templates.
LGTM (not an owner). just some nits / suggestions below: http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... File base/android/scoped_java_global_reference.h (right): http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:18: // to the lifetime of this object. perhaps we can also add a comment saying that this isn't thread-friendly as the jnienv can't cross thread boundaries / must be only used / destroyed in the thread that created it? http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:28: // reference is deleted when this object goes out of scope. the second sentence seems a bit redundant with the class comment on 17. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:48: } I'm not sure if we have a pattern for \n, but I'd add one here to separate the methods above. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... File base/android/scoped_java_reference.h (right): http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:26: // goes out of scope. same here. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:51: T obj() const { return obj_; } \n http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:61: remove \n
I doubt we should have copy constructor. From my point of view, the ScopedJavaReference just a convenient class which could avoid calling DeleteLocalRef() explicitly. Having copy constructor and release method might imply the embedded Java object will still available as long as we keep the reference. Actually, the JVM will release the object sometime later. Thought?
LGTM
> I doubt we should have copy constructor. Unfortunately, as mentioned, this is required to allow us to return ScopedJavaReference by value, such as from JavaObjectWeakGlobalRef::get(). > Having copy constructor and release method might imply the > embedded Java object will still available as long as we > keep the reference. I'm not sure what you mean. Can you elaborate? Given that we seem to be agreed on the basic approach for these classes, and that this patch has been in review for so long already, I'm going to commit it. Tao, I'm happy to work with you to remove the need for the ScopedJavaReference copy constructor. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... File base/android/scoped_java_global_reference.h (right): http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:18: // to the lifetime of this object. On 2011/08/05 12:03:02, bulach wrote: > perhaps we can also add a comment saying that this isn't thread-friendly as the > jnienv can't cross thread boundaries / must be only used / destroyed in the > thread that created it? Done. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:28: // reference is deleted when this object goes out of scope. On 2011/08/05 12:03:02, bulach wrote: > the second sentence seems a bit redundant with the class comment on 17. Done. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... base/android/scoped_java_global_reference.h:48: } On 2011/08/05 12:03:02, bulach wrote: > I'm not sure if we have a pattern for \n, but I'd add one here to separate the > methods above. Done. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... File base/android/scoped_java_reference.h (right): http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:26: // goes out of scope. On 2011/08/05 12:03:02, bulach wrote: > same here. Done. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:51: T obj() const { return obj_; } On 2011/08/05 12:03:02, bulach wrote: > \n Done. http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... base/android/scoped_java_reference.h:61: On 2011/08/05 12:03:02, bulach wrote: > remove \n Done.
I didn't want to get in your way of submitting it, please feel free to do it. The orignal design of AutoJObject and AutoGlobalJObject might have some issues. The calling DeleteLocalRef is optional, which means the JVM could gc it at anytime when we left the function or the block where the local reference was gotten. So, what's the point of returning a ScopedJavaReference? Why ScopedJavaGlobalReference just has the constructor which has java object itself as parameter instead of ScopedJavaReference since we have ScopedJavaReference::obj(). On 2011/08/08 11:28:32, Steve Block wrote: > > I doubt we should have copy constructor. > Unfortunately, as mentioned, this is required to allow us to return > ScopedJavaReference by value, such as from JavaObjectWeakGlobalRef::get(). > > > Having copy constructor and release method might imply the > > embedded Java object will still available as long as we > > keep the reference. > I'm not sure what you mean. Can you elaborate? > > Given that we seem to be agreed on the basic approach for these classes, and > that this patch has been in review for so long already, I'm going to commit it. > Tao, I'm happy to work with you to remove the need for the ScopedJavaReference > copy constructor. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... > File base/android/scoped_java_global_reference.h (right): > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... > base/android/scoped_java_global_reference.h:18: // to the lifetime of this > object. > On 2011/08/05 12:03:02, bulach wrote: > > perhaps we can also add a comment saying that this isn't thread-friendly as > the > > jnienv can't cross thread boundaries / must be only used / destroyed in the > > thread that created it? > > Done. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... > base/android/scoped_java_global_reference.h:28: // reference is deleted when > this object goes out of scope. > On 2011/08/05 12:03:02, bulach wrote: > > the second sentence seems a bit redundant with the class comment on 17. > > Done. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_gl... > base/android/scoped_java_global_reference.h:48: } > On 2011/08/05 12:03:02, bulach wrote: > > I'm not sure if we have a pattern for \n, but I'd add one here to separate the > > methods above. > > Done. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... > File base/android/scoped_java_reference.h (right): > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... > base/android/scoped_java_reference.h:26: // goes out of scope. > On 2011/08/05 12:03:02, bulach wrote: > > same here. > > Done. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... > base/android/scoped_java_reference.h:51: T obj() const { return obj_; } > On 2011/08/05 12:03:02, bulach wrote: > > \n > > Done. > > http://codereview.chromium.org/7480016/diff/13002/base/android/scoped_java_re... > base/android/scoped_java_reference.h:61: > On 2011/08/05 12:03:02, bulach wrote: > > remove \n > > Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
