Feature Request: billingContact - return zip code prior to user auth
Created attachment 389609 [details] Patch
Created attachment 389683 [details] Patch
Comment on attachment 389683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389683&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:64 > +ENABLE_APPLE_PAY_SESSION_V9_iphoneos[sdk=iphone*13.*] = ; APPLE_PAY_SESSION_V9 should be enabled on iOS 13. Since we don't support any iOSes less than 13 in trunk, you can remove this. > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:66 > +ENABLE_APPLE_PAY_SESSION_V9_iphonesimulator[sdk=iphone*13.*] = ; Ditto. > Source/WTF/wtf/PlatformHave.h:104 > +#if !defined(HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) && ((PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) \ No need to check __IPHONE_OS_VERSION_MIN_REQUIRED in this case. This would read more easily if you separated the !defined(HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) check and the PLATFORM() checks into two #if blocks. > Source/WebCore/Modules/applepay/ApplePayPaymentMethod.h:58 > +#define ALLOW_COUNTRY 1 > +#define ALLOW_COUNTRY_CODE 1 > +#define ALLOW_ADMIN_AREA 1 > +#define ALLOW_POSTAL_CODE 1 > +// #define ALLOW_SUB_ADMIN_AREA 1 > +// #define ALLOW_ADDRESS_LINES 1 > +// #define ALLOW_SUB_LOCALITY 1 > +// #define ALLOW_LOCALITY 1 These can move to PaymentMethodCocoa.mm since that's the only place they're used. Also, we don't want to commit commented-out code. You can either define the commented-out defines to 0 or remove them. After thinking more about it, we probably want to redact postal code for now. In some cases the postal code is a ZIP+4, which can designate a specific location. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:97 > +#ifdef HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS #if HAVE(PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:110 > + if (NSString *city = postalAddress.value.city) > + addressLine.append(city); > + if (NSString *state = postalAddress.value.state) > + addressLine.append(state); addressLine only represents the street address. City and state are stored elsewhere. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:125 > + if (NSArray<CNLabeledValue<CNPhoneNumber*>*> *phoneNumbers = billingContact.phoneNumbers) { > + for (CNLabeledValue<CNPhoneNumber*> *phoneNumber in phoneNumbers) > + result.phoneNumber = phoneNumber.value.stringValue; > + } Why did you choose to convert the last phone number in the contact? Which does PassKit choose when converting CNContacts to PKContacts? > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:130 > + if (NSArray<CNLabeledValue<NSString*>*> *emailAddresses = billingContact.emailAddresses) { > + for (CNLabeledValue<NSString*> *emailAddress in emailAddresses) > + result.emailAddress = emailAddress.value; > + } Ditto. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:142 > + for (CNLabeledValue<CNPostalAddress*> *postalAddress in postalAddresses) { Ditto. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:143 > +#ifdef ALLOW_ADDRESS_LINES This just checks if ALLOW_ADDRESS_LINES is defined, but you probably want to check if it's defined to 1. Maybe you could introduce an ALLOW() function macro in this file along the lines of ENABLE(), HAVE(), etc. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:189 > +#ifdef HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS #if HAVE(PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) > LayoutTests/http/tests/ssl/applepay/ApplePayBillingAddress.html:105 > + debug(event.methodDetails.billingAddress.phoneNumber); > + debug(event.methodDetails.billingAddress.emailAddress); > + debug(event.methodDetails.billingAddress.givenName); > + debug(event.methodDetails.billingAddress.familyName); > + debug(event.methodDetails.billingAddress.phoneticGivenName); > + debug(event.methodDetails.billingAddress.phoneticFamilyName); > + debug(event.methodDetails.billingAddress.addressLines); > + debug(event.methodDetails.billingAddress.subLocality); > + debug(event.methodDetails.billingAddress.locality); > + debug(event.methodDetails.billingAddress.postalCode); > + debug(event.methodDetails.billingAddress.subAdministrativeArea); > + debug(event.methodDetails.billingAddress.country); > + debug(event.methodDetails.billingAddress.countryCode); > + debug(event.methodDetails.network); > + debug(event.methodDetails.type); I'd use shouldBe() instead of debug() for these.
Created attachment 389726 [details] Patch
Comment on attachment 389726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389726&action=review > Source/WTF/wtf/PlatformHave.h:105 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) No need for the () > Source/WebCore/Modules/applepay/ApplePayPaymentMethod.h:48 > + Optional<ApplePayPaymentContact> billingAddress; I know this is called `billingAddress` in PKPaymentMethod, but let's call it `billingContact` here. > Source/WebCore/Modules/applepay/ApplePayPaymentMethod.idl:35 > + ApplePayPaymentContact billingAddress; Ditto. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:54 > +#define ALLOW(FIELD) (ALLOW_##FIELD == 1) Please #undef this at the end of the file. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:134 > + if (NSArray<CNLabeledValue<CNPhoneNumber*>*> *phoneNumbers = billingContact.phoneNumbers) { > + if (CNLabeledValue<CNPhoneNumber*> *phoneNumber = [phoneNumbers firstObject]) > + result.phoneNumber = phoneNumber.value.stringValue; > + } You're probably basing this on the other convert() functions in this file, but I think we can be more concise here. These nil checks are unnecessary, because in Objective-C you can safely send a message to nil and get a return value of 0/nil. 0/nil converted to a WTF::String is the same as WTF::String(), which is the value result.phoneNumber would have if you hadn't set it. So this is the same as `result.phoneNumber = billingContact.phoneNumbers.firstObject.value.stringValue`. That's a lot for one statement, though, so maybe I'd break it up like this: if (auto firstPhoneNumber = billingContact.phoneNumbers.firstObject) result.phoneNumber = firstPhoneNumber.value.stringValue; > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:139 > + if (NSArray<CNLabeledValue<NSString*>*> *emailAddresses = billingContact.emailAddresses) { > + if (CNLabeledValue<NSString*> *emailAddress = [emailAddresses firstObject]) > + result.emailAddress = emailAddress.value; > + } Ditto. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:142 > + if (NSString *givenName = billingContact.givenName) > + result.givenName = givenName; `result.givenName = billingContact.givenName` > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:149 > + if (NSString *familyName = billingContact.familyName) > + result.familyName = familyName; > + > + if (NSString *phoneticGivenName = billingContact.phoneticGivenName) > + result.phoneticGivenName = phoneticGivenName; > + if (NSString *phoneticFamilyName = billingContact.phoneticFamilyName) > + result.phoneticFamilyName = phoneticFamilyName; Ditto. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:182 > +#if ALLOW(SUB_LOCALITY) > + if (NSString *subLocality = postalAddress.value.subLocality) > + result.subLocality = subLocality; > +#endif > +#if ALLOW(LOCALITY) > + if (NSString *locality = postalAddress.value.city) > + result.locality = locality; > +#endif > +#if ALLOW(SUB_ADMIN_AREA) > + if (NSString *subAdministrativeArea = postalAddress.value.subAdministrativeArea) > + result.subAdministrativeArea = subAdministrativeArea; > +#endif > +#if ALLOW(ADMIN_AREA) > + if (NSString *administrativeArea = postalAddress.value.state) > + result.administrativeArea = administrativeArea; > +#endif > +#if ALLOW(POSTAL_CODE) > + if (NSString *postalCode = postalAddress.value.postalCode) > + result.postalCode = postalCode; > +#endif > +#if ALLOW(COUNTRY) > + if (NSString *country = postalAddress.value.country) > + result.country = country; > +#endif > +#if ALLOW(COUNTRY_CODE) > + if (NSString *ISOCountryCode = postalAddress.value.ISOCountryCode) > + result.countryCode = ISOCountryCode; > +#endif I think these should move into the convert function for CNPostalAddress. > LayoutTests/http/tests/ssl/applepay/ApplePayBillingAddress.html:105 > + shouldBe("event.methodDetails.billingAddress.phoneNumber", "'12345678910'"); > + shouldBe("event.methodDetails.billingAddress.emailAddress", "'wpt@w3.org'"); > + shouldBe("event.methodDetails.billingAddress.givenName", "'Web'"); > + shouldBe("event.methodDetails.billingAddress.familyName", "'Test'"); > + shouldBe("event.methodDetails.billingAddress.phoneticGivenName", "'Web'"); > + shouldBe("event.methodDetails.billingAddress.phoneticFamilyName", "'Test'"); > + shouldBe("event.methodDetails.billingAddress.addressLines", "['1 wpt street']"); > + shouldBe("event.methodDetails.billingAddress.subLocality", "'AA'"); > + shouldBe("event.methodDetails.billingAddress.locality", "'BB'"); > + shouldBe("event.methodDetails.billingAddress.postalCode", "'12345'"); > + shouldBe("event.methodDetails.billingAddress.subAdministrativeArea", "'CC'"); > + shouldBe("event.methodDetails.billingAddress.country", "'United States'"); > + shouldBe("event.methodDetails.billingAddress.countryCode", "'US'"); > + shouldBe("event.methodDetails.network","'visa'"); > + shouldBe("event.methodDetails.type","'credit'"); You can use the variables defined on lines 48-60 as the expected values here.
Created attachment 389864 [details] Patch
Comment on attachment 389864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389864&action=review > Source/WebCore/ChangeLog:3 > + Feature Request: billingContact - return zip code prior to user auth I think this bug (and the associated radar) could use a better title. Perhaps something like "[Apple Pay] Provide a redacted billing contact during payment method selection". > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:117 > + result.addressLine = addressLine; result.addressLine = WTFMove(addressLine) > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:122 > + if (NSString *subLocality = postalAddress.value.subLocality) > + result.subLocality = subLocality; No need for the if statement. I'd just do `result.subLocality = postalAddress.value.subLocality`. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:146 > + if (NSString *locality = postalAddress.value.city) > + result.locality = locality; > +#endif > +#if ALLOW(SUB_ADMIN_AREA) > + if (NSString *subAdministrativeArea = postalAddress.value.subAdministrativeArea) > + result.subAdministrativeArea = subAdministrativeArea; > +#endif > +#if ALLOW(ADMIN_AREA) > + if (NSString *administrativeArea = postalAddress.value.state) > + result.administrativeArea = administrativeArea; > +#endif > +#if ALLOW(POSTAL_CODE) > + if (NSString *postalCode = postalAddress.value.postalCode) > + result.postalCode = postalCode; > +#endif > +#if ALLOW(COUNTRY) > + if (NSString *country = postalAddress.value.country) > + result.country = country; > +#endif > +#if ALLOW(COUNTRY_CODE) > + if (NSString *ISOCountryCode = postalAddress.value.ISOCountryCode) > + result.countryCode = ISOCountryCode; Ditto for each of these assignments. > Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:171 > + if (NSString *givenName = billingContact.givenName) > + result.givenName = billingContact.givenName; > + if (NSString *familyName = billingContact.familyName) > + result.familyName = billingContact.familyName; > + > + if (NSString *phoneticGivenName = billingContact.phoneticGivenName) > + result.phoneticGivenName = phoneticGivenName; > + if (NSString *phoneticFamilyName = billingContact.phoneticFamilyName) > + result.phoneticFamilyName = phoneticFamilyName; And these.
Created attachment 390003 [details] Patch
Comment on attachment 390003 [details] Patch Rejecting attachment 390003 [details] from commit-queue. nmouchtaris@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 390003 [details] Patch Clearing flags on attachment: 390003 Committed r256041: <https://trac.webkit.org/changeset/256041>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59265585>
It looks like the changes in https://trac.webkit.org/changeset/256041/webkit broke the Catalina build: https://build.webkit.org/builders/Apple-Catalina-Release-Build/builds/3270 error: ./Modules/applepay/cocoa/PaymentMethodCocoa.mm:178:51: error: property 'billingAddress' not found on object of type 'PKPaymentMethod *'
Reopening to attach new patch.
Created attachment 390130 [details] Patch
Created attachment 390131 [details] Patch
Comment on attachment 390131 [details] Patch Clearing flags on attachment: 390131 Committed r256071: <https://trac.webkit.org/changeset/256071>