Bug 130142 – [iOS] Get code to compile on older iOS versions

• Component: New Bugs
• Status: RESOLVED FIXED

[Pratik Solanki]

[iOS] Get code to compile on older iOS versions

[Pratik Solanki]

Created attachment 226534 [details]
Patch

[Pratik Solanki]

Created attachment 226786 [details]
Patch

[Pratik Solanki]

Created attachment 226886 [details]
take3

[Pratik Solanki]

<rdar://problem/16302908>

[Build Bot]

Comment on attachment 226886 [details]
take3

Attachment 226886 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5952395880169472

New failing tests:
mathml/wbr-in-mroot-crash.html

[Build Bot]

Created attachment 226891 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 226886 [details]
take3

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.

[Pratik Solanki]

Comment on attachment 226886 [details]
take3

View in context: https://bugs.webkit.org/attachment.cgi?id=226886&action=review

Thanks for the review. I'll commit after making the changes.

>> Source/WebCore/WebCore.exp.in:3218
>> +#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.

Will fix.

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

I was getting a compiler warning since the mediaElement argument was not used.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:50
>> +    return nullptr;
> 
> And no UNUSED_PARAM here?

There is no argument for this method. Am I missing something?

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:29
>> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000
> 
> Not sure we need the PLATFORM(IOS) here at all.

Removed it.

>> Source/WebCore/platform/mac/HTMLConverter.mm:1570
>> +#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.

We do need them. At least for iOS just having __MAC_OS_X_VERSION_MIN_REQUIRED will fail with an undefined macro warning. So we need PLATFORM(MAC) to guard against that. I think we would see similar failure for Mac if it ran into __IPHONE_OS_VERSION_MIN_REQUIRED. Hence the guard.

>> 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.

Removed it. The whole file is protected by PLATFORM(IOS) with a FIXME that says "Rename this file to LocalizedDataCacheIOS.mm and remove this guard." Guess we can do that in a separate bug.

[Pratik Solanki]

Comment on attachment 226886 [details]
take3

View in context: https://bugs.webkit.org/attachment.cgi?id=226886&action=review

>>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:29
>>> +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 80000
>> 
>> Not sure we need the PLATFORM(IOS) here at all.
> 
> Removed it.

Nope. Can't do this. Although the file is in iOS directory, it seems to be compiled on Mac as well.

[Pratik Solanki]

Committed r165944: <http://trac.webkit.org/changeset/165944>

[Babak Shafiei]

Roll out in r165953, broke the build.

[Pratik Solanki]

Can't use PLATFORM(IOS) in header files. :( Need to use TARGET_OS_IPHONE. I'll fix and land again.

[Pratik Solanki]

Committed r165979: <http://trac.webkit.org/changeset/165979>

[Pratik Solanki]

Follow on iOS build fix in <https://trac.webkit.org/r165992>.