| Summary: | Progress towards CMake on Mac | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
| Component: | New Bugs | Assignee: | 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
Alex Christensen
2015-10-22 13:12:07 PDT
Created attachment 263850 [details]
Patch
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? Created attachment 263858 [details]
Patch
(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 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. Created attachment 263861 [details]
Patch
All right, I dropped the WTF changes. 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). (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 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 ? (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. (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. (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. |