[Payment Request API] Support canMakePayment flow

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM: Web Payments
P2
normal
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: edenchuang, Assigned: edenchuang)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla56
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [WP-MVP][M3])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 months ago
This bug is used for tracking canMakePayment flow implementation, which is a part of Payment Request API.
(Assignee)

Updated

9 months ago
Blocks: 1318987
(Assignee)

Updated

9 months ago
No longer blocks: 1318987
Depends on: 1345391, 1345392
Hi Marcos, per my understanding this is planned to have in the next releases, so P2 is the right priority, right? :)
Component: DOM → DOM: Device Interfaces
Flags: needinfo?(mcaceres)
Yep, P2 is fine. However, I can't say with any certainty which release we will target yet.
Flags: needinfo?(mcaceres)

Updated

9 months ago
Priority: -- → P3

Updated

9 months ago
Priority: P3 → P2

Updated

8 months ago
Component: DOM: Device Interfaces → DOM: Web Payments

Updated

7 months ago
Blocks: 1318987
Whiteboard: [WP-MVP][M3]
(Assignee)

Updated

6 months ago
Assignee: nobody → echuang
(Assignee)

Comment 3

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe7ffc9daa6f82f2aed0c883a84e08e4d83f994
(Assignee)

Comment 4

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d01cf86b14e7221a29534410f302c92aa7c4a4e
(Assignee)

Comment 5

6 months ago
Created attachment 8873684 [details] [diff] [review]
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.
Attachment #8873684 - Flags: review?(amarchesini)
(Assignee)

Comment 6

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

Fix some nits from previous patch.
Attachment #8873684 - Attachment is obsolete: true
Attachment #8873684 - Flags: review?(amarchesini)
Attachment #8876578 - Flags: review?(amarchesini)
(Assignee)

Comment 7

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f93feb62259c17b2b1a185a325df9d44fa5a74e
(Assignee)

Comment 8

5 months ago
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.
Attachment #8876578 - Flags: review?(amarchesini)
(Assignee)

Comment 9

5 months ago
Created attachment 8877023 [details] [diff] [review]
Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r?baku
Attachment #8876578 - Attachment is obsolete: true
Attachment #8877023 - Flags: review?(amarchesini)
(Assignee)

Comment 10

5 months ago
Created attachment 8877025 [details] [diff] [review]
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
Attachment #8877025 - Flags: review?(amarchesini)
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?
Attachment #8877023 - Flags: review?(amarchesini) → review+
Attachment #8877025 - Flags: review?(amarchesini) → review+
Keywords: dev-doc-needed
(Assignee)

Comment 12

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dabbdebc569856e016a41948b396cca639c122f
(Assignee)

Comment 13

5 months ago
Created attachment 8877571 [details] [diff] [review]
Bug 1345365 - PaymentRequest API canMakePayment(), abort() and show() implementation. r=baku
Attachment #8877023 - Attachment is obsolete: true
Attachment #8877571 - Flags: review+
(Assignee)

Comment 14

5 months ago
Created attachment 8877573 [details] [diff] [review]
Bug 1345365 - Mochitest for PaymentRequest API canMakePayment(), abort() and show(). r=baku
Attachment #8877025 - Attachment is obsolete: true
Attachment #8877573 - Flags: review+
(Assignee)

Comment 15

5 months ago
[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
Keywords: checkin-needed

Comment 16

5 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-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
Flags: needinfo?(echuang)
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
(Assignee)

Comment 19

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bac1eb8459104dd398f655d46f4c197d763d9ca
(Assignee)

Comment 20

5 months ago
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.
Flags: needinfo?(echuang)
Keywords: checkin-needed

Comment 21

5 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 22

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe227c1c67e5
https://hg.mozilla.org/mozilla-central/rev/b1fcadce1fed
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

5 months ago
Duplicate of this bug: 1318989
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 25

4 months ago
(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).
(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.
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 ;-) )
You need to log in before you can comment on or make changes to this bug.