1367694 - [Payment Request API] Add mechanisms for shipping options in constructor

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Alphan Chen [:alchen]]

We should do the following in constructor.

1. If there is no selected shipping option, then PaymentRequest.shippingOption remains null.
2. If there is a selected shipping option, then it becomes synchronously selected.
3. If there is a multiple selected shipping options, only the last is selected.
4. If there are any duplicate shipping option ids, then there are no shipping options.
5. If options.requestShipping is true, request.shippingType will be options.shippingType.

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Just a quick note here, to fix this, we should implement "Process shipping options" step from the spec of PaymentRequest constructor, and populate shippingOption and shippingType properly when requestShipping is true.

Comment 2

[Alphan Chen [:alchen]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #1)
> Just a quick note here, to fix this, we should implement "Process shipping
> options" step from the spec of PaymentRequest constructor, and populate
> shippingOption and shippingType properly when requestShipping is true.

Correct.

One more thing, we should save the latest shippingOption(with the changes by the steps mentioned in this bug) into mShippingOption before returning the request object.
The test will try to get shippingOption right after they create PaymentRequest.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694-Set payment request's shippingOption and shippingType attributes when creating it.

This patch
- fixes wpt cases mentioned in comment 0.
- resets details.shippingOptions to an empty array when converting to IPC structure if there are duplicate IDs in the given options.

TODO:
- mochitest for ensuring details.shippingOptions is an empty array if there are duplicate IDs in the given options.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694 - Mochitest for resetting details.shippingOptions when there are duplicate option IDs.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Eden,

Would you mind to take a look of my patches since the main revision is in PaymentRequestManager?

Thanks,
Jocelyn

Comment 6

[Eden Chuang[:edenchuang]]

Comment on attachment 8879667 [details] [diff] [review]
Bug 1367694-Set payment request's shippingOption and shippingType attributes when creating it.

Review of attachment 8879667 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestManager.cpp
@@ +405,5 @@
> +   *  Otherwise, set mShippingOption to null and set the resetShippingOptions
> +   *  flag to reset details.shippingOptions to an empty array later when
> +   *  converting details to IPC structure.
> +   */
> +  nsString shippingOption;

Can we use nsAutoString?

@@ +407,5 @@
> +   *  converting details to IPC structure.
> +   */
> +  nsString shippingOption;
> +  bool resetShippingOptions = false;
> +  GetSelectedShippingOption(aDetails, shippingOption, resetShippingOptions);

Moving above codes(line 402 to 411) and following convert codes(line 425 to 442) before calling new PaymentRequest()(line 385), such that we can returns nullptr without new PaymentRequest() when any one of parameters is invalid or not converted.

Comment 7

[Eden Chuang[:edenchuang]]

clear ni requset.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Eden Chuang[:edenchuang] from comment #6)
> Comment on attachment 8879667 [details] [diff] [review]
> Bug 1367694-Set payment request's shippingOption and shippingType attributes
> when creating it.
> 
> Review of attachment 8879667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payments/PaymentRequestManager.cpp
> @@ +405,5 @@
> > +   *  Otherwise, set mShippingOption to null and set the resetShippingOptions
> > +   *  flag to reset details.shippingOptions to an empty array later when
> > +   *  converting details to IPC structure.
> > +   */
> > +  nsString shippingOption;
> 
> Can we use nsAutoString?
> 

Sure.

> @@ +407,5 @@
> > +   *  converting details to IPC structure.
> > +   */
> > +  nsString shippingOption;
> > +  bool resetShippingOptions = false;
> > +  GetSelectedShippingOption(aDetails, shippingOption, resetShippingOptions);
> 
> Moving above codes(line 402 to 411) and following convert codes(line 425 to
> 442) before calling new PaymentRequest()(line 385), such that we can returns
> nullptr without new PaymentRequest() when any one of parameters is invalid
> or not converted.

Isn't aRequest only get assigned by request.forget(aRequest) right before we return NS_OK?
If possible, I would prefer not to split shippingOptions processing and assigning request's mShippingOption for readability.

Comment 9

[Eden Chuang[:edenchuang]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #8)
> (In reply to Eden Chuang[:edenchuang] from comment #6)
> > Comment on attachment 8879667 [details] [diff] [review]
> > Bug 1367694-Set payment request's shippingOption and shippingType attributes
> > when creating it.
> > 
> > Review of attachment 8879667 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/payments/PaymentRequestManager.cpp
> > @@ +405,5 @@
> > > +   *  Otherwise, set mShippingOption to null and set the resetShippingOptions
> > > +   *  flag to reset details.shippingOptions to an empty array later when
> > > +   *  converting details to IPC structure.
> > > +   */
> > > +  nsString shippingOption;
> > 
> > Can we use nsAutoString?
> > 
> 
> Sure.
> 
> > @@ +407,5 @@
> > > +   *  converting details to IPC structure.
> > > +   */
> > > +  nsString shippingOption;
> > > +  bool resetShippingOptions = false;
> > > +  GetSelectedShippingOption(aDetails, shippingOption, resetShippingOptions);
> > 
> > Moving above codes(line 402 to 411) and following convert codes(line 425 to
> > 442) before calling new PaymentRequest()(line 385), such that we can returns
> > nullptr without new PaymentRequest() when any one of parameters is invalid
> > or not converted.
> 
> Isn't aRequest only get assigned by request.forget(aRequest) right before we
> return NS_OK?

Yes, you are right, I just want to avoid unnecessary new/delete.

> If possible, I would prefer not to split shippingOptions processing and
> assigning request's mShippingOption for readability.

Since SetShippingOption() and SetShippingType() should be only called in Constructor, could we pass them as parameters of PaymentRequest::CreatePaymentRequest()? That means

  already_AddRefed<PaymentRequest> PaymentRequest::CreatePaymentRequest(nsPIDOMWindowInner* aWindow, nsresult& aRv, const nsAString& shippingOption = EmptyString(), const Nullable<PaymentShippingType>& aShippingType = nullvalue);
  
  and

  PaymentRequest::PaymentRequest(nsPIDOMWindowInner* aWindow, const nsAString& aInternalId, const nsAString& shippingOption, const Nullable<PaymentShippingType>& aShippingType); 

such that we can avoid exposing SetShippingOption() and SetShippingType() to others.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

Have an offline discussion with Eden, I'll keep the original approach for now to put related logic as close as possible.

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694(v2)-Set payment request's shippingOption and shippingType attributes when creating it.

- use nsAutoString for shippingOption.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8881571 [details] [diff] [review]
Bug 1367694(v2)-Set payment request's shippingOption and shippingType attributes when creating it.

Review of attachment 8881571 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestManager.cpp
@@ +163,5 @@
>    // Convert PaymentDetailsBase members
>    nsTArray<IPCPaymentItem> displayItems;
>    nsTArray<IPCPaymentShippingOption> shippingOptions;
>    nsTArray<IPCPaymentDetailsModifier> modifiers;
> +  nsresult rv = ConvertDetailsBase(aDetails, displayItems, shippingOptions, modifiers, aResetShippingOptions);

80 chars max.

@@ +341,5 @@
>    return nullptr;
>  }
>  
> +void
> +GetSelectedShippingOption(const PaymentDetailsInit& aDetails, nsAString& aOption, bool& aResetOptions)

1. 80 chars?
2. aResetOptions is not set to any value.
3. Can you use pointers for the boolean? We usually do this to immediately understand that it's an output param.

Comment 13

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Andrea Marchesini [:baku] from comment #12)
> Comment on attachment 8881571 [details] [diff] [review]
> Bug 1367694(v2)-Set payment request's shippingOption and shippingType
> attributes when creating it.
> 
> Review of attachment 8881571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payments/PaymentRequestManager.cpp
> @@ +163,5 @@
> >    // Convert PaymentDetailsBase members
> >    nsTArray<IPCPaymentItem> displayItems;
> >    nsTArray<IPCPaymentShippingOption> shippingOptions;
> >    nsTArray<IPCPaymentDetailsModifier> modifiers;
> > +  nsresult rv = ConvertDetailsBase(aDetails, displayItems, shippingOptions, modifiers, aResetShippingOptions);
> 
> 80 chars max.
> 
> @@ +341,5 @@
> >    return nullptr;
> >  }
> >  
> > +void
> > +GetSelectedShippingOption(const PaymentDetailsInit& aDetails, nsAString& aOption, bool& aResetOptions)
> 
> 1. 80 chars?
> 2. aResetOptions is not set to any value.
> 3. Can you use pointers for the boolean? We usually do this to immediately
> understand that it's an output param.

Thanks for the review.
Sure, I'll change it to a pointer.
aResetOptions was initialized to false before passing in, and only set to true if duplicate IDs are found in GetSelectedShippingOption.
Would you prefer I set it to false inside GetSelectedShippingOption?

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694(v3)-Set payment request's shippingOption and shippingType attributes when creating it.

- rebase
- add TODO comments for processing shipping options while updating payments (introduced by Bug 1318990)

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694(v3)-Set payment request's shippingOption and shippingType attributes when creating it. r=baku

- fix 80 chars

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1367694 (v2) - Mochitest for resetting details.shippingOptions when there are duplicate option IDs.

- Add duplicate ID tests into new plain tests added by Bug 1318990

Hi Baku,

Would you mind to take a look again since this version add tests into another test set added by Bug 1318990?

Thanks,
Jocelyn

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e8aba476bfa961c7dc6f7df757942d00da07434

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/251b82b42ba2
Set payment request's shippingOption and shippingType attributes when creating it. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7459e78047a8
Mochitest for resetting details.shippingOptions when there are duplicate option IDs. r=baku

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/251b82b42ba2
https://hg.mozilla.org/mozilla-central/rev/7459e78047a8

Comment 20

[Chris Mills [:cmills]]

Setting this to ddn, as shippingOptions are covered on the constructor ref page:

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