[Webkit-unassigned] [Bug 207169] Feature Request: billingContact - return zip code prior to user auth

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=207169

Andy Estes <aestes at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #389683|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200204/3fd6ede4/attachment.htm>