Bug 51156 – Fix V8 JNI bindings

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[sw]

Change http://trac.webkit.org/changeset/72974 broke V8 JNI bindings which is used by Android DumpRenderTree files.

[sw]

Created attachment 76723 [details]
Fix V8 JNI binding.

Make sure short/long are correctly converted.

[Adam Barth]

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?

[sw]

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.

[Steve Block]

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.

[Ben Murdoch]

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.

[sw]

Created attachment 76790 [details]
Fix V8 JNI binding.

[Steve Block]

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

[Ben Murdoch]

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.

[sw]

Created attachment 76895 [details]
Fix V8 JNI binding.

[Steve Block]

Comment on attachment 76895 [details]
Fix V8 JNI binding.

Looks like you need to rebase

[sw]

Created attachment 76898 [details]
Fix V8 JNI binding.

[Steve Block]

Comment on attachment 76898 [details]
Fix V8 JNI binding.

r=me

[WebKit Commit Bot]

Comment on attachment 76898 [details]
Fix V8 JNI binding.

Clearing flags on attachment: 76898

Committed r74288: <http://trac.webkit.org/changeset/74288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76898 [details]:

fast/preloader/script.html bug 50879 (author: [e-mail])
fast/loader/recursive-before-unload-crash.html bug 50880 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.