Bug 202119 – Coalesce or remove PLATFORM(MAC) || PLATFORM(IOS_FAMILY)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Keith Rollin]

After refactoring and other code evolution, some platform checks have ended up looking like PLATFORM(MAC) || PLATFORM(IOS_FAMILY) (or vice-versa). These can be converted into the equivalent PLATFORM(COCOA). Where the instance occurs in a Cocoa-only file, the check can be removed altogether (along with any "#else" branches).

[Radar WebKit Bug Importer]

<rdar://problem/55638792>

[Keith Rollin]

Created attachment 379405 [details]
Patch

[Keith Rollin]

To anyone reviewing this, the changes in Tools/DumpRenderTree/AccessibilityUIElement.cpp were a little twisty, so please notice that they weren't as straightforward as most of the rest of the changes in this patch.

[WebKit Commit Bot]

Comment on attachment 379405 [details]
Patch

Clearing flags on attachment: 379405

Committed r250309: <https://trac.webkit.org/changeset/250309>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 379405 [details]
Patch

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

DIdn’t get a chance to look at this before you landed it, but spotted a couple small things.

> Source/WebCore/Modules/applepay/cocoa/PaymentContactCocoa.mm:61
>  static NSString *subLocality(CNPostalAddress *address)
>  {
> -#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)
>      return address.subLocality;
> -#else
> -    UNUSED_PARAM(address);
> -    return nil;
> -#endif
>  }
>  
>  static void setSubLocality(CNMutablePostalAddress *address, NSString *subLocality)
>  {
> -#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)
>      address.subLocality = subLocality;
> -#else
> -    UNUSED_PARAM(address);
> -    UNUSED_PARAM(subLocality);
> -#endif
>  }
>  
>  static NSString *subAdministrativeArea(CNPostalAddress *address)
>  {
> -#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)
>      return address.subAdministrativeArea;
> -#else
> -    UNUSED_PARAM(address);
> -    return nil;
> -#endif
>  }
>  
>  static void setSubAdministrativeArea(CNMutablePostalAddress *address, NSString *subAdministrativeArea)
>  {
> -#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)
>      address.subAdministrativeArea = subAdministrativeArea;
> -#else
> -    UNUSED_PARAM(address);
> -    UNUSED_PARAM(subAdministrativeArea);
> -#endif
>  }

Here we should go one step further and remove these functions and inline the code at the call site.

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:221
> -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> +#if PLATFORM(COCOA)

Should have removed this instead of changing it. These SPI headers are only for Cocoa platforms.

> Source/WebCore/PAL/pal/spi/cocoa/NEFilterSourceSPI.h:72
> -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> +#if PLATFORM(COCOA)

Ditto.

> Source/WebCore/PAL/pal/spi/mac/AVFoundationSPI.h:57
> -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> +#if PLATFORM(COCOA)

Ditto.

[Keith Rollin]

(In reply to Darin Adler from comment #6)
> Comment on attachment 379405 [details]
> Patch
> > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:221
> > -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> > +#if PLATFORM(COCOA)
> 
> Should have removed this instead of changing it. These SPI headers are only
> for Cocoa platforms.
> 
> > Source/WebCore/PAL/pal/spi/cocoa/NEFilterSourceSPI.h:72
> > -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> > +#if PLATFORM(COCOA)
> 
> Ditto.
> 
> > Source/WebCore/PAL/pal/spi/mac/AVFoundationSPI.h:57
> > -#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> > +#if PLATFORM(COCOA)
> 
> Ditto.

I'll double check. Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h, at least, has PLATFORM(WIN) conditionals. The other two look like good changes, though.

[Darin Adler]

Comment on attachment 379405 [details]
Patch

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

>>> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:221
>>> +#if PLATFORM(COCOA)
>> 
>> Should have removed this instead of changing it. These SPI headers are only for Cocoa platforms.
> 
> I'll double check. Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h, at least, has PLATFORM(WIN) conditionals. The other two look like good changes, though.

Good point about PLATFORM(WIN). But this part of CFNetworkSPI.h is inside "#if defined(__OBJC__)" so we don’t also need #if PLATFORM(COCOA).

[Keith Rollin]

(In reply to Darin Adler from comment #8)
> Comment on attachment 379405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379405&action=review
> 
> >>> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:221
> >>> +#if PLATFORM(COCOA)
> >> 
> >> Should have removed this instead of changing it. These SPI headers are only for Cocoa platforms.
> > 
> > I'll double check. Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h, at least, has PLATFORM(WIN) conditionals. The other two look like good changes, though.
> 
> Good point about PLATFORM(WIN). But this part of CFNetworkSPI.h is inside
> "#if defined(__OBJC__)" so we don’t also need #if PLATFORM(COCOA).

OK. Then I guess the PLATFORM(COCOA) is not needed in the following in the same file?

#if defined(__OBJC__) && PLATFORM(COCOA)
#import <CFNetwork/CFNSURLConnection.h>
#endif

Checking the history, I can see how we ended up with this check -- it's "devolved" from version-specific platform checks.

[Keith Rollin]

Committed r250522: <https://trac.webkit.org/changeset/250522>