[webkit-reviews] review denied: [Bug 107406] Collapse PLATFORM(MAC)||PLATFORM(IOS) to just PLATFORM(MAC) : [Attachment 183694] proposed change

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Laszlo Gombos
<laszlo.gombos at webkit.org>'s request for review:
Bug 107406: Collapse PLATFORM(MAC)||PLATFORM(IOS) to just PLATFORM(MAC)
https://bugs.webkit.org/show_bug.cgi?id=107406

Attachment 183694: proposed change
https://bugs.webkit.org/attachment.cgi?id=183694&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183694&action=review


r- to incorporate suggested changes outside of Platform.h.

> Source/JavaScriptCore/runtime/DatePrototype.cpp:65
> -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WX) && OS(DARWIN)) ||
(PLATFORM(QT) && OS(DARWIN))
> +#if PLATFORM(MAC) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) &&
OS(DARWIN))
>  #include <CoreFoundation/CoreFoundation.h>

This #if should probably be:  #if USE(CF)

However, this code would also now compile on the following platforms:
PLATFORM(WIN) && !OS(WINCE)
PLATFORM(CHROMIUM) && OS(DARWIN)

The Chromium-on-Mac macro doesn't matter since Chromium uses V8.
Not sure about !WinCE, though.

> Source/JavaScriptCore/runtime/DatePrototype.cpp:128
> -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WX) && OS(DARWIN)) ||
(PLATFORM(QT) && OS(DARWIN))
> +#if PLATFORM(MAC) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) &&
OS(DARWIN))

Ditto about switching to #if USE(CF) here.

> Source/WTF/wtf/Platform.h:473
> -#if PLATFORM(MAC) || PLATFORM(IOS)
> +#if PLATFORM(MAC)

I'm going to restate what's already been said, but we need to stop defining
PLATFORM(MAC) when building PLATFORM(IOS) (and PLATFORM(IOS_SIMULATOR)) to make
the decision explicit about whether a given piece of code supports both Mac and
iOS, and to make Mac-only code not require "PLATFORM(MAC) && !PLATFORM(IOS)". 
I'll file a follow-up bug.

The other thing I noticed about these uses of "PLATFORM(MAC) || PLATFORM(IOS)"
in Platform.h is that' they're all defining other ENABLE() or USE() macros,
which is exactly how they should be used since it makes it explicit about which
platforms are defining each macro.

To put it another way, using "PLATFORM(MAC) || PLATFORM(IOS)" in Platform.h is
good, but using it outside of Platform.h means we're probably missing an
ENABLE() or USE() macro.

> Source/WebCore/config.h:148
>  // CoreAnimation is available to IOS, Mac and Windows if using CG
> -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WIN) && USE(CG))
> +#if PLATFORM(MAC) || (PLATFORM(WIN) && USE(CG))
>  #define WTF_USE_CA 1
>  #endif

This exact code is already in Source/WTF/wtf/Platform.h, and <wtf/Platform.h>
is included in config.h above.	It should just be removed.