Bug 108132

Summary: Refactor the way HAVE_XXX macros are set
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, cmarcelo, darin, ddkilzer, eric, ojan.autocc, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed refactoring
none
rebaseline
benjamin: review-
fix the issue pointed out in the review
benjamin: review+, benjamin: commit-queue-
for cq
webkit-ews: commit-queue-
cq 2nd try none

Description Laszlo Gombos 2013-01-28 16:36:27 PST
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? */ ).
Comment 1 Laszlo Gombos 2013-01-28 16:41:27 PST
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.
Comment 2 Laszlo Gombos 2013-01-28 18:37:19 PST
Created attachment 185130 [details]
rebaseline
Comment 3 Benjamin Poulain 2013-02-05 14:10:58 PST
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?
Comment 4 Laszlo Gombos 2013-02-05 14:36:51 PST
Created attachment 186709 [details]
fix the issue pointed out in the review
Comment 5 Laszlo Gombos 2013-02-05 14:39:53 PST
(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 6 Benjamin Poulain 2013-02-07 18:39:50 PST
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)
Comment 7 Laszlo Gombos 2013-02-09 19:46:00 PST
Created attachment 187457 [details]
for cq
Comment 8 Early Warning System Bot 2013-02-09 19:53:46 PST
Comment on attachment 187457 [details]
for cq

Attachment 187457 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16478560
Comment 9 Early Warning System Bot 2013-02-09 19:56:58 PST
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 10 EFL EWS Bot 2013-02-09 19:59:21 PST
Comment on attachment 187457 [details]
for cq

Attachment 187457 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16479553
Comment 11 Laszlo Gombos 2013-02-09 20:24:50 PST
Created attachment 187458 [details]
cq 2nd try
Comment 12 WebKit Review Bot 2013-02-10 06:15:00 PST
Comment on attachment 187458 [details]
cq 2nd try

Clearing flags on attachment: 187458

Committed r142399: <http://trac.webkit.org/changeset/142399>
Comment 13 WebKit Review Bot 2013-02-10 06:15:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Laszlo Gombos 2013-02-10 14:02:35 PST
(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.