Implement PaymentRequestUpdateEvent Interface

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: marcosc, Assigned: alchen)

Tracking

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

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

The PaymentRequestUpdateEvent enables the web page to update the details of the payment request in response to a user interaction
Duplicate of this bug: 1318991
Mapping the Trello_P1 priority into bugzilla P3.
Priority: -- → P3
Blocks: 1345369
Blocks: 1345371
Component: DOM → DOM: Web Payments
Depends on: 1355398, 1355397
Whiteboard: [WP-MVP][M3]
Assignee: nobody → alchen
PaymentRequestUpdateEvent implementation and also support PaymentRequest API onshippingaddress/optionchange.
Attachment #8876580 - Flags: review?(amarchesini)
Comment on attachment 8876580 [details] [diff] [review]
Bug 1318990 - PaymentRequestUpdateEvent implementation and PaymentRequest API onshippingaddress/optionchange implementation. r?baku

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

lgtm, but please move the check of mActorAlived on the main-thread side of the methods.

::: dom/payments/PaymentActionRequest.cpp
@@ +175,5 @@
> +NS_IMETHODIMP
> +PaymentUpdateActionRequest::GetDetails(nsIPaymentDetails** aDetails)
> +{
> +  NS_ENSURE_ARG_POINTER(aDetails);
> +  *aDetails = nullptr;

this can be removed.

::: dom/payments/PaymentRequest.cpp
@@ +122,5 @@
>  
>  bool
> +PaymentRequest::IsValidDetailsUpdate(const PaymentDetailsUpdate& aDetails)
> +{
> +  nsString message;

nsAutoString ?

::: dom/payments/PaymentRequestData.cpp
@@ +669,5 @@
> +  , mSortingCode(EmptyString())
> +  , mLanguageCode(EmptyString())
> +  , mOrganization(EmptyString())
> +  , mRecipient(EmptyString())
> +  , mPhone(EmptyString())

by default they all are EmptyString(). Remove this CTOR completely and in .h do:

PaymentAddress() = default;

::: dom/payments/PaymentRequestUpdateEvent.cpp
@@ +20,5 @@
> +NS_IMPL_ADDREF_INHERITED(PaymentRequestUpdateEvent, Event)
> +NS_IMPL_RELEASE_INHERITED(PaymentRequestUpdateEvent, Event)
> +
> +already_AddRefed<PaymentRequestUpdateEvent>
> +PaymentRequestUpdateEvent::Constructor(mozilla::dom::EventTarget* aOwner,

remove mozilla::dom::

@@ +22,5 @@
> +
> +already_AddRefed<PaymentRequestUpdateEvent>
> +PaymentRequestUpdateEvent::Constructor(mozilla::dom::EventTarget* aOwner,
> +                                       const nsAString& aType,
> +                                       const PaymentRequestUpdateEventInit& aEventInitDict)

why do we need this constructor?

::: dom/payments/PaymentRequestUpdateEvent.h
@@ +18,5 @@
> +
> +class Promise;
> +
> +} // namespace dom
> +} // namespace mozilla

don't close the namespaces here. Remove lines 21-25

@@ +33,5 @@
> +
> +  explicit PaymentRequestUpdateEvent(EventTarget* aOwner);
> +
> +  virtual JSObject*
> +    WrapObjectInternal(JSContext* aCx,

don't use this indentation. Just do:

virtual JSObject*
WrapObjectInternal(...

here and elsewhere.

@@ +44,5 @@
> +
> +  static already_AddRefed<PaymentRequestUpdateEvent>
> +    Constructor(mozilla::dom::EventTarget* aOwner,
> +                const nsAString& aType,
> +                const PaymentRequestUpdateEventInit& aEventInitDict);

why 2 constructors?

::: dom/payments/ipc/PaymentRequestParent.cpp
@@ +345,5 @@
> +NS_IMETHODIMP
> +PaymentRequestParent::ChangeShippingOption(const nsAString& aRequestId,
> +                                           const nsAString& aOption)
> +{
> +  if (!mActorAlived) {

Do you protect this with a mutex? You should. Or better, you should do the check only on the main-thread.
Basically move it after line 361. Here and elsewhere.
Attachment #8876580 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #4)

> why 2 constructors?

  static already_AddRefed<PaymentRequestUpdateEvent>
    Constructor(const GlobalObject& aGlobal,
                const nsAString& aType,
                const PaymentRequestUpdateEventInit& aEventInitDict,
                ErrorResult& aRv);

This is for webIDL binding.


> 
> @@ +22,5 @@
> > +
> > +already_AddRefed<PaymentRequestUpdateEvent>
> > +PaymentRequestUpdateEvent::Constructor(mozilla::dom::EventTarget* aOwner,
> > +                                       const nsAString& aType,
> > +                                       const PaymentRequestUpdateEventInit& aEventInitDict)
> 
> why do we need this constructor?
> 

Currently, I reference "VRDisplayEvent" for this implementation.
http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/dom/vr/VRDisplayEvent.cpp#74

However, we can rename this function to avoid confusion.
What do you think?
(In reply to Alphan Chen [:alchen] from comment #5)
> > > +
> > > +already_AddRefed<PaymentRequestUpdateEvent>
> > > +PaymentRequestUpdateEvent::Constructor(mozilla::dom::EventTarget* aOwner,
> > > +                                       const nsAString& aType,
> > > +                                       const PaymentRequestUpdateEventInit& aEventInitDict)
> > 
> > why do we need this constructor?
> > 
> 
> Currently, I reference "VRDisplayEvent" for this implementation.
> http://searchfox.org/mozilla-central/rev/
> 61054508641ee76f9c49bcf7303ef3cfb6b410d2/dom/vr/VRDisplayEvent.cpp#74
> 
> However, we can rename this function to avoid confusion.
> What do you think?

How about "CreateUpdateEvent()"?
Flags: needinfo?(amarchesini)
VRDisplayEvent::Constructor is used in http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#13875.
You don't have a similar scenario. I don't think you need this second CTOR. Do you?
Flags: needinfo?(amarchesini)
Comment on attachment 8876580 [details] [diff] [review]
Bug 1318990 - PaymentRequestUpdateEvent implementation and PaymentRequest API onshippingaddress/optionchange implementation. r?baku

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

(In reply to Andrea Marchesini [:baku] from comment #7)
> VRDisplayEvent::Constructor is used in
> http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#13875.
> You don't have a similar scenario. I don't think you need this second CTOR.
> Do you?

We create the event by using the second CTOR in PaymentRequest::DispatchUpdateEvent.

::: dom/payments/PaymentRequest.cpp
@@ +519,5 @@
> +  init.mBubbles = false;
> +  init.mCancelable = false;
> +
> +  RefPtr<PaymentRequestUpdateEvent> event =
> +    PaymentRequestUpdateEvent::Constructor(this, aType, init);

Now we use the second CTOR here.
I see. Ok. Keep the 2 ctors.
Attachment #8880219 - Flags: review?(amarchesini) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd8a6411e8d
PaymentRequestUpdateEvent interface and PaymentRequest API onshippingaddress/optionchange implementation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/922502d61ee2
Mochitest for PaymentRequestUpdateEvent and PaymentRequest API show(). r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acd8a6411e8d
https://hg.mozilla.org/mozilla-central/rev/922502d61ee2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I've updated the browser compat info for these features:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequestUpdateEvent (see subpages too)

I've also made sure it is clear that this API will only be available in secure contexts:

https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #20)
> I've updated the browser compat info for these features:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequestUpdateEvent
> (see subpages too)

One correction: updataWith returns void instead of a promise, per [1].
https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequestUpdateEvent/updateWith

[1] https://w3c.github.io/payment-request/#paymentrequestupdateevent-interface
(In reply to Ben Tian [:btian] from comment #21)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #20)
> > I've updated the browser compat info for these features:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequestUpdateEvent
> > (see subpages too)
> 
> One correction: updataWith returns void instead of a promise, per [1].
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequestUpdateEvent/
> updateWith
> 
> [1]
> https://w3c.github.io/payment-request/#paymentrequestupdateevent-interface

Ah, thanks! Updated.
You need to log in before you can comment on or make changes to this bug.