Summary: | Remove BUILDING_ON / TARGETING macros in favor of system availability macros | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Mark Rowe (bdash) <mrowe> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, avi, eric.carlson, eric, feature-media-reviews, gustavo, japhet, jochen, mark, menard, mifenton, mrobinson, ossy, pnormand, tasak, tony, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 91051 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2012-07-11 14:18:12 PDT
Created attachment 151789 [details]
Part 1 - Scripted replacement
Created attachment 151790 [details]
Part 2 - Fix the few cases that wanted to check what SDK we're building against
Created attachment 151791 [details]
Part 3 - Remove the macros
FWIW, I know close to nothing about Chromium Mac's build process. Adding some people who do. 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. (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. 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 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 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. (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. 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 Created attachment 151811 [details]
Part 1 - Scripted replacement (v2)
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 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 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. 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.
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 (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. It broke the Qt Mac build. I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=91051 |