Bug 150466 – Progress towards CMake on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Progress towards CMake on Mac

[Alex Christensen]

Created attachment 263850 [details]
Patch

[Csaba Osztrogonác]

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?

[Alex Christensen]

Created attachment 263858 [details]
Patch

[Alex Christensen]

(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.

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 263861 [details]
Patch

[Alex Christensen]

All right, I dropped the WTF changes.

[Chris Dumez]

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).

[Alex Christensen]

(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.

[Alex Christensen]

http://trac.webkit.org/changeset/191473

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

(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.

[Alex Christensen]

(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.

[Csaba Osztrogonác]

(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.