[webkit-reviews] review granted: [Bug 130142] [iOS] Get code to compile on older iOS versions : [Attachment 226886] take3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Pratik Solanki <psolanki at apple.com>'s
request for review:
Bug 130142: [iOS] Get code to compile on older iOS versions
https://bugs.webkit.org/show_bug.cgi?id=130142

Attachment 226886: take3
https://bugs.webkit.org/attachment.cgi?id=226886&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226886&action=review


> Source/WebCore/WebCore.exp.in:3218
>  #if ENABLE(VIDEO) && PLATFORM(IOS)
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000

Please do not nest #if statements like this. Put the exports into a separate
section with this around the section:

#if ENABLE(VIDEO) && PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000


I think if you run the sort-export-file script it will do that for you, in
fact.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:45
> +    UNUSED_PARAM(mediaElement);

Why UNUSED_PARAM here?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:50
> +    return nullptr;

And no UNUSED_PARAM here?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:30
> -#if PLATFORM(IOS)
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000

Not sure we need the PLATFORM(IOS) here at all, although I guess in a header
file it’s nice to use it.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:29
> -#if PLATFORM(IOS)
> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000

Not sure we need the PLATFORM(IOS) here at all.

> Source/WebCore/platform/mac/HTMLConverter.mm:1570
> -#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000) ||
(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)

Not sure we need the PLATFORM(IOS) and PLATFORM(MAC) here at all. These get
really long and hard to read so it would be nice to know if we need it or not.

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:44
> +#if PLATFORM(WIN) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <=
1090) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 80000)

Ditto.

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:106
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000) ||
(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 10100)

Ditto.

> Source/WebCore/platform/text/ios/LocalizedDateCache.mm:159
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000)

Seems silly to check PLATFORM(IOS) in an iOS-specific source file.

> Source/WebCore/platform/text/mac/LocaleMac.mm:88
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000) ||
(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)

Again, can we omit PLATFORM(IOS) and PLATFORM(MAC)?

> Source/WebKit/mac/History/WebHistory.mm:157
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000) ||
(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090)

Ditto.