It seems to me that OS(WINDOWS) and OS(UNIX) are so broadly defined that for each builds/ports exactly one of them is enabled. This opens up a door for re-factoring the fallback "#else" block to make the OS() guards always explicit in Platform.h (see /* FIXME: is this actually used or do other platforms generate their own config.h? */ ).
Created attachment 185101 [details] proposed refactoring I also flattened out the nested guards because I find it more readable. If others feel that this is not the case I am happy to keep the nesting as it is now.
Created attachment 185130 [details] rebaseline
Comment on attachment 185130 [details] rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=185130&action=review Unless I missed a HAVE_DISPATCH_H, this is gonna break iOS. > Source/WTF/wtf/Platform.h:725 > -#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 > - > +#if OS(DARWIN) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 > #define HAVE_DISPATCH_H 1 > #define HAVE_HOSTED_CORE_ANIMATION 1 This will break iOS. Do we still need __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 actually?
Created attachment 186709 [details] fix the issue pointed out in the review
(In reply to comment #3) > (From update of attachment 185130 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185130&action=review Thanks for the review. > Unless I missed a HAVE_DISPATCH_H, this is gonna break iOS. Great catch ! Fixed. > Do we still need __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 actually? Had the same question, so I filed bug 107964 a while back. I think the version work should be done in a separate patch.
Comment on attachment 186709 [details] fix the issue pointed out in the review View in context: https://bugs.webkit.org/attachment.cgi?id=186709&action=review I am not convinced you are making things simpler when splitting blocks. E.g.: #if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 Foo Bar #endif #if OS(DARWIN) && !OS(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 MacFoobar #endif Against: #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) Foo Bar #if !PLATFORM(IOS) MacFooBar #endf // !PLATFORM(IOS) #endif > Source/WTF/wtf/Platform.h:687 > #endif #endif // OS(UNIX) > Source/WTF/wtf/Platform.h:721 > +#endif #endif // OS(DARWIN) > Source/WTF/wtf/Platform.h:723 > +#if OS(IOS) || (OS(DARWIN) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) It is probably better to avoid OS(IOS) until Dave decided the fate of PLATFORM(MAC) and PLATFORM(IOS). This? #if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 or #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) Having DARWIN and __MAC_OS_X_VERSION_MIN_REQUIRED together is not adding readability because iOS is also a Darwin system. > Source/WTF/wtf/Platform.h:730 > +#if OS(DARWIN) && !OS(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 PLATFORM(IOS)
Created attachment 187457 [details] for cq
Comment on attachment 187457 [details] for cq Attachment 187457 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16478560
Comment on attachment 187457 [details] for cq Attachment 187457 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16469656
Comment on attachment 187457 [details] for cq Attachment 187457 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16479553
Created attachment 187458 [details] cq 2nd try
Comment on attachment 187458 [details] cq 2nd try Clearing flags on attachment: 187458 Committed r142399: <http://trac.webkit.org/changeset/142399>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > (From update of attachment 186709 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186709&action=review Thanks for the review Benjamin ! > It is probably better to avoid OS(IOS) until Dave decided the fate of PLATFORM(MAC) and PLATFORM(IOS). Ok, will do. For future discussions, I though we have consensus on preferring OS() macros over PLATFORM() macros in general. > This? > #if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 > or > #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) > > Having DARWIN and __MAC_OS_X_VERSION_MIN_REQUIRED together is not adding readability because iOS is also a Darwin system. (__MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) test needs either an OS(DARWIN) or a PLATFORM(MAC) guard around, otherwise the build fails on e.g. Qt Linux port because __MAC_OS_X_VERSION_MIN_REQUIRED is undefined.