|Summary:||Add a new JavaValue to type to represent a Java value in the Java bridge|
|Product:||WebKit||Reporter:||Steve Block <steveblock>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||andreip, commit-queue, jorlow, steveblock|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
|Bug Blocks:||57019, 57023|
Description Steve Block 2011-03-24 08:51:58 PDT
This type is required to allow us to make the JavaInstance API independent of JNI. See 57019.
Comment 1 Steve Block 2011-03-24 09:03:42 PDT
This bug will add the JavaType class and use it for the conversions to and from JavaNPObject that are used by the V8 Java bridge. Bug 57023 tracks using JavaType in the corresponding conversions for JSC.
Comment 4 Andrei Popescu 2011-03-25 06:02:05 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=86916&action=review > Source/WebCore/bridge/jni/JavaType.h:62 > + // special JavaTypeString which will be converted to a Java String when we I think you could make this comment a bit clearer by saying something like: "We cannot make the assumption that, at conversion time, the type to convert to is a JNI type. This is because there may be some mechanism other than JNI for reaching back to Java. Instead, we use..." > Source/WebCore/bridge/jni/v8/JNIUtilityPrivate.cpp:68 > + if (type == NPVariantType_String) should this be on the line above, next to the else? > Source/WebCore/bridge/jni/v8/JNIUtilityPrivate.cpp:298 > + result.l = getJNIEnv()->NewString(value.m_stringValue.characters(), value.m_stringValue.length()); Add a comment saying this is a local ref so it'll be released once the the executions returns back to Java from native? Also mention that probably this may leak if the call does not originate from java (e.g. from a worker, which may run its own native message loop).
Comment 5 Steve Block 2011-03-25 07:33:44 PDT
> I think you could make this comment a bit clearer by saying something like: Done > should this be on the line above, next to the else? No, there's an '#else' in between > Add a comment saying this is a local ref so it'll be released once the the executions returns back to Java from native? Done
Comment 7 Andrei Popescu 2011-03-25 07:53:56 PDT
Comment 8 Jeremy Orlow 2011-03-28 13:01:40 PDT
Comment on attachment 86935 [details] Patch rubber stamp since Andrei said it LGTM and style wise it seems fine (There's a good reason for the #ifs)
Comment 9 WebKit Commit Bot 2011-03-28 22:38:43 PDT
The commit-queue encountered the following flaky tests while processing attachment 86935 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: firstname.lastname@example.org) The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2011-03-28 22:41:24 PDT
Comment on attachment 86935 [details] Patch Clearing flags on attachment: 86935 Committed r82194: <http://trac.webkit.org/changeset/82194>
Comment 11 WebKit Commit Bot 2011-03-28 22:41:30 PDT
All reviewed patches have been landed. Closing bug.