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.
Created attachment 44314 [details] Patch 1 for Bug 32157
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
Created attachment 44317 [details] Patch 2 for Bug 32157 Fixed style
style-queue ran check-webkit-style on attachment 44317 [details] without any errors.
Created attachment 44321 [details] Patch 3 for Bug 32157
style-queue ran check-webkit-style on attachment 44321 [details] without any errors.
OMG, the style bot is out of control! :p
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.
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).
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.
Landed manually as http://trac.webkit.org/changeset/51756