Bug 38525 - JavaInstanceJSC.cpp and JNIUtilityPrivate.cpp need to include jni_jsobject.h for jlong_to_pt() and ptr_to_jlong()
Summary: JavaInstanceJSC.cpp and JNIUtilityPrivate.cpp need to include jni_jsobject.h ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-04 08:56 PDT by Steve Block
Modified: 2010-05-04 14:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.48 KB, patch)
2010-05-04 10:04 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2010-05-04 13:54 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.