1318990 - Implement PaymentRequestUpdateEvent Interface

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect, P3)

Description

[Marcos Caceres [:marcosc]]

The PaymentRequestUpdateEvent enables the web page to update the details of the payment request in response to a user interaction

Comment 2

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

Mapping the Trello_P1 priority into bugzilla P3.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1318990 - PaymentRequestUpdateEvent implementation and PaymentRequest API onshippingaddress/optionchange implementation. r?baku

PaymentRequestUpdateEvent implementation and also support PaymentRequest API onshippingaddress/optionchange.

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Alphan Chen [:alchen]]

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

Comment 6

[Alphan Chen [:alchen]]

(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()"?

Comment 7

[Andrea Marchesini [:baku]]

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?

Comment 8

[Alphan Chen [:alchen]]

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.

Comment 9

[Andrea Marchesini [:baku]]

I see. Ok. Keep the 2 ctors.

Comment 10

[Eden Chuang[:edenchuang]]

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

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Bug 1318990 - PaymentRequestUpdateEvent implementation and PaymentRequest API onshippingaddress/optionchange implementation. r=baku

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Bug 1318990 - Mochitest for PaymentRequestUpdateEvent and PaymentRequest show(). r?baku

Comment 13

[Eden Chuang[:edenchuang]]

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

Comment 14

[Eden Chuang[:edenchuang]]

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

Comment 15

[Eden Chuang[:edenchuang]]

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

Comment 16

[Eden Chuang[:edenchuang]]

Attachment: Bug 1318990 - PaymentRequestUpdateEvent implementation and PaymentRequest API onshippingaddress/optionchange implementation. r=baku

Comment 17

[Eden Chuang[:edenchuang]]

Attachment: Bug 1318990 - Mochitest for PaymentRequestUpdateEvent and PaymentRequest show(). r=baku

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acd8a6411e8d
https://hg.mozilla.org/mozilla-central/rev/922502d61ee2

Comment 20

[Chris Mills [:cmills]]

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

Comment 21

[Ben Tian (inactive)]

(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

Comment 22

[Chris Mills [:cmills]]

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