[webkit-reviews] review denied: [Bug 207169] Feature Request: billingContact - return zip code prior to user auth : [Attachment 389726] 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 389726: Patch

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




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