Bug 51156

Created attachment 76723 [details] Fix V8 JNI binding. Make sure short/long are correctly converted.

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?

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.

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

Created attachment 76790 [details] Fix V8 JNI binding.

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

Created attachment 76895 [details] Fix V8 JNI binding.

Comment on attachment 76895 [details] Fix V8 JNI binding. Looks like you need to rebase

Created attachment 76898 [details] Fix V8 JNI binding.

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

Comment on attachment 76898 [details] Fix V8 JNI binding. Clearing flags on attachment: 76898 Committed r74288: <http://trac.webkit.org/changeset/74288>

All reviewed patches have been landed. Closing bug.

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.