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

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

Summary: Remove BUILDING_ON / TARGETING macros in favor of system availability macros

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit Misc. (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Rowe (bdash)

URL:
Keywords:

Depends on:	91051
Blocks:
	Show dependency tree / graph

Reported:	2012-07-11 14:18 PDT by Mark Rowe (bdash)
Modified:	2012-07-11 22:56 PDT (History)
CC List:	17 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mark Rowe (bdash) 2012-07-11 14:18:12 PDT

As proposed on webkit-dev.

Comment 1 Mark Rowe (bdash) 2012-07-11 14:57:21 PDT

Created attachment 151789 [details]
Part 1 - Scripted replacement

Comment 2 Mark Rowe (bdash) 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

Comment 3 Mark Rowe (bdash) 2012-07-11 14:58:43 PDT

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

Comment 4 Tony Chang 2012-07-11 15:13:45 PDT

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

Comment 5 Filip Pizlo 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.

Comment 6 Mark Rowe (bdash) 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.

Comment 7 Early Warning System Bot 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

Comment 8 Build Bot 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

Comment 9 Mark Rowe (bdash) 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.

Comment 10 Mark Rowe (bdash) 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.

Comment 11 Early Warning System Bot 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

Comment 12 Mark Rowe (bdash) 2012-07-11 16:05:32 PDT

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

Comment 13 Early Warning System Bot 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

Comment 14 Early Warning System Bot 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

Comment 15 Mark Mentovai 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.

Comment 16 Mark Rowe (bdash) 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.

Comment 17 Mark Rowe (bdash) 2012-07-11 19:09:00 PDT

Landed in r122400 through r122402.

Comment 18 Takashi Sakamoto 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

Comment 19 Mark Rowe (bdash) 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.

Comment 20 Csaba Osztrogonác 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