Bug 189938 – [Apple Pay] Support granular errors in PaymentDetailsUpdate

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Apple Pay] Support granular errors in PaymentDetailsUpdate

[Andy Estes]

Created attachment 350717 [details]
Patch

[Andy Estes]

Created attachment 350899 [details]
Patch

[Andy Estes]

Created attachment 350903 [details]
Patch

[youenn fablet]

Comment on attachment 350903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350903&action=review

> Source/WebCore/Modules/applepay/ApplePayErrorCode.h:2
> + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.

2018?

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:315
> +static inline void appendShippingContactInvalidError(String&& message, std::optional<PaymentError::ContactField>&& contactField, Vector<PaymentError>& errors)

If ContactField is an enum, I believe we go with std::optional<> without &&.

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:321
> +Vector<PaymentError> ApplePayPaymentHandler::computeErrors(String&& error, AddressErrors&& addressErrors, PayerErrorFields&& payerErrors, JSC::Strong<JSC::JSObject>&& paymentMethodErrors) const

Does paymentMethodErrors need to be a JSC::Strong, can we make it a JSC::JSObject& instead?

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:357
> +                    errors.append({ applePayError->code(), applePayError->message(), applePayError->contactField() });

We can probably move applePayError->message()

[Andy Estes]

(In reply to youenn fablet from comment #4)
> Comment on attachment 350903 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350903&action=review
> 
> > Source/WebCore/Modules/applepay/ApplePayErrorCode.h:2
> > + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
> 
> 2018?

Yeah I'll remove this.

> 
> > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:315
> > +static inline void appendShippingContactInvalidError(String&& message, std::optional<PaymentError::ContactField>&& contactField, Vector<PaymentError>& errors)
> 
> If ContactField is an enum, I believe we go with std::optional<> without &&.

Ok.

> 
> > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:321
> > +Vector<PaymentError> ApplePayPaymentHandler::computeErrors(String&& error, AddressErrors&& addressErrors, PayerErrorFields&& payerErrors, JSC::Strong<JSC::JSObject>&& paymentMethodErrors) const
> 
> Does paymentMethodErrors need to be a JSC::Strong, can we make it a
> JSC::JSObject& instead?

We can make it a JSC::JSObject*. It might be null.

> 
> > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:357
> > +                    errors.append({ applePayError->code(), applePayError->message(), applePayError->contactField() });
> 
> We can probably move applePayError->message()

Yes.

Thanks for reviewing!

[Andy Estes]

(In reply to Andy Estes from comment #5)
> (In reply to youenn fablet from comment #4)
> > Comment on attachment 350903 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350903&action=review
> > 
> > > Source/WebCore/Modules/applepay/ApplePayErrorCode.h:2
> > > + * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
> > 
> > 2018?
> 
> Yeah I'll remove this.
> 
> > 
> > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:315
> > > +static inline void appendShippingContactInvalidError(String&& message, std::optional<PaymentError::ContactField>&& contactField, Vector<PaymentError>& errors)
> > 
> > If ContactField is an enum, I believe we go with std::optional<> without &&.
> 
> Ok.
> 
> > 
> > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:321
> > > +Vector<PaymentError> ApplePayPaymentHandler::computeErrors(String&& error, AddressErrors&& addressErrors, PayerErrorFields&& payerErrors, JSC::Strong<JSC::JSObject>&& paymentMethodErrors) const
> > 
> > Does paymentMethodErrors need to be a JSC::Strong, can we make it a
> > JSC::JSObject& instead?
> 
> We can make it a JSC::JSObject*. It might be null.
> 
> > 
> > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:357
> > > +                    errors.append({ applePayError->code(), applePayError->message(), applePayError->contactField() });
> > 
> > We can probably move applePayError->message()
> 
> Yes.

Actually we don't need to move, because since message() returns a temporary the compiler will elide the copy.

[Andy Estes]

Created attachment 350969 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350969 [details]
Patch

Clearing flags on attachment: 350969

Committed r236552: <https://trac.webkit.org/changeset/236552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44833449>

[Truitt Savell]

The new test: 
http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https.html

added in:
https://trac.webkit.org/changeset/236552/webkit

is flakey failure.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fssl%2Fapplepay%2FApplePayShippingAddressChangeEventErrorsV3.https.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https-actual.txt
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object is in an invalid state.
 Test specifying Apple Pay errors in response to the shippingaddresschange event.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

[Andy Estes]

(In reply to Truitt Savell from comment #11)
> The new test: 
> http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https.html
> 
> added in:
> https://trac.webkit.org/changeset/236552/webkit
> 
> is flakey failure.
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fssl%2Fapplepay%2FApplePayShippingA
> ddressChangeEventErrorsV3.https.html
> 
> Diff:
> ---
> /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/
> http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https-
> expected.txt
> +++
> /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/
> http/tests/ssl/applepay/ApplePayShippingAddressChangeEventErrorsV3.https-
> actual.txt
> @@ -1,3 +1,4 @@
> +CONSOLE MESSAGE: Unhandled Promise Rejection: InvalidStateError: The object
> is in an invalid state.
>  Test specifying Apple Pay errors in response to the shippingaddresschange
> event.
>  
>  On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".

Tracked by bug #190650.