Summary: | [iOS] Get code to compile on older iOS versions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||||||
Component: | New Bugs | Assignee: | Pratik Solanki <psolanki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bshafiei, buildbot, eric.carlson, glenn, jer.noble, philipj, psolanki, rniwa, sergio, webkit-ews | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Pratik Solanki
2014-03-12 11:06:51 PDT
Created attachment 226534 [details]
Patch
Created attachment 226786 [details]
Patch
Created attachment 226886 [details]
take3
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 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 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 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 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. Committed r165944: <http://trac.webkit.org/changeset/165944> Roll out in r165953, broke the build. Can't use PLATFORM(IOS) in header files. :( Need to use TARGET_OS_IPHONE. I'll fix and land again. Committed r165979: <http://trac.webkit.org/changeset/165979> Follow on iOS build fix in <https://trac.webkit.org/r165992>. |