RESOLVED FIXED 91015
Remove BUILDING_ON / TARGETING macros in favor of system availability macros
https://bugs.webkit.org/show_bug.cgi?id=91015
Summary Remove BUILDING_ON / TARGETING macros in favor of system availability macros
Mark Rowe (bdash)
Reported 2012-07-11 14:18:12 PDT
As proposed on webkit-dev.
Attachments
Part 1 - Scripted replacement (226.87 KB, patch)
2012-07-11 14:57 PDT, Mark Rowe (bdash)
fpizlo: review+
webkit-ews: commit-queue-
Part 2 - Fix the few cases that wanted to check what SDK we're building against (7.54 KB, patch)
2012-07-11 14:58 PDT, Mark Rowe (bdash)
fpizlo: review+
Part 3 - Remove the macros (6.61 KB, patch)
2012-07-11 14:58 PDT, Mark Rowe (bdash)
andersca: review+
buildbot: commit-queue-
Part 1 - Scripted replacement (v2) (227.00 KB, patch)
2012-07-11 16:05 PDT, Mark Rowe (bdash)
webkit-ews: commit-queue-
Part 1 - Scripted replacement (v3) (227.23 KB, patch)
2012-07-11 18:11 PDT, Mark Rowe (bdash)
no flags
Mark Rowe (bdash)
Comment 1 2012-07-11 14:57:21 PDT
Created attachment 151789 [details] Part 1 - Scripted replacement
Mark Rowe (bdash)
Comment 2 2012-07-11 14:58:17 PDT
Created attachment 151790 [details] Part 2 - Fix the few cases that wanted to check what SDK we're building against
Mark Rowe (bdash)
Comment 3 2012-07-11 14:58:43 PDT
Created attachment 151791 [details] Part 3 - Remove the macros
Tony Chang
Comment 4 2012-07-11 15:13:45 PDT
FWIW, I know close to nothing about Chromium Mac's build process. Adding some people who do.
Filip Pizlo
Comment 5 2012-07-11 15:17:21 PDT
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)
Comment 6 2012-07-11 15:26:56 PDT
(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 7 2012-07-11 15:51:23 PDT
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 8 2012-07-11 15:57:49 PDT
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)
Comment 9 2012-07-11 15:59:37 PDT
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)
Comment 10 2012-07-11 16:02:13 PDT
(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 11 2012-07-11 16:03:41 PDT
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)
Comment 12 2012-07-11 16:05:32 PDT
Created attachment 151811 [details] Part 1 - Scripted replacement (v2)
Early Warning System Bot
Comment 13 2012-07-11 17:37:41 PDT
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 14 2012-07-11 17:40:17 PDT
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
Comment 15 2012-07-11 17:50:27 PDT
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)
Comment 16 2012-07-11 18:11:26 PDT
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)
Comment 17 2012-07-11 19:09:00 PDT
Landed in r122400 through r122402.
Takashi Sakamoto
Comment 18 2012-07-11 20:50:06 PDT
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)
Comment 19 2012-07-11 20:56:06 PDT
(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
Comment 20 2012-07-11 22:56:53 PDT
It broke the Qt Mac build. I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=91051
Note You need to log in before you can comment on or make changes to this bug.