JSC and V8 versions of Java bridge should share JobjectWrapper
Created attachment 84722 [details] Patch
Attachment 84722 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8084626
Comment on attachment 84722 [details] Patch webkit-patch seems to have screwed up my patch, fixing ...
Created attachment 84723 [details] Patch 2
I think you forgot the Android makefiles :) >53: m_env->DeleteGlobalRef(m_instance); I know this is what the old code looked like, but should you not check if m_instance is non-null before calling DeleteGlobalRef()?
Comment on attachment 84723 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > + m_env->DeleteGlobalRef(m_instance); I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null.
Created attachment 84753 [details] Patch
(In reply to comment #6) > (From update of attachment 84723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > > > Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > > + m_env->DeleteGlobalRef(m_instance); > > I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes.
Created attachment 84754 [details] Patch 4
Attachment 84753 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8083730
Comment on attachment 84723 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review >>> Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 >>> + m_env->DeleteGlobalRef(m_instance); >> >> I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. > > But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes. Sure, but in that case we'll have crashed long ago because we don't check that at every call site that m_instance is non-null, so adding a check here doesn't add much.
(In reply to comment #11) > (From update of attachment 84723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84723&action=review > > >>> Source/WebCore/bridge/jni/JobjectWrapper.cpp:53 > >>> + m_env->DeleteGlobalRef(m_instance); > >> > >> I don't think there's any need to check this, as we assert that the reference passed to the constructor is non-null. > > > > But some JNI environments only allow a limited number of global references, so you may encounter a situation where NewGlobalRef() returns 0 even if 'instance' was non-null. Now, I am not sure what DeleteGlobalRef() does when you pass 0, but I think it crashes. > > Sure, but in that case we'll have crashed long ago because we don't check that at every call site that m_instance is non-null, so adding a check here doesn't add much. Ah yeah, you're right...well ok. LGTM.
Comment on attachment 84754 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84754&action=review > Source/WebCore/bridge/jni/JobjectWrapper.h:34 > +namespace JSC { Why in the JSC namespace if this is for V8 as well? > Source/WebCore/bridge/jni/JobjectWrapper.h:47 > + void ref() { m_refCount++; } There's no way to just use RefCounted? > Source/WebCore/bridge/jni/JobjectWrapper.h:50 > + if (!(--m_refCount)) Are the parenthesis really necessary?
Comment on attachment 84754 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84754&action=review >> Source/WebCore/bridge/jni/JobjectWrapper.h:34 >> +namespace JSC { > > Why in the JSC namespace if this is for V8 as well? All of the bridge code is in JSC::Bindings, I guess for legacy reasons >> Source/WebCore/bridge/jni/JobjectWrapper.h:47 >> + void ref() { m_refCount++; } > > There's no way to just use RefCounted? Hmm, I can look into this, but would prefer to keep this patch as just code movement. >> Source/WebCore/bridge/jni/JobjectWrapper.h:50 >> + if (!(--m_refCount)) > > Are the parenthesis really necessary? Will fix.
Comment on attachment 84754 [details] Patch 4 r=me please file a bug to keep a log of things that should be looked at after refactoring. I think the namespace issue should be one of them.
Filed Bug 55785 to look at the namespace issue and Bug 55786 to use RefCounted for JobjectWrapper. Will fix the parentheses when landing.
Committed r80367: <http://trac.webkit.org/changeset/80367>