Bug 130142 - [iOS] Get code to compile on older iOS versions
Summary: [iOS] Get code to compile on older iOS versions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-12 11:06 PDT by Pratik Solanki
Modified: 2014-03-20 13:31 PDT (History)
10 users (show)

See Also:


Attachments
Patch (15.39 KB, patch)
2014-03-12 11:09 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2014-03-14 17:57 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
take3 (15.45 KB, patch)
2014-03-16 23:05 PDT, Pratik Solanki
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (493.98 KB, application/zip)
2014-03-17 00:11 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-03-12 11:06:51 PDT
[iOS] Get code to compile on older iOS versions
Comment 1 Pratik Solanki 2014-03-12 11:09:20 PDT
Created attachment 226534 [details]
Patch
Comment 2 Pratik Solanki 2014-03-14 17:57:18 PDT
Created attachment 226786 [details]
Patch
Comment 3 Pratik Solanki 2014-03-16 23:05:37 PDT
Created attachment 226886 [details]
take3
Comment 4 Pratik Solanki 2014-03-16 23:06:10 PDT
<rdar://problem/16302908>
Comment 5 Build Bot 2014-03-17 00:11:45 PDT
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
Comment 6 Build Bot 2014-03-17 00:11:48 PDT
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
Comment 7 Darin Adler 2014-03-17 01:00:12 PDT
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.
Comment 8 Pratik Solanki 2014-03-19 16:43:14 PDT
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.
Comment 9 Pratik Solanki 2014-03-19 21:50:50 PDT
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.
Comment 10 Pratik Solanki 2014-03-19 22:11:25 PDT
Committed r165944: <http://trac.webkit.org/changeset/165944>
Comment 11 Babak Shafiei 2014-03-20 09:10:39 PDT
Roll out in r165953, broke the build.
Comment 12 Pratik Solanki 2014-03-20 11:06:12 PDT
Can't use PLATFORM(IOS) in header files. :( Need to use TARGET_OS_IPHONE. I'll fix and land again.
Comment 13 Pratik Solanki 2014-03-20 11:41:12 PDT
Committed r165979: <http://trac.webkit.org/changeset/165979>
Comment 14 Pratik Solanki 2014-03-20 13:31:54 PDT
Follow on iOS build fix in <https://trac.webkit.org/r165992>.