Bug 57022

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:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

57019, 57023

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Steve Block

Reported 2011-03-24 08:51:58 PDT

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

Attachments
Patch (30.33 KB, patch) 2011-03-24 09:52 PDT, Steve Block	no flags	Details Formatted Diff Diff
Patch (23.25 KB, patch) 2011-03-25 04:42 PDT, Steve Block	no flags	Details Formatted Diff Diff
Patch (23.44 KB, patch) 2011-03-25 07:33 PDT, Steve Block	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Steve Block

Comment 1 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.

Steve Block

Comment 2 2011-03-24 09:52:41 PDT

Created attachment 86787 [details] Patch

Steve Block

Comment 3 2011-03-25 04:42:39 PDT

Created attachment 86916 [details] Patch

Andrei Popescu

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

Steve Block

Comment 5 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

Steve Block

Comment 6 2011-03-25 07:33:50 PDT

Created attachment 86935 [details] Patch

Andrei Popescu

Comment 7 2011-03-25 07:53:56 PDT

LGTM

Jeremy Orlow

Comment 8 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)

WebKit Commit Bot

Comment 9 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: dumi@chromium.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 10 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>

WebKit Commit Bot

Comment 11 2011-03-28 22:41:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.