Bug 32157 – [Android] jni_utility includes two JSC-specific functions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

jni_utility mostly provides a number of script-engine-independent helper functions. In addition it provides two JSC-specific functions - convertValueToJValue and dispatchJNICall. This causes problems on Android, where we can build with JSC or V8. See Bug 32154, which this bug blocks.

It seems that jni_utility should provide only the script-engine-independent functions. The two JSC-specific functions it currently provides are only used within bridge/jni, so should be moved to a separate, private header. Perhaps jni_utility_private?

The V8-specific equivalents to these functions, which are required by Android, can then be added to this file with the necessary guards, or the file can be split into two. Perhaps [jsc|v8]/jni_utility_private? Note that code in other files in bridge/jni will also require separation into JSC and V8 variants. Given this, splitting the files into two directories seems the cleanest choice.

[Steve Block]

Created attachment 44314 [details]
Patch 1 for Bug 32157

[WebKit Review Bot]

Attachment 44314 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/jni/jsc/jni_utility_private.cpp:44:  Declaration has space between type name and * in JNIEnv *env  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:53:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:55:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:59:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:62:  Missing space after ,  [whitespace/comma] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:71:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:81:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:91:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:104:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:114:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:124:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:134:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:144:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:162:  _JNIType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:169:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:168:  Missing space before {  [whitespace/braces] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:174:  Missing space before {  [whitespace/braces] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:178:  Declaration has space between type name and * in JavaInstance *instance  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:182:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:185:  Declaration has space between type name and * in JavaArray *array  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:188:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:196:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:199:  Declaration has space between type name and * in JNIEnv *env  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:201:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:204:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:210:  Declaration has space between type name and * in JNIEnv *env  [whitespace/declaration] [3]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:211:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:214:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:215:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/jni_utility_private.cpp:264:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bridge/jni/jsc/jni_utility_private.h:35:  Missing space before {  [whitespace/braces] [5]
Total errors found: 31

[Steve Block]

Created attachment 44317 [details]
Patch 2 for Bug 32157

Fixed style

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44317 [details] without any errors.

[Steve Block]

Created attachment 44321 [details]
Patch 3 for Bug 32157

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44321 [details] without any errors.

[Eric Seidel (no email)]

OMG, the style bot is out of control! :p

[Adam Barth]

We can truncate the messages sooner, if that would be helpful, but it seems useful to give the full report in this case since Steve addressed all the issues.

[Adam Barth]

Comment on attachment 44321 [details]
Patch 3 for Bug 32157

I don't know much about JNI, but this patch appears formally correct (and has beautiful style).

[Eric Seidel (no email)]

The out of control comment was more about how it motivated steve to change a huge section of the file when a small change (with imperfect style) would have probably been easier to review.

[Steve Block]

Landed manually as http://trac.webkit.org/changeset/51756