Bug 191875 – [Cocoa] Create a soft-linking file for PassKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Cocoa] Create a soft-linking file for PassKit

[Andy Estes]

Created attachment 355384 [details]
Patch

[Andy Estes]

Created attachment 355385 [details]
Patch

[Andy Estes]

Created attachment 355387 [details]
Patch

[Andy Estes]

Created attachment 355415 [details]
Patch

[Andy Estes]

Created attachment 355421 [details]
Patch

[Andy Estes]

Created attachment 355424 [details]
Patch

[Andy Estes]

Created attachment 355426 [details]
Patch

[Andy Estes]

Created attachment 355428 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46203215>

[Andy Estes]

Created attachment 355431 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 355431 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1058
> +#define USE_PASSKIT 1

I thought we were moving toward runtime switches for this kind of thing?

> Source/WTF/wtf/cocoa/SoftLinking.h:2
> - * Copyright (C) 2007, 2008, 2011-2015 Apple Inc. All rights reserved.
> + * Copyright (C) 2007-2018 Apple Inc. All rights reserved.

Is this correct? I thought our policy was to add the current year, instead of retroactively adding years.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:-41
> -#if PLATFORM(MAC)
> -SOFT_LINK_PRIVATE_FRAMEWORK(PassKit)
> -#else
> -SOFT_LINK_FRAMEWORK(PassKit)
> -#endif

This is good because the symbols are added to the global namespace, so they cause our unified build system problems. Moving them to headers is a better design.

> Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:591
> -            pkContactField = getPKContactFieldPhoneNumber();
> +            pkContactField = PAL::get_PassKit_PKContactFieldPhoneNumber();
>              break;
>  
>          case WebCore::PaymentError::ContactField::EmailAddress:
> -            pkContactField = getPKContactFieldEmailAddress();
> +            pkContactField = PAL::get_PassKit_PKContactFieldEmailAddress();
>              break;
>  
>          case WebCore::PaymentError::ContactField::Name:
> -            pkContactField = getPKContactFieldName();
> +            pkContactField = PAL::get_PassKit_PKContactFieldName();
>              break;
>  
>          case WebCore::PaymentError::ContactField::PhoneticName:
> -            pkContactField = getPKContactFieldPhoneticName();
> +            pkContactField = PAL::get_PassKit_PKContactFieldPhoneticName();
>              break;
>  
>          case WebCore::PaymentError::ContactField::PostalAddress:
> -            pkContactField = getPKContactFieldPostalAddress();
> +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
>              break;
>  
>          case WebCore::PaymentError::ContactField::AddressLines:
> -            pkContactField = getPKContactFieldPostalAddress();
> +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
>              postalAddressKey = getCNPostalAddressStreetKey();
>              break;
>  
>          case WebCore::PaymentError::ContactField::SubLocality:
> -            pkContactField = getPKContactFieldPostalAddress();
> +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();

Why change the name mangling strategy?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-690
> -		2D92A79821348D8500F493FD /* WebPaymentCoordinatorProxyMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AB1F77D1D1B30A9007C9BD1 /* WebPaymentCoordinatorProxyMac.mm */; };
> -		2D92A79F2134B07E00F493FD /* WebPaymentCoordinatorProxyIOS.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AB1F77B1D1B30A9007C9BD1 /* WebPaymentCoordinatorProxyIOS.mm */; };

Does this make the files disappear from the Xcode ui? I hope not.

[Andy Estes]

(In reply to Myles C. Maxfield from comment #11)
> Comment on attachment 355431 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355431&action=review
> 
> > Source/WTF/wtf/Platform.h:1058
> > +#define USE_PASSKIT 1
> 
> I thought we were moving toward runtime switches for this kind of thing?

I'm not sure I know what kind of thing you're asking about.

I added this so I can know at compile time whether we want to use PassKit. It's a more descriptive version of (PLATFORM(MAC) && PLATFORM(IOS)). There are already runtime switches for the features that use PassKit (applePayEnabled and paymentRequestEnabled).

Does that answer your question?

> 
> > Source/WTF/wtf/cocoa/SoftLinking.h:2
> > - * Copyright (C) 2007, 2008, 2011-2015 Apple Inc. All rights reserved.
> > + * Copyright (C) 2007-2018 Apple Inc. All rights reserved.
> 
> Is this correct? I thought our policy was to add the current year, instead
> of retroactively adding years.

I believe it's now ok to list date ranges.

> 
> > Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:591
> > -            pkContactField = getPKContactFieldPhoneNumber();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldPhoneNumber();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::EmailAddress:
> > -            pkContactField = getPKContactFieldEmailAddress();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldEmailAddress();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::Name:
> > -            pkContactField = getPKContactFieldName();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldName();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::PhoneticName:
> > -            pkContactField = getPKContactFieldPhoneticName();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldPhoneticName();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::PostalAddress:
> > -            pkContactField = getPKContactFieldPostalAddress();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::AddressLines:
> > -            pkContactField = getPKContactFieldPostalAddress();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
> >              postalAddressKey = getCNPostalAddressStreetKey();
> >              break;
> >  
> >          case WebCore::PaymentError::ContactField::SubLocality:
> > -            pkContactField = getPKContactFieldPostalAddress();
> > +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
> 
> Why change the name mangling strategy?

SOFT_LINK_CLASS_FOR_HEADER creates slightly wordier getter functions than the old SOFT_LINK_CLASS did. I could create better names by adding #defines to PassKitSoftLink.h, but I didn't feel strongly about doing that.

> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:-690
> > -		2D92A79821348D8500F493FD /* WebPaymentCoordinatorProxyMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AB1F77D1D1B30A9007C9BD1 /* WebPaymentCoordinatorProxyMac.mm */; };
> > -		2D92A79F2134B07E00F493FD /* WebPaymentCoordinatorProxyIOS.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AB1F77B1D1B30A9007C9BD1 /* WebPaymentCoordinatorProxyIOS.mm */; };
> 
> Does this make the files disappear from the Xcode ui? I hope not.

It does not.

Thanks for reviewing!

[Andy Estes]

(In reply to Andy Estes from comment #12)
> (In reply to Myles C. Maxfield from comment #11)
> > Comment on attachment 355431 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355431&action=review 
> > 
> > > Source/WTF/wtf/cocoa/SoftLinking.h:2
> > > - * Copyright (C) 2007, 2008, 2011-2015 Apple Inc. All rights reserved.
> > > + * Copyright (C) 2007-2018 Apple Inc. All rights reserved.
> > 
> > Is this correct? I thought our policy was to add the current year, instead
> > of retroactively adding years.
> 
> I believe it's now ok to list date ranges.

More specifically, I believe it's now ok to collapse all years into a single range.

[Andy Estes]

Comment on attachment 355431 [details]
Patch

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

>>> Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:591
>>> +            pkContactField = PAL::get_PassKit_PKContactFieldPostalAddress();
>> 
>> Why change the name mangling strategy?
> 
> SOFT_LINK_CLASS_FOR_HEADER creates slightly wordier getter functions than the old SOFT_LINK_CLASS did. I could create better names by adding #defines to PassKitSoftLink.h, but I didn't feel strongly about doing that.

Actually, I wonder if SOFT_LINK_CLASS_FOR_HEADER is being overly conservative with its naming. Objective-C classes already must coexist in a global namespace, so I'm not sure we really need to prepend the framework name to these class getters.

[Andy Estes]

Created attachment 355437 [details]
Patch

[Andy Estes]

Created attachment 355446 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355446 [details]
Patch

Clearing flags on attachment: 355446

Committed r238434: <https://trac.webkit.org/changeset/238434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.