Closed Bug 1398991 Opened 4 years ago Closed 4 years ago

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


(Core :: DOM: Web Payments, enhancement)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: edenchuang, Assigned: edenchuang)



(Keywords: dev-doc-complete)


(2 files, 2 obsolete files)

According to the spec change

shippingOption should always be null when options.requestShipping is false, even there is a shippingOption is set as selected.
Assignee: nobody → echuang
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.
Attachment #8908527 - Flags: review?(amarchesini)
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
Attachment #8908531 - Flags: review?(amarchesini)
Attachment #8908527 - Flags: review?(amarchesini) → review+
Attachment #8908531 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

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!
Thanks for the documentation, I am ok with the description. 

Marcos, how do you think?
Flags: needinfo?(mcaceres)
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:

// 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.
Flags: needinfo?(mcaceres)
Depends on: 1436903
You need to log in before you can comment on or make changes to this bug.