Bug 91015 – Remove BUILDING_ON / TARGETING macros in favor of system availability macros

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Mark Rowe (bdash)]

As proposed on webkit-dev.

[Mark Rowe (bdash)]

Created attachment 151789 [details]
Part 1 - Scripted replacement

[Mark Rowe (bdash)]

Created attachment 151790 [details]
Part 2 - Fix the few cases that wanted to check what SDK we're building against

[Mark Rowe (bdash)]

Created attachment 151791 [details]
Part 3 - Remove the macros

[Tony Chang]

FWIW, I know close to nothing about Chromium Mac's build process.  Adding some people who do.

[Filip Pizlo]

Comment on attachment 151789 [details]
Part 1 - Scripted replacement

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

R=me

> Source/WebCore/platform/mac/ScrollElasticityController.mm:75
> -#if defined(BUILDING_ON_LEOPARD) || defined(BULDING_ON_SNOW_LEOPARD) || \
> -    defined(BUILDING_ON_LION) || PLATFORM(CHROMIUM)
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED == 1050 || defined(BULDING_ON_SNOW_LEOPARD) || \
> +    __MAC_OS_X_VERSION_MIN_REQUIRED == 1070 || PLATFORM(CHROMIUM)

Might be good to remove the misspelled form of SNOW_LEOPARD.  This could become __MAC_OS_X_VERSION_MIN_REQUIRED >= 1050

> Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:47
> -#define HAVE_MEDIA_CONTROL (!defined(BUILDING_ON_LEOPARD))
> +#define HAVE_MEDIA_CONTROL __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060

It feels like having parentheses around this would be good.

[Mark Rowe (bdash)]

(In reply to comment #5)
> (From update of attachment 151789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151789&action=review
> 
> R=me
> 
> > Source/WebCore/platform/mac/ScrollElasticityController.mm:75
> > -#if defined(BUILDING_ON_LEOPARD) || defined(BULDING_ON_SNOW_LEOPARD) || \
> > -    defined(BUILDING_ON_LION) || PLATFORM(CHROMIUM)
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED == 1050 || defined(BULDING_ON_SNOW_LEOPARD) || \
> > +    __MAC_OS_X_VERSION_MIN_REQUIRED == 1070 || PLATFORM(CHROMIUM)
> 
> Might be good to remove the misspelled form of SNOW_LEOPARD.  This could become __MAC_OS_X_VERSION_MIN_REQUIRED >= 1050

It'd actually be <= 1070.
> 
> > Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:47
> > -#define HAVE_MEDIA_CONTROL (!defined(BUILDING_ON_LEOPARD))
> > +#define HAVE_MEDIA_CONTROL __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
> 
> It feels like having parentheses around this would be good.

Will do.

[Early Warning System Bot]

Comment on attachment 151789 [details]
Part 1 - Scripted replacement

Attachment 151789 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13182574

[Build Bot]

Comment on attachment 151791 [details]
Part 3 - Remove the macros

Attachment 151791 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13200464

[Mark Rowe (bdash)]

Qt seems to build with -Wundef, so throws warnings when __MAC_OS_X_VERSION_MIN_REQUIRED is used on non-Mac platforms:

RenderLayerCompositor.cpp:1756:5: error: "__MAC_OS_X_VERSION_MIN_REQUIRED" is not defined

I think this test against __MAC_OS_X_VERSION_MIN_REQUIRED should first test PLATFORM(MAC):
-#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080
+#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080

If this isn't the only instance where this causes a problem then this could be solved in a more brute-force manner by doing something like the following at the end of Platform.h:

+#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
+#define __MAC_OS_X_VERSION_MIN_REQUIRED 0
+#endif
+
+#ifndef __MAC_OS_X_VERSION_MAX_ALLOWED
+#define __MAC_OS_X_VERSION_MAX_ALLOWED 0
+#endif


I'll upload a new patch with the first approach in a moment to see if it is sufficient to make Qt happy.

[Mark Rowe (bdash)]

(In reply to comment #9)
> Qt seems to build with -Wundef, so throws warnings when __MAC_OS_X_VERSION_MIN_REQUIRED is used on non-Mac platforms:
> 
> RenderLayerCompositor.cpp:1756:5: error: "__MAC_OS_X_VERSION_MIN_REQUIRED" is not defined
> 
> I think this test against __MAC_OS_X_VERSION_MIN_REQUIRED should first test PLATFORM(MAC):
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080

Actually, that'll want to be:

#if !PLATFORM(MAC) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080

Otherwise it'll disable the block for non-Mac platforms where it previously would have been enabled.

[Early Warning System Bot]

Comment on attachment 151789 [details]
Part 1 - Scripted replacement

Attachment 151789 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13198547

[Mark Rowe (bdash)]

Created attachment 151811 [details]
Part 1 - Scripted replacement (v2)

[Early Warning System Bot]

Comment on attachment 151811 [details]
Part 1 - Scripted replacement (v2)

Attachment 151811 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13202495

[Early Warning System Bot]

Comment on attachment 151811 [details]
Part 1 - Scripted replacement (v2)

Attachment 151811 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13202497

[Mark Mentovai]

Bottom line up front: Chromium is fine with this change. I posted my comments on the overall approach to the webkit-dev thread at http://lists.webkit.org/pipermail/webkit-dev/2012-July/021425.html.

[Mark Rowe (bdash)]

Created attachment 151828 [details]
Part 1 - Scripted replacement (v3)

One more shot at fixing Qt by adding some extra checks for PLATFORM(MAC) in OS version checks in cross-platform files.

[Mark Rowe (bdash)]

Landed in r122400 through r122402.

[Takashi Sakamoto]

Hello, 

I looked at src/third_party/WebKit/Source/WTF/wtf/Platform.h and I think,

BUILDING_ON_LEOPARD will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6".
BUILDING_ON_SNOW_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7".
BUILDING_ON_LION  will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_8".

And

TARGETING_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_6".
TARGETING_SNOW_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_7".
TARGETING_LION  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_8".

However, talking about HyphenationCF.cpp, 
"#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) " was replaced with "#if !PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070".

I think, __MAC_OS_X_VERSION_MIN_REQUIRED is not correct, should be  __MAC_OS_X_VERSION_MAX_ALLOWED.

Would you check the patch again?

Best regards,
Takashi Sakamoto

[Mark Rowe (bdash)]

(In reply to comment #18)
> Hello, 
> 
> I looked at src/third_party/WebKit/Source/WTF/wtf/Platform.h and I think,
> 
> BUILDING_ON_LEOPARD will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6".
> BUILDING_ON_SNOW_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7".
> BUILDING_ON_LION  will be replaced with "MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_8".
> 
> And
> 
> TARGETING_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_6".
> TARGETING_SNOW_LEOPARD  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_7".
> TARGETING_LION  will be replaced with "MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_8".
> 
> However, talking about HyphenationCF.cpp, 
> "#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) " was replaced with "#if !PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070".
> 
> I think, __MAC_OS_X_VERSION_MIN_REQUIRED is not correct, should be  __MAC_OS_X_VERSION_MAX_ALLOWED.
> 
> Would you check the patch again?
> 

Almost everywhere in WebKit that was using BUILDING_ON should have been using TARGETING instead. If you look at the patch you'll see that almost all instances of BUILDING_ON were replaced with comparisons against __MAC_OS_X_VERSION_MIN_REQUIRED in order to correct that.

[Csaba Osztrogonác]

It broke the Qt Mac build. I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=91051