Closed Bug 1441709 Opened 2 years ago Closed 2 years ago

PaymentRequest.show() should take an optional details update promise

Categories

(Core :: DOM: Web Payments, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: marcosc, Assigned: chenyu.chuang)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [webpayments])

Attachments

(2 files, 3 obsolete files)

The `show()` method now takes a `promise<PaymentDetailsUpdate>`, allowing the content of the payment sheet to be updated before shown to the user.
Priority: -- → P2
Fancy! Thanks Rouslan! These are super useful. If you spot any functionality gaps with in the web platform ones, let me know.
Assignee: nobody → chenyu.chuang
Priority: P2 → P1
Hello

Eden will implement this spec change.
Attached patch bug1441709_WIP.patch (obsolete) — Splinter Review
WIP patch for supporting PaymentRequest.show() with an optional details update promise.
Status: NEW → ASSIGNED
Whiteboard: [webpayments]
Support PaymentRequest.show() with an optional PaymentDetailsUpdate promise parameter

    1. Add "optional Promise<PaymentDetailsUpdate> detailsPromise" as a parameter of PaymentRequest.show() in PaymentRequest.webidl.
    2. Let PaymentRequest inherit from PromiseNativeHandler, and implement the ResolvedCallback() and RejectedCallback() to handle the PaymentDetailsUpdate promise.
    3. Update PaymentRequest.show() implementation. If PaymentDetailsUpdate Promise is not nullptr, the show request would not be transferred to chrome process immediately until the promise is resolved/rejected.
    4. Update selectedShippingOption when requestShipping is true.
    5. Change the PaymentMethod id validation sequence according to the spec.
Attachment #8960150 - Attachment is obsolete: true
Attachment #8963950 - Flags: review?(amarchesini)
Comment on attachment 8963950 [details] [diff] [review]
Bug 1441709 - Support PaymentRequest.show() run with an opational PaymentDetailsUpdate promise. r?baku

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

r+ but answer the questions or fix the code.

::: dom/payments/PaymentRequest.cpp
@@ +403,5 @@
>    rv = IsValidCurrency(aItem, aAmount.mCurrency, aErrorMsg);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +

why this?

@@ +1034,5 @@
> +  if (NS_FAILED(UpdatePayment(aCx, details))) {
> +    AbortUpdate(NS_ERROR_DOM_ABORT_ERR);
> +    return;
> +  }
> +  mUpdating = false;

this should be set to false immediately.

::: dom/payments/PaymentRequest.h
@@ +112,5 @@
>  
>    bool Equals(const nsAString& aInternalId) const;
>  
>    bool ReadyForUpdate();
> +  inline bool IsUpdating() { return mUpdating; }

remove inline and make it bool IsUpdating() const { ...

@@ +145,5 @@
>    {
>      mRequestShipping = true;
>    }
>  
> +  virtual void

no virtual for final classes.

::: dom/payments/PaymentRequestManager.cpp
@@ +278,5 @@
>      NS_ENSURE_TRUE(requestOwner, NS_ERROR_FAILURE);
>      TabChild* tmpChild = TabChild::GetFrom(requestOwner->GetDocShell());
>      NS_ENSURE_TRUE(tmpChild, NS_ERROR_FAILURE);
>      if (tmpChild->GetTabId() == tabChild->GetTabId()) {
> +      fprintf(stderr, "the same tab child\n");

remove this. If you need logging, use MozLog
Attachment #8963950 - Flags: review?(amarchesini) → review+
Attachment #8963951 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #10)
> Comment on attachment 8963950 [details] [diff] [review]
> Bug 1441709 - Support PaymentRequest.show() run with an opational
> PaymentDetailsUpdate promise. r?baku
> 
> Review of attachment 8963950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but answer the questions or fix the code.
> 
> ::: dom/payments/PaymentRequest.cpp
> @@ +403,5 @@
> >    rv = IsValidCurrency(aItem, aAmount.mCurrency, aErrorMsg);
> >    if (NS_FAILED(rv)) {
> >      return rv;
> >    }
> > +
> 
> why this?
> 

According to the spec, valid the total item first, then currency system and currency format. Our previous implementation does not follow this sequence. It makes gecko report the different error with the spec under the case CurrencyAmount has both invalid total item and invalid currency.
(In reply to EdenChuang(ChenYu Chuang) from comment #11)
> (In reply to Andrea Marchesini [:baku] from comment #10)
> > Comment on attachment 8963950 [details] [diff] [review]
> > Bug 1441709 - Support PaymentRequest.show() run with an opational
> > PaymentDetailsUpdate promise. r?baku
> > 
> > Review of attachment 8963950 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r+ but answer the questions or fix the code.
> > 
> > ::: dom/payments/PaymentRequest.cpp
> > @@ +403,5 @@
> > >    rv = IsValidCurrency(aItem, aAmount.mCurrency, aErrorMsg);
> > >    if (NS_FAILED(rv)) {
> > >      return rv;
> > >    }
> > > +
> > 
> > why this?
> > 
> 
> According to the spec, valid the total item first, then currency system and
> currency format. Our previous implementation does not follow this sequence.
> It makes gecko report the different error with the spec under the case
> CurrencyAmount has both invalid total item and invalid currency.

Sorry the correct sequence is validing currency system, currency format, then the total value.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/767166c4bef9
Support PaymentRequest.show() with an optional PaymentDetailsUpdate promise parameter. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c074e1397b87
Mochitest for PaymentRequest.show(optional Promise<PaymentDetailsUpdate> detailsPromise). r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/767166c4bef9
https://hg.mozilla.org/mozilla-central/rev/c074e1397b87
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
When we are closer to shipping, we might need to do some kind of "let's document payment request" hackathon thing. Is that something we could organize?
(In reply to Marcos Caceres [:marcosc] from comment #18)
> When we are closer to shipping, we might need to do some kind of "let's
> document payment request" hackathon thing. Is that something we could
> organize?

For sure. I mean, we've got "sort out payment request docs" as a line item on our roadmap for sometime in Q4, so it'd be great to get engineering input on when would make sense as a completion date, and helping with demos, page reviews, etc.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #19)
> For sure. I mean, we've got "sort out payment request docs" as a line item
> on our roadmap for sometime in Q4, so it'd be great to get engineering input
> on when would make sense as a completion date, and helping with demos, page
> reviews, etc.

Are you all meeting as a team at some point in the future? If yes, the maybe I can come along? I'd like to be able to devote a few serious days to this, as it needs a big passthrough. 

In the mean time, I'll keep updating MDN as of the standardization process.
You need to log in before you can comment on or make changes to this bug.