Bug 38525

Summary: JavaInstanceJSC.cpp and JNIUtilityPrivate.cpp need to include jni_jsobject.h for jlong_to_pt() and ptr_to_jlong()
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking, ap, darin, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Attachments:
Description Flags
Patch
none
Patch none

Description Steve Block 2010-05-04 08:56:44 PDT 
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).
Comment 1 Steve Block 2010-05-04 10:04:11 PDT 
Created attachment 55024 [details]
Patch
Comment 2 Darin Adler 2010-05-04 10:10:34 PDT 
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).
Comment 3 Steve Block 2010-05-04 10:16:39 PDT 
> 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.
Comment 4 Darin Adler 2010-05-04 10:25:22 PDT 
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
Comment 5 Steve Block 2010-05-04 13:48:56 PDT 
I've filed Bug 38544 to track the renaming of MAC_JAVA_BRIDGE.
Comment 6 Steve Block 2010-05-04 13:54:14 PDT 
Created attachment 55043 [details]
Patch
Comment 7 Steve Block 2010-05-04 13:55:36 PDT 
> 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.
Comment 8 Steve Block 2010-05-04 14:17:27 PDT 
Builders look OK.

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

Closing bug as resolved fixed.