Bug 38525 – JavaInstanceJSC.cpp and JNIUtilityPrivate.cpp need to include jni_jsobject.h for jlong_to_pt() and ptr_to_jlong()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Change http://trac.webkit.org/changeset/55250 added code to JavaInstanceJSC.cpp and JNIUtilityPrivate.cpp which uses jlong_to_pt() and ptr_to_jlong(). These methods are provided in jni_jsobject.h but this header is not included directly. This causes build errors on Android.

Furthermore, jni_jsobject.h includes Mac-specific code which is not guarded by PLATFORM(MAC).

[Steve Block]

Created attachment 55024 [details]
Patch

[Darin Adler]

Comment on attachment 55024 [details]
Patch

> +        * bridge/jni/jni_jsobject.h: Guard Mac-specific code with PLATFORM(MAC)

Why are you making this change? The entire file is inside #if ENABLE(MAC_JAVA_BRIDGE).

[Steve Block]

> Why are you making this change? The entire file is inside #if
> ENABLE(MAC_JAVA_BRIDGE).
Android also defines MAC_JAVA_BRIDGE, which is used to protect all parts of the Java bridge, not just those parts specific to Mac. (The guard is probably poorly named. I can open a separate bug to fix that?) Without the additional PLATFORM(MAC) guard, the build fails on Android.

[Darin Adler]

Comment on attachment 55024 [details]
Patch

>  #define jlong_to_impptr(a) (static_cast<JSC::JSObject*>(((void*)(uintptr_t)(a))))
>  #define ptr_to_jlong(a) ((jlong)(uintptr_t)(a))
>  
> +#if PLATFORM(MAC)
> +
> +#include <CoreFoundation/CoreFoundation.h>

Normally in WebKit, conditional includes go up at the top of the file with the other includes, not down below things like macros. I also think this include is unneeded.

r=me

[Steve Block]

I've filed Bug 38544 to track the renaming of MAC_JAVA_BRIDGE.

[Steve Block]

Created attachment 55043 [details]
Patch

[Steve Block]

> I also think this include is unneeded.
I think you're right. I've uploaded a new patch without this include to check the build on the buildbots.

[Steve Block]

Builders look OK.

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

Closing bug as resolved fixed.