Bug 150466

Summary: Progress towards CMake on Mac
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, gyuyoung.kim, ossy, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch cdumez: review+

Description Alex Christensen 2015-10-22 13:12:07 PDT
Progress towards CMake on Mac
Comment 1 Alex Christensen 2015-10-22 13:14:59 PDT
Created attachment 263850 [details]
Patch
Comment 2 Csaba Osztrogon√°c 2015-10-22 13:57:09 PDT
Comment on attachment 263850 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        Add a space necessary for some files in WebKit1 compiled with -std=c99 with CMake.

Isn't c++11 compulsory for all ports? How come it builds now and why should we change for Mac-cmake build?
Comment 3 Alex Christensen 2015-10-22 14:02:00 PDT
Created attachment 263858 [details]
Patch
Comment 4 Alex Christensen 2015-10-22 14:04:52 PDT
(In reply to comment #2)
> Comment on attachment 263850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263850&action=review
> 
> > Source/WTF/ChangeLog:9
> > +        Add a space necessary for some files in WebKit1 compiled with -std=c99 with CMake.
> 
> Isn't c++11 compulsory for all ports? How come it builds now and why should
> we change for Mac-cmake build?
I don't know why it builds now, but I think this space won't hurt anything, and there are definitely some C and ObjC files in WebKit1 that don't use C++11.
Comment 5 Chris Dumez 2015-10-22 14:09:02 PDT
Comment on attachment 263858 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        Add a space necessary for some files in WebKit1 compiled with -std=c99 with CMake.

It seems odd to make the code uglier only for CMake build. If Xcode is able to build to existing code as is, I think we should get the CMake build to work with the existing code instead of updating the code.
Comment 6 Alex Christensen 2015-10-22 14:10:11 PDT
Created attachment 263861 [details]
Patch
Comment 7 Alex Christensen 2015-10-22 14:10:50 PDT
All right, I dropped the WTF changes.
Comment 8 Chris Dumez 2015-10-22 14:14:16 PDT
Comment on attachment 263861 [details]
Patch

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

r=me with comment.

> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:56
> +#import "WebMemoryPressureHandlerIOS.h"

Why is this needed? That header has already has #if PLATFORM(IOS).
Comment 9 Alex Christensen 2015-10-22 14:17:05 PDT
(In reply to comment #8)
> Comment on attachment 263861 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263861&action=review
> 
> r=me with comment.
> 
> > Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:56
> > +#import "WebMemoryPressureHandlerIOS.h"
> 
> Why is this needed? That header has already has #if PLATFORM(IOS).
It can't find  the header.  I don't want to add iOS include directories to the Mac CMake build.
Comment 10 Alex Christensen 2015-10-22 15:29:04 PDT
http://trac.webkit.org/changeset/191473
Comment 11 Gyuyoung Kim 2015-10-22 19:20:49 PDT
Comment on attachment 263861 [details]
Patch

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

> Source/WebKit2/CMakeLists.txt:776
> +    "${WEBCORE_DIR}/page"

I don't see why we need to add *page* include path again here, not 101 line. What problem ?
Comment 12 Gyuyoung Kim 2015-10-22 21:23:23 PDT
(In reply to comment #11)
> Comment on attachment 263861 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263861&action=review
> 
> > Source/WebKit2/CMakeLists.txt:776
> > +    "${WEBCORE_DIR}/page"
> 
> I don't see why we need to add *page* include path again here, not 101 line.
> What problem ?

I removed it in r191490 with build fix. If there is any problem, please let me know.
Comment 13 Alex Christensen 2015-10-23 11:32:39 PDT
(In reply to comment #11)
> Comment on attachment 263861 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263861&action=review
> 
> > Source/WebKit2/CMakeLists.txt:776
> > +    "${WEBCORE_DIR}/page"
> 
> I don't see why we need to add *page* include path again here, not 101 line.
> What problem ?
WebKit2/UIProcess/Cocoa/DiagnosticLoggingClient.h needs to be included instead of WebCore/page/DiagnosticLoggingClient.h.  I'll probably solve this by renaming one of the headers.
Comment 14 Csaba Osztrogon√°c 2015-10-26 04:19:15 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 263861 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=263861&action=review
> > 
> > > Source/WebKit2/CMakeLists.txt:776
> > > +    "${WEBCORE_DIR}/page"
> > 
> > I don't see why we need to add *page* include path again here, not 101 line.
> > What problem ?
> WebKit2/UIProcess/Cocoa/DiagnosticLoggingClient.h needs to be included
> instead of WebCore/page/DiagnosticLoggingClient.h.  I'll probably solve this
> by renaming one of the headers.

I prefer renaming one of this header to depending on include path order.