[webkit-reviews] review denied: [Bug 207169] Feature Request: billingContact - return zip code prior to user auth : [Attachment 389683] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 207169: Feature Request: billingContact - return zip code prior to user
auth
https://bugs.webkit.org/show_bug.cgi?id=207169

Attachment 389683: Patch

https://bugs.webkit.org/attachment.cgi?id=389683&action=review




--- Comment #3 from Andy Estes <aestes at apple.com> ---
Comment on attachment 389683
  --> https://bugs.webkit.org/attachment.cgi?id=389683
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.