Bug 147325 - Progress towards building AppleWin with CMake
Summary: Progress towards building AppleWin with CMake
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-27 11:04 PDT by Alex Christensen
Modified: 2015-07-27 15:41 PDT (History)
0 users

See Also:


Attachments
Patch (18.50 KB, patch)
2015-07-27 11:14 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2015-07-27 15:03 PDT, Alex Christensen
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-07-27 11:04:20 PDT
Almost there.  It would be nice to upstream this then polish of the last few quirks.
Comment 1 Alex Christensen 2015-07-27 11:14:01 PDT
Created attachment 257568 [details]
Patch
Comment 2 Martin Robinson 2015-07-27 14:53:52 PDT
Comment on attachment 257568 [details]
Patch

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

Looks pretty reasonable to me, but I have a question about the new platform-independent include path.

> Source/JavaScriptCore/CMakeLists.txt:36
>      "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector"
> +    "${CMAKE_BINARY_DIR}/Source/JavaScriptCore"

What files are generated here? It might be more appropriate to generate them in another directory or to move this include to the platform-specific CMake file. I ask because the GTK+ port uses Ninja and this include is not necessary for it.

> Source/WebCore/PlatformAppleWin.cmake:12
> +)
> +list(APPEND WebCore_SOURCES

Style nit. I think there is usually an empty line of whitespace between definitions.
Comment 3 Alex Christensen 2015-07-27 15:00:34 PDT
(In reply to comment #2)
> Comment on attachment 257568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257568&action=review
> 
> Looks pretty reasonable to me, but I have a question about the new
> platform-independent include path.
> 
> > Source/JavaScriptCore/CMakeLists.txt:36
> >      "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector"
> > +    "${CMAKE_BINARY_DIR}/Source/JavaScriptCore"
> 
> What files are generated here? It might be more appropriate to generate them
> in another directory or to move this include to the platform-specific CMake
> file. I ask because the GTK+ port uses Ninja and this include is not
> necessary for it.
It was something that's #included in LowLevelInterpreterWin.asm.  I should probably just generate that into a different directory somehow. r=you without this change and the style nit?
Comment 4 Alex Christensen 2015-07-27 15:03:45 PDT
Created attachment 257597 [details]
Patch
Comment 5 Martin Robinson 2015-07-27 15:36:19 PDT
Comment on attachment 257597 [details]
Patch

Thanks!
Comment 6 Alex Christensen 2015-07-27 15:41:21 PDT
http://trac.webkit.org/changeset/187458