Bug 57022 – Add a new JavaValue to type to represent a Java value in the Java bridge

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

This type is required to allow us to make the JavaInstance API independent of JNI. See 57019.

[Steve Block]

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.

[Steve Block]

Created attachment 86787 [details]
Patch

[Steve Block]

Created attachment 86916 [details]
Patch

[Andrei Popescu]

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).

[Steve Block]

> 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

[Steve Block]

Created attachment 86935 [details]
Patch

[Andrei Popescu]

LGTM

[Jeremy Orlow]

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)

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 86935 [details]
Patch

Clearing flags on attachment: 86935

Committed r82194: <http://trac.webkit.org/changeset/82194>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.