Marcos Caceres [:marcosc] Reporter
	Description • 6 years ago

The `show()` method now takes a `promise<PaymentDetailsUpdate>`, allowing the content of the payment sheet to be updated before shown to the user.

	Matthew N. [:MattN]
	Comment 1 • 6 years ago

Manual test case: https://rsolomakhin.github.io/pr/wait/

	Marcos Caceres [:marcosc] Reporter
	Comment 2 • 6 years ago

Moar manual tests:

https://w3c-test.org/payment-request/show-method-optional-promise-rejects-manual.https.html
https://w3c-test.org/payment-request/show-method-optional-promise-resolves-manual.https.html

	Rouslan Solomakhin
	Comment 3 • 6 years ago

More manual tests:

https://rsolomakhin.github.io/pr/single-wait/
https://rsolomakhin.github.io/pr/multi-wait/
https://rsolomakhin.github.io/pr/us-wait/
https://rsolomakhin.github.io/pr/ko/reject-show-promise/
https://rsolomakhin.github.io/pr/ko/timeout-show-promise/
https://rsolomakhin.github.io/pr/ko/show-promise-unsupported-method/

	Marcos Caceres [:marcosc] Reporter
	Comment 4 • 6 years ago

Fancy! Thanks Rouslan! These are super useful. If you spot any functionality gaps with in the web platform ones, let me know.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 5 • 6 years ago

Hello

Eden will implement this spec change.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 6 • 6 years ago

WIP patch for supporting PaymentRequest.show() with an optional details update promise.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 7 • 6 years ago

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.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 9 • 6 years ago

Try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=70f1d11b8a53b9da5acf3811a3247282a1dde2bf&selectedJob=171211877

	Andrea Marchesini [:baku]
	Comment 10 • 6 years ago

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

	EdenChuang(ChenYu Chuang) Assignee
	Comment 11 • 6 years ago

(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.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 12 • 6 years ago

(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.

	EdenChuang(ChenYu Chuang) Assignee
	Comment 13 • 6 years ago

Fix the patch according to comment 10.

	Pulsebot
	Comment 15 • 6 years ago

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

	Dorel Luca [:dluca]
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/767166c4bef9
https://hg.mozilla.org/mozilla-central/rev/c074e1397b87

	Eric Shepherd [:sheppy]
	Comment 17 • 6 years ago

I've updated the following documents:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show
https://developer.mozilla.org/en-US/Firefox/Releases/61

New pages:

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

A detailed example for how this works probably still needs to be added but I think it can wait for now.

	Marcos Caceres [:marcosc] Reporter
	Comment 18 • 6 years ago

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?

	Chris Mills [:cmills]
	Comment 19 • 6 years ago

(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.

	Marcos Caceres [:marcosc] Reporter
	Comment 20 • 6 years ago

(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.