Bug 220810 – [Apple Pay] use the first item in `shippingOptions` even when it's not `selected`

• Component: New Bugs
• Status: RESOLVED FIXED

[Devin Rousso]

This matches what PassKit does with `PKShippingMethod`.

[Devin Rousso]

Created attachment 418072 [details]
Patch

[Andy Estes]

Comment on attachment 418072 [details]
Patch

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

> LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102
> +    // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change");

My preference would be to let this assertion fail and update the test expectation (rather than commenting it out).

[Andy Estes]

Do you think we should log to the console when a merchant sets `selected` to true on something other than the first shipping option, letting them know that the selection won't be honored in the Apple Pay UI?

[Devin Rousso]

Comment on attachment 418072 [details]
Patch

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

(In reply to Andy Estes from comment #3)
> Do you think we should log to the console when a merchant sets `selected` to true on something other than the first shipping option, letting them know that the selection won't be honored in the Apple Pay UI?
Good idea!

>> LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102
>> +    // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change");
> 
> My preference would be to let this assertion fail and update the test expectation (rather than commenting it out).

I would agree, but AFAIK `assert_equals` actually throws an error, meaning that any `assert_equals` after it won't execute.  Do you know of any ways to deal with this (e.g. have each test log instead of silent-pass-and-throw-on-failure)?

[Andy Estes]

Comment on attachment 418072 [details]
Patch

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

>>> LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102
>>> +    // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change");
>> 
>> My preference would be to let this assertion fail and update the test expectation (rather than commenting it out).
> 
> I would agree, but AFAIK `assert_equals` actually throws an error, meaning that any `assert_equals` after it won't execute.  Do you know of any ways to deal with this (e.g. have each test log instead of silent-pass-and-throw-on-failure)?

Good point. You could move this assert into a new `user_activation_test` to isolate it from the others.

[Devin Rousso]

Created attachment 418098 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS]

Committed r271735: <https://trac.webkit.org/changeset/271735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418098 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73481254>