Closed Bug 1345365 Opened 3 years ago Closed 2 years ago

[Payment Request API] Support canMakePayment flow

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: edenchuang, Assigned: edenchuang)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [WP-MVP][M3])

Attachments

(2 files, 4 obsolete files)

This bug is used for tracking canMakePayment flow implementation, which is a part of Payment Request API.
Blocks: 1318987
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)
Priority: -- → P3
Priority: P3 → P2
Component: DOM: Device Interfaces → DOM: Web Payments
Blocks: 1318987
Whiteboard: [WP-MVP][M3]
Assignee: nobody → echuang
Since the canMakePayment(), abort() and show() method implementation is very similar, I merge the implementation into one patch.
Attachment #8873684 - Flags: review?(amarchesini)
Fix some nits from previous patch.
Attachment #8873684 - Attachment is obsolete: true
Attachment #8873684 - Flags: review?(amarchesini)
Attachment #8876578 - Flags: review?(amarchesini)
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)
Attachment #8876578 - Attachment is obsolete: true
Attachment #8877023 - Flags: review?(amarchesini)
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+
[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
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
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
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
https://hg.mozilla.org/mozilla-central/rev/fe227c1c67e5
https://hg.mozilla.org/mozilla-central/rev/b1fcadce1fed
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1318989
(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.