Bug 51156

Summary: Fix V8 JNI bindings
Product: WebKit Reporter: sw <swang>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benm, commit-queue, jorlow, steveblock, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Attachments:
Description Flags
Fix V8 JNI binding.
none
Fix V8 JNI binding.
none
Fix V8 JNI binding.
steveblock: review+
Fix V8 JNI binding. none

Description sw 2010-12-15 17:43:00 PST
Change http://trac.webkit.org/changeset/72974 broke V8 JNI bindings which is used by Android DumpRenderTree files.
Comment 1 sw 2010-12-15 18:12:58 PST
Created attachment 76723 [details]
Fix V8 JNI binding.

Make sure short/long are correctly converted.
Comment 2 Adam Barth 2010-12-15 19:44:26 PST
Comment on attachment 76723 [details]
Fix V8 JNI binding.

View in context: https://bugs.webkit.org/attachment.cgi?id=76723&action=review

> WebCore/ChangeLog:8
> +        Existing tests shall suffice to test.

Really?  What test does this patch fix?
Comment 3 sw 2010-12-15 22:12:10 PST
Thanks for review Adam.  You have a very good point.

Since I'm not that familiar with this part of code, wonder whether you have any suggestion how to test this change.

Thanks.
Comment 4 Steve Block 2010-12-16 03:27:24 PST
Simon, you mentioned to me that this change fixes a number of test on Android in fast/events/touch (and perhaps fast/dom/Geolocation too)? The reason being that numeric values to LayoutTestController functions were not being correctly passed through to Android's Java DRT. This may be a sufficient test in itself.

If you want to add a test specifically for these bindings, I think the correct place is in the java/ LayoutTests. There may already be a test case there, but we don't run these on Android.

As for the change itself, it looks correct to me, based on http://www.mozilla.org/js/liveconnect/lc3_method_overloading.html. This conversion is lacking in V8 JNI bridge, irrespective of http://trac.webkit.org/changeset/72974.
Comment 5 Ben Murdoch 2010-12-16 04:08:08 PST
Comment on attachment 76723 [details]
Fix V8 JNI binding.

View in context: https://bugs.webkit.org/attachment.cgi?id=76723&action=review

> WebCore/bridge/jni/v8/JNIUtilityPrivate.cpp:110
> +                result.s = static_cast<jshort>(NPVARIANT_TO_DOUBLE(value));

I think in addition to short and int types we need to do it for bytes too.
Comment 6 sw 2010-12-16 11:12:42 PST
Created attachment 76790 [details]
Fix V8 JNI binding.
Comment 7 Steve Block 2010-12-17 02:41:30 PST
> If you want to add a test specifically for these bindings, I think the correct
> place is in the java/ LayoutTests. There may already be a test case there, but
> we don't run these on Android.
Did you look into these tests? Such a test would exercise the V8+JNI case if run on Chrome.

The problem is that Android doesn't have an upstream bot. I'd be more comfortable r+ing if this if it were tested (or at least could be tested) by an upstream bot.
Comment 8 Ben Murdoch 2010-12-17 02:45:47 PST
Comment on attachment 76790 [details]
Fix V8 JNI binding.

View in context: https://bugs.webkit.org/attachment.cgi?id=76790&action=review

> WebCore/bridge/jni/v8/JNIUtilityPrivate.cpp:92
> +                result.b = static_cast<char>(NPVARIANT_TO_DOUBLE(value));

I see that we cast to char on the line above, but I think this is wrong and a good opportunity to clean it up - should be jbyte.
Comment 9 sw 2010-12-17 11:19:27 PST
Created attachment 76895 [details]
Fix V8 JNI binding.
Comment 10 Steve Block 2010-12-17 11:24:39 PST
Comment on attachment 76895 [details]
Fix V8 JNI binding.

Looks like you need to rebase
Comment 11 sw 2010-12-17 11:32:26 PST
Created attachment 76898 [details]
Fix V8 JNI binding.
Comment 12 Steve Block 2010-12-17 11:38:49 PST
Comment on attachment 76898 [details]
Fix V8 JNI binding.

r=me
Comment 13 WebKit Commit Bot 2010-12-17 13:24:55 PST
Comment on attachment 76898 [details]
Fix V8 JNI binding.

Clearing flags on attachment: 76898

Committed r74288: <http://trac.webkit.org/changeset/74288>
Comment 14 WebKit Commit Bot 2010-12-17 13:25:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Commit Bot 2010-12-17 14:21:01 PST
The commit-queue encountered the following flaky tests while processing attachment 76898 [details]:

fast/preloader/script.html bug 50879 (author: abarth@webkit.org)
fast/loader/recursive-before-unload-crash.html bug 50880 (authors: beidson@apple.com and eric@webkit.org)
The commit-queue is continuing to process your patch.