Bug 135528

Summary: progress towards cmake on mac
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: WebKit Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bunhere, clopez, cmarcelo, commit-queue, gyuyoung.kim, mrobinson, mrowe, oliver, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch gyuyoung.kim: review+

Description Alex Christensen 2014-08-01 16:02:03 PDT
Here's some more work to review.  I'm not sure if copying all the API headers to the derived sources directory is similar to anything else that is done or if there's a better place for them, but they need to be copied to somewhere to be accessed by #include <JavaScriptCore/APIHeader.h>
Comment 1 Alex Christensen 2014-08-01 16:07:19 PDT
Created attachment 235919 [details]
Patch
Comment 2 Martin Robinson 2014-08-01 16:08:35 PDT
Comment on attachment 235919 [details]
Patch

Where is APPLE defined or is it a CMake builtin?
Comment 3 Martin Robinson 2014-08-01 16:09:51 PDT
Comment on attachment 235919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235919&action=review

> Source/JavaScriptCore/CMakeLists.txt:596
> +if (APPLE)
> +    list(APPEND JavaScriptCore_SOURCES
> +        API/JSAPIWrapperObject.mm
> +        API/JSContext.mm
> +        API/JSManagedValue.mm
> +        API/JSStringRefCF.cpp
> +        API/JSValue.mm
> +        API/JSVirtualMachine.mm
> +        API/JSWrapperMap.mm
> +        API/ObjCCallbackFunction.mm
>      )

If these are specific to the Mac port they should probably be moved to the PlatformMac.cmake file.

> Source/WTF/wtf/CMakeLists.txt:269
> +if (APPLE)
> +    list(APPEND WTF_SOURCES
> +        text/cf/AtomicStringCF.cpp
> +        text/cf/StringCF.cpp
> +        text/cf/StringImplCF.cpp
> +        text/cf/StringViewCF.cpp
> +        text/mac/StringMac.mm
> +        text/mac/StringImplMac.mm
> +        text/mac/StringViewObjC.mm
> +        mac/MainThreadMac.mm
> +        mac/DeprecatedSymbolsUsedBySafari.mm
> +    )
> +endif ()

Ditto. Platform-specific source files should go in the Platform*.cmake files.
Comment 4 Mark Rowe (bdash) 2014-08-01 16:15:57 PDT
(In reply to comment #0)
> Here's some more work to review.  I'm not sure if copying all the API headers to the derived sources directory is similar to anything else that is done or if there's a better place for them, but they need to be copied to somewhere to be accessed by #include <JavaScriptCore/APIHeader.h>

Shouldn't the API headers be copied in to the framework wrapper and accessed from there?
Comment 5 Alex Christensen 2014-08-01 16:52:57 PDT
Created attachment 235924 [details]
Patch
Comment 6 Alex Christensen 2014-08-04 16:05:03 PDT
Created attachment 235988 [details]
Patch
Comment 7 Alex Christensen 2014-08-04 16:09:26 PDT
jsc compiles, links, and runs on Mac with this.  No FTL yet :(
I started into WebCore, but that's WIP.
Comment 8 Alex Christensen 2014-08-04 16:57:40 PDT
Created attachment 235990 [details]
Patch
Comment 9 Gyuyoung Kim 2014-08-04 17:18:50 PDT
Comment on attachment 235990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235990&action=review

As an initial patch for supporting CMake on mac, I think there is no critical issue except for minor nits.

BTW, don't you need to add PlatformMac.cmake for Source/WebKit2 dir ?

> Source/JavaScriptCore/CMakeLists.txt:17
> +    "${JAVASCRIPTCORE_DIR}/inspector/remote"

Wrong alphabet order.

> Source/JavaScriptCore/CMakeLists.txt:32
> +    "${DERIVED_SOURCES_DIR}/ForwardingHeaders"

ditto ?

> Source/JavaScriptCore/CMakeLists.txt:584
> +        disassembler/UDis86Disassembler.cpp

Isn't upper case placed to top side ?

> Source/JavaScriptCore/PlatformMac.cmake:6
> +    ${COCOA_LIBRARY}

ditto.

> Source/cmake/OptionsMac.cmake:32
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS3_TEXT_LINE_BREAK OFF)

ditto.

> Source/cmake/OptionsMac.cmake:41
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_DEVICE_ORIENTATION_iphoneos ON)

As far as I know, we have defined option values in Source/cmake/WebKitFeatures.cmake, then use it for each port. Can these mac port specific values be enabled when you build on mac ?
Comment 10 Alex Christensen 2014-08-04 17:31:05 PDT
Fixed some alphabetization and committed to http://trac.webkit.org/changeset/172014