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> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andreip, commit-queue, jorlow, steveblock | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 57019, 57023 | ||||||||||
Attachments: |
|
Description
Steve Block
2011-03-24 08:51:58 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. Created attachment 86787 [details]
Patch
Created attachment 86916 [details]
Patch
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). > 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 Created attachment 86935 [details]
Patch
LGTM 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)
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: dumi@chromium.org) The commit-queue is continuing to process your patch. Comment on attachment 86935 [details] Patch Clearing flags on attachment: 86935 Committed r82194: <http://trac.webkit.org/changeset/82194> All reviewed patches have been landed. Closing bug. |