Bug 191875

Summary: [Cocoa] Create a soft-linking file for PassKit
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ddkilzer, dino, hi, joepeck, mmaxfield, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=191899
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andy Estes 2018-11-20 21:59:19 PST 
[Cocoa] Create a soft-linking file for PassKit
Comment 1 Andy Estes 2018-11-20 22:01:14 PST Comment hidden (obsolete)
Comment 2 Andy Estes 2018-11-20 22:05:32 PST Comment hidden (obsolete)
Comment 3 Andy Estes 2018-11-20 22:09:12 PST Comment hidden (obsolete)
Comment 4 Andy Estes 2018-11-21 08:45:10 PST Comment hidden (obsolete)
Comment 5 Andy Estes 2018-11-21 09:40:49 PST Comment hidden (obsolete)
Comment 6 Andy Estes 2018-11-21 09:58:49 PST Comment hidden (obsolete)
Comment 7 Andy Estes 2018-11-21 10:26:17 PST Comment hidden (obsolete)
Comment 8 Andy Estes 2018-11-21 10:33:36 PST Comment hidden (obsolete)
Comment 9 Radar WebKit Bug Importer 2018-11-21 11:03:29 PST 
<rdar://problem/46203215>
Comment 10 Andy Estes 2018-11-21 11:12:51 PST 
Created attachment 355431 [details]
Patch
Comment 11 Myles C. Maxfield 2018-11-21 12:01:52 PST 
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.
Comment 12 Andy Estes 2018-11-21 12:34:44 PST 
(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!
Comment 13 Andy Estes 2018-11-21 12:36:40 PST 
(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.
Comment 14 Andy Estes 2018-11-21 12:48:28 PST 
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.
Comment 15 Andy Estes 2018-11-21 13:54:24 PST Comment hidden (obsolete)
Comment 16 Andy Estes 2018-11-21 16:45:24 PST 
Created attachment 355446 [details]
Patch
Comment 17 WebKit Commit Bot 2018-11-21 18:12:11 PST 
Comment on attachment 355446 [details]
Patch

Clearing flags on attachment: 355446

Committed r238434: <https://trac.webkit.org/changeset/238434>
Comment 18 WebKit Commit Bot 2018-11-21 18:12:13 PST 
All reviewed patches have been landed.  Closing bug.