1345365 - [Payment Request API] Support canMakePayment flow

Status: RESOLVED FIXED

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

Description

[Eden Chuang[:edenchuang]]

This bug is used for tracking canMakePayment flow implementation, which is a part of Payment Request API.

Comment 1

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Hi Marcos, per my understanding this is planned to have in the next releases, so P2 is the right priority, right? :)

Comment 2

[Marcos Caceres [:marcosc]]

Yep, P2 is fine. However, I can't say with any certainty which release we will target yet.

Comment 3

[Eden Chuang[:edenchuang]]

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

Comment 4

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d01cf86b14e7221a29534410f302c92aa7c4a4e

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - PaymentRequest API implementation. Support canMakePayment(), abort() and show(). r?baku

Since the canMakePayment(), abort() and show() method implementation is very similar, I merge the implementation into one patch.

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r?baku

Fix some nits from previous patch.

Comment 7

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f93feb62259c17b2b1a185a325df9d44fa5a74e

Comment 8

[Eden Chuang[:edenchuang]]

Comment on attachment 8876578 [details] [diff] [review]
Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r?baku

canceling review to update the new one for review.

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r?baku

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - Mochitest for PaymentRequest API canMakePayment(), abort() and show(). r?baku

This patch is the mochitests for PaymentRequest API canMakePayment() and abort().

And following is the try result with all attached patches

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

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8877023 [details] [diff] [review]
Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r?baku

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

::: dom/payments/PaymentActionResponse.cpp
@@ +29,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentActionResponse::GetType(uint32_t* aType)
> +{

pointer check?

@@ +47,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentCanMakeActionResponse::GetResult(bool* aResult)
> +{

pointer check?

@@ +135,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentShowActionResponse::IsAccepted(bool* aIsAccepted)
> +{

pointer check?

@@ +153,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentAbortActionResponse::GetAbortStatus(uint32_t* aAbortStatus)
> +{

pointer check?

@@ +169,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentAbortActionResponse::IsSucceeded(bool* aIsSucceeded)
> +{

pointer check?

@@ +187,5 @@
> +}
> +
> +nsresult
> +PaymentCompleteActionResponse::Init(const nsAString& aRequestId,
> +                                       const uint32_t aCompleteStatus)

indentation

@@ +196,5 @@
> +}
> +
> +nsresult
> +PaymentCompleteActionResponse::GetCompleteStatus(uint32_t* aCompleteStatus)
> +{

check the pointer?

@@ +203,5 @@
> +}
> +
> +nsresult
> +PaymentCompleteActionResponse::IsCompleted(bool* aIsCompleted)
> +{

check the pointer?

::: dom/payments/PaymentRequest.cpp
@@ +335,5 @@
> +                                   nsresult aRv)
> +{
> +  MOZ_ASSERT(mAcceptPromise);
> +  MOZ_ASSERT(ReadyForUpdate());
> +

should you check the state here?

@@ +398,5 @@
> +
> +void
> +PaymentRequest::RespondAbortPayment(bool aSuccess)
> +{
> +  MOZ_ASSERT(mAbortPromise);

You should check the state here?

::: dom/payments/PaymentRequestService.cpp
@@ +322,5 @@
> +NS_IMETHODIMP
> +PaymentRequestService::RespondPayment(nsIPaymentActionResponse* aResponse)
> +{
> +  NS_ENSURE_ARG_POINTER(aResponse);
> +  nsString requestId;

nsAutoString ?

::: dom/payments/PaymentResponse.h
@@ +17,5 @@
> +class PaymentAddress;
> +class Promise;
> +
> +} // namespace dom
> +} // namespace mozilla

remove from line 21 to 25.

::: dom/payments/ipc/PPaymentRequest.ipdl
@@ +113,5 @@
> +
> +struct IPCPaymentShowActionResponse
> +{
> +  nsString requestId;
> +  bool     isAccepted;

don't do this indentation. bool isAccepted;

::: dom/payments/ipc/PaymentRequestChild.cpp
@@ +32,5 @@
> +{
> +  if (!mActorAlive) {
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +  IPCPaymentActionResponse response = aResponse;

Don't duplicate data: const IPCPaymenetActionResponse& response = aResponse;

::: dom/payments/ipc/PaymentRequestParent.cpp
@@ +37,4 @@
>    nsresult rv;
>    switch (aRequest.type()) {
>      case IPCPaymentActionRequest::TIPCPaymentCreateActionRequest: {
>        IPCPaymentCreateActionRequest request = aRequest;

change this as the following comments.

@@ +84,5 @@
> +      MOZ_ASSERT(action);
> +      break;
> +    }
> +    case IPCPaymentActionRequest::TIPCPaymentCanMakeActionRequest: {
> +      IPCPaymentCanMakeActionRequest request = aRequest;

This duplicate data. Can you do:

const IPCPaymenetCanMakeActionRequest& request = aRequest;

@@ +94,5 @@
> +      }
> +      break;
> +    }
> +    case IPCPaymentActionRequest::TIPCPaymentShowActionRequest: {
> +      IPCPaymentShowActionRequest request = aRequest;

same here.

@@ +104,5 @@
> +      }
> +      break;
> +    }
> +    case IPCPaymentActionRequest::TIPCPaymentAbortActionRequest: {
> +      IPCPaymentAbortActionRequest request = aRequest;

and here.

@@ +182,5 @@
> +      nsCOMPtr<nsIPaymentShowActionResponse> response =
> +        do_QueryInterface(aResponse);
> +      MOZ_ASSERT(response);
> +      bool isAccepted;
> +      nsString methodName;

nsAutoString for all of these?

Comment 12

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dabbdebc569856e016a41948b396cca639c122f

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r=baku

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug 1345365 - Mochitest for PaymentRequest API canMakePayment(), abort() and show(). r=baku

Comment 15

[Eden Chuang[:edenchuang]]

[Feature/regressing bug #]: Bug 1345365
[User impact if declined]: No impact. 
[Describe test coverage new/current, TreeHerder]: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dabbdebc569856e016a41948b396cca639c122f
[Risks and why]: No risk, need preference to turn on the feature. 
[String/UUID change made/needed]: No

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bba1dee06769
PaymentRequest API canMakePayment(), abort() and show() implementation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea7a5deacf6
Mochitest for PaymentRequest API canMakePayment() and abort(). r=baku

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have caused a frequent intermittent failure: https://treeherder.mozilla.org/logviewer.html#?job_id=107223293&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cb10c0fd0c6a585a13f1796f17fa77aa0cc6202c

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If it helps, this shows the frequency of the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=980a145fce8634ddb92193f7ce278e1916f9eabb&noautoclassify&filter-searchStr=e01cd1c388e31c27eb40afa6a41221dec482fc30&tochange=cb10c0fd0c6a585a13f1796f17fa77aa0cc6202c&group_state=expanded&selectedJob=107222423

Comment 19

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bac1eb8459104dd398f655d46f4c197d763d9ca

Comment 20

[Eden Chuang[:edenchuang]]

I guess the problem is not related my patches. Even my patches is backed out, the intermittent failure still happens frequently.
Now bug 1373346 is tracking the intermittent issues. and I would like to set checkin needed for these patches again.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe227c1c67e5
PaymentRequest API canMakePayment(), abort() and show() implementation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fcadce1fed
Mochitest for PaymentRequest API canMakePayment() and abort(). r=baku

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe227c1c67e5
https://hg.mozilla.org/mozilla-central/rev/b1fcadce1fed

Comment 24

[Chris Mills [:cmills]]

I've updated the browser support info for these pages:

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

Comment 25

[Ben Tian (inactive)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #24)
> I've updated the browser support info for these pages:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> canMakePayment
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/abort
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show

The Specifications link in above pages are either invalid (show) or incorrect (canMakePaymenta & abort).

Comment 26

[Chris Mills [:cmills]]

(In reply to Ben Tian [:btian] from comment #25)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #24)
> > I've updated the browser support info for these pages:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> > canMakePayment
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/abort
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show
> 
> The Specifications link in above pages are either invalid (show) or
> incorrect (canMakePaymenta & abort).

I've updated the URL fragments in the articles, and updated the Payment Request API root URL in the MDN JSON file used to generate the links. Once this PR is accepted, the URLs will be correct on the page.

Comment 27

[Chris Mills [:cmills]]

I've also made sure the browser compat info, etc. is up to date for PaymentResponse:

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

(I didn't realize it was part of this bug until now ;-) )