WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
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+
Details
Formatted Diff
Diff
Part 3 - Remove the macros
(6.61 KB, patch)
2012-07-11 14:58 PDT
,
Mark Rowe (bdash)
andersca
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Part 1 - Scripted replacement (v2)
(227.00 KB, patch)
2012-07-11 16:05 PDT
,
Mark Rowe (bdash)
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
Part 1 - Scripted replacement (v3)
(227.23 KB, patch)
2012-07-11 18:11 PDT
,
Mark Rowe (bdash)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug