1398991 - Set shippingOption during construction only if options.requestShipping is true.

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

According to the spec change 

https://github.com/w3c/payment-request/issues/609

shippingOption should always be null when options.requestShipping is false, even there is a shippingOption is set as selected.

Comment 1

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce703e732b548d623fb8003b942cae0621af7f7

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1398991 - Set PaymentRequest.shippingOption only if PaymentOptions.requestShipping is true. r?baku

This patch updates the PaymentRequest.shippingOption setting implementation according to the spec changes.

1. Return TypeError when creating PaymentRequest with duplicate shippingOption id.
2. Setting the default selected shippingOption only if PaymentOptions.requestShipping is true, otherwise, set the default shippingOption with null.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1398991 - Mochitest for PaymentRequest.shippingOption attribute setting. r?baku

This patch updates the tests related to PaymentRequest.shippingOption setting. 
It changes the test_construction.html with following modifications.
   
    1. Modify the test testWithDuplicateShippingOptionsParameters to expect a
       TypeError when constructing with duplicate shippingOption ids.
    2. Add test testShippingOtpionAttribute for shippingOption setting checking
       with following conditions
       1. No selected shippingOption and PaymentOptions.requestShipping is false
       2. One selected shippingOption and PaymentOptions.requestShipping is false
       3. One selected shippingOption and PaymentOptions.requestShipping is true
       4. Multiple selected shippingOptions and PaymentOptions.requestShipping is
          true.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1398991 - Set PaymentRequest.shippingOption only if PaymentOptions.requestShipping is true. r=baku

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1398991 - Mochitest for PaymentRequest.shippingOption attribute setting. r=baku

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/080349a1a621
https://hg.mozilla.org/mozilla-central/rev/26289fa4d71a

Comment 7

[Chris Mills [:cmills]]

The 

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/shippingOption

page already describes this behaviour (I edited it a little bit), so I've marked this as ddc. Let me know if you think the description is ok!

Comment 8

[Eden Chuang[:edenchuang]]

Thanks for the documentation, I am ok with the description. 

Marcos, how do you think?

Comment 9

[Marcos Caceres [:marcosc]]

I replaced the example with the one from the spec, as it was not using promises correctly and did some questionable things (sorry, Chris!). 

cmills - few critical things: avoid calling `reject()` with text, always use new Error("message")... otherwise, the developer doesn't get a stack trace if things go bad. 

Also, avoid ever passing around resolve()/reject() to another function. That should never ever ever ever be necessary - if that happens, then that means there is a serious bug in the code. 

Remember all also that APIs methods that expect a promise (e.g., `updateWith()`) will always "auto-wrap" things in promises, so these are all equivalent:

```JS
// just an object - auto-wrap
ev.updateWith({foo: "bar"});

// resolving but sync. 
ev.updateWith(new Promise(resolve => resolve({foo: "bar"}));

// calling with a resolved promise. 
ev.updateWith(Promise.resolve({foo: "bar"}));

```

Generally, it's best to now avoid using Promises entirely and just use async/await - treat `.then()` or `.catch()` as a code smell.