Closed Bug 1367694 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: alchen, Assigned: yrliou)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 4 obsolete files)

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.
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.
(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.
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.
Assignee: nobody → yrliou
Status: NEW → ASSIGNED
Hi Eden,

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

Thanks,
Jocelyn
Flags: needinfo?(echuang)
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.
clear ni requset.
Flags: needinfo?(echuang)
(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.
(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.
Have an offline discussion with Eden, I'll keep the original approach for now to put related logic as close as possible.
- use nsAutoString for shippingOption.
Attachment #8879667 - Attachment is obsolete: true
Attachment #8881571 - Flags: review?(amarchesini)
Attachment #8879720 - Flags: review?(amarchesini)
Blocks: 1318987
Attachment #8879720 - Flags: review?(amarchesini) → review+
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.
Attachment #8881571 - Flags: review?(amarchesini) → review+
(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?
- rebase
- add TODO comments for processing shipping options while updating payments (introduced by Bug 1318990)
Attachment #8881571 - Attachment is obsolete: true
Attachment #8883127 - Attachment description: Bug 1367694(v3)-Set payment request's shippingOption and shippingType attributes when creating it. → Bug 1367694(v3)-Set payment request's shippingOption and shippingType attributes when creating it. r=baku
- 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
Attachment #8883129 - Flags: review?(amarchesini)
Attachment #8879720 - Attachment is obsolete: true
Attachment #8883129 - Flags: review?(amarchesini) → review+
Pushed by yrliou@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/251b82b42ba2
https://hg.mozilla.org/mozilla-central/rev/7459e78047a8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
No longer blocks: 1318987
Setting this to ddn, as shippingOptions are covered on the constructor ref page:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/PaymentRequest
You need to log in before you can comment on or make changes to this bug.