1468356 - Implement paymentmethodchange event

Status: RESOLVED FIXED

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

Description

[Marcos Caceres [:marcosc]]

Details at: 
https://github.com/w3c/payment-request/pull/695

Comment 1

[Jean Gong :jgong]

Marcos, can you confirm whether you are going to implement this DOM work yourself?

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: Part 1 - Define PaymentRequest.prototype.onpaymentmethodchange handler

Hi Blake, to define an IDL event handler attribute, is there more that's needed than this (patch)? 

Just want to make sure before defining the actual event PaymentMethodChangeEvent.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986297 [details] [diff] [review]
Part 1 - Define PaymentRequest.prototype.onpaymentmethodchange handler

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

Yep, this looks good.

Comment 4

[Marcos Caceres [:marcosc]]

Ok, cool. Tests for attributes also passing...
https://w3c-test.org/payment-request/onpaymentmenthodchange-attribute.https.html

Have started work on the PaymentMethodChangeEvent itself.

Comment 5

[Marcos Caceres [:marcosc]]

Attachment: WIP patch...

Hey Blake.... Ok, here come the noob questions... :) Probably a lot wrong here, as I'm copy/pasting my way to C++ success.

Getting the following error ("base class has incomplete type"): 

```
 0:08.29 /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dist/include/mozilla/dom/PaymentMethodChangeEvent.h:16:40: error: base class has incomplete type
 0:08.30 class PaymentMethodChangeEvent final : PaymentRequestUpdateEvent
 0:08.30                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:08.30 /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dist/include/mozilla/dom/PaymentRequestUpdateEventBinding.h:19:7: note: forward declaration of 'mozilla::dom::PaymentRequestUpdateEvent'
 0:08.30 class PaymentRequestUpdateEvent;
 0:08.30       ^                                     ^~~~~~~~~~~~~~~~~~~~~~~~~
```

A separate question, `PaymentRequestUpdateEvent`'s class definition is marked as "final"... but I'm not sure if that means "final" in the same sense as in Java (that the class can't be extended)? 

Any guidance would be greatly appreciated.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marcos Caceres [:marcosc] from comment #5)
> Getting the following error ("base class has incomplete type"): 

Unlike Java, simply declaring a class isn't enough to use it in all situations in C++. You'll have to include the actual header file to be able to inherit from PaymentRequestUpdateEvent. So instead of

class PaymentRequestUpdateEvent; // forward declaration, introduces the name without any info

you'll need:

#include "PaymentRequestUpdateEvent.h"


> A separate question, `PaymentRequestUpdateEvent`'s class definition is
> marked as "final"... but I'm not sure if that means "final" in the same
> sense as in Java (that the class can't be extended)? 

It does. Because you're inheriting from it now, you'll need to un-mark it as final (once you #include its header, the compiler will be able to see that it's marked as final and should tell you something similar).

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986319 [details] [diff] [review]
WIP patch...

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

Along with my comment, there are a couple of nitpicks to deal with.

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +22,5 @@
> +
> +already_AddRefed<PaymentMethodChangeEvent>
> +PaymentMethodChangeEvent::Constructor(mozilla::dom::EventTarget* aOwner,
> +                                       const nsAString& aType,
> +                                       const PaymentMethodChangeEventInit& aEventInitDict)

Nit (here and below): these lines should line up with the 'm' in mozilla.

@@ +44,5 @@
> +}
> +
> +PaymentMethodChangeEvent::PaymentMethodChangeEvent(EventTarget* aOwner)
> +  : PaymentRequestUpdateEvent(aOwner, nullptr, nullptr)
> +    , mRequest(nullptr)

Nit: the , should line up with the : above it.

Comment 8

[Marcos Caceres [:marcosc]]

Attachment: WIP nits and setters

Um, I'm getting some errors I can't figure out related to the macros (e.g., NS_IMPL_CYCLE_COLLECTION_INHERITED). 

I've pasted the build output here: https://pastebin.com/xUhPc3vx

Blake, would appreciate your guidance again as I'm not sure how any of the macros work. 

Thanks in advance!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986553 [details] [diff] [review]
WIP nits and setters

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

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(PaymentMethodChangeEventInit, PaymentRequestUpdateEvent, Event, mRequest)

Oops, where'd that Init come from? This should be for PaymentMethodChangeEvent!

@@ +42,5 @@
> +  return Constructor(owner, aType, aEventInitDict);
> +}
> +
> +PaymentMethodChangeEvent::PaymentMethodChangeEvent(EventTarget* aOwner)
> +  : PaymentMethodChangeEvent(aOwner)

Another typo: this should be calling the base class constructor: PaymentMethodUpdateEvent

@@ +50,5 @@
> +
> +void
> +PaymentMethodChangeEvent::SetMethodName(nsAutoString* aMethodName)
> +{
> +  mMethodName = aMethodName;

Some food for thought since you're coming from Java. In Java, there are no explicit pointers and when you pass objects around, the Right Thing (TM) happens. This works because it's a GC'd language. In C++, however, we have to worry about ownership. If somebody passes a pointer to you and you then store that pointer, when does the object pointed to by that pointer get deleted? In Gecko, we use reference counting (and the cycle collector) as a form of GC for objects. That doesn't exist for strings, so when looking at the code here, we'd have to worry about ownership. Fortunately, you'll be holding nsString objects directly in PaymentMethodChangeEvent and you should be taking nsAString objects by const reference here, so we don't actually have to worry too much about this sort of stuff yet.

::: dom/payments/PaymentMethodChangeEvent.h
@@ +37,5 @@
> +              const nsAString& aType,
> +              const PaymentMethodChangeEventInit& aEventInitDict,
> +              ErrorResult& aRv);
> +
> +  void SetMethodName(nsAutoString* aMethodName);

See below for more information why, but these two methods will probably want to be:

void SetMethod...(const nsAString& aMethod...);

@@ +45,5 @@
> +  ~PaymentMethodChangeEvent();
> +
> +private:
> +  nsAutoString* mMethodDetails;
> +  nsAutoString* mMethodName;

A couple of things to note here:

In C++, class members can be broken into three broad categories (I'm oversimplifying a bit): primitives types (integers, chars, booleans), strings, and interface objects. By and large, primitive types and strings should be held directly by the class and the only objects that we'd want pointers to would be interface objects. Since these are strings, we'll want to be holding nsString members here. [1] has a ton more information about what string classes to use when.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings

Comment 10

[Marcos Caceres [:marcosc]]

Attachment: WIP - const, copy by value, etc.

Ok, getting closer

Comment 11

[Marcos Caceres [:marcosc]]

My comment got chopped because I included an emoji :) ... thanks again for the help, Blake. The explanation about primitives makes sense - and I was also wondering about it.

Having another compile error that I can't figure out (full output https://pastebin.com/At0QN1Kg). 

 0:11.92 In file included from /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dom/payments/Unified_cpp_dom_payments0.cpp:47:
 0:11.93 /Users/marcos/gecko/dom/payments/PaymentRequest.cpp:946:21: error: cannot initialize a parameter of type 'mozilla::dom::payments::PaymentRequest *' with an rvalue of type 'mozilla::dom::PaymentRequest *'
 0:11.93   event->SetRequest(this);
 0:11.93                     ^~~~
 0:11.93 /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dist/include/mozilla/dom/PaymentRequestUpdateEvent.h:53:35: note: passing argument to parameter 'aRequest' here
 0:11.93   void SetRequest(PaymentRequest* aRequest);

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8987158 [details] [diff] [review]
WIP - const, copy by value, etc.

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

Definitely closer :)

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +14,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(PaymentMethodChangeEvent, PaymentRequestUpdateEvent)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PaymentRequestUpdateEvent)
> +NS_INTERFACE_MAP_END_INHERITING(Event)

This was copy/pasted and not updated (the class name is wrong and the inheriting class name is wrong).

::: dom/payments/PaymentMethodChangeEvent.h
@@ +39,5 @@
> +              ErrorResult& aRv);
> +
> +  void SetMethodName(const nsAString& aMethodName);
> +
> +  nsString GetMethodName();

This method should return void and take an `nsAString& aMethodName` as an out parameter (this is for now mostly historical reasons to do with copying the strings too many times).

@@ +41,5 @@
> +  void SetMethodName(const nsAString& aMethodName);
> +
> +  nsString GetMethodName();
> +
> +  void SetMethodDetails(const nsAString& aMethodDetails);

These two functions are going to look different because they don't take a string, they actually take an object. I guess you'll come to that when you get there?

@@ +49,5 @@
> +protected:
> +  ~PaymentMethodChangeEvent();
> +
> +private:
> +  nsString mMethodDetails;

This is going to end up being a JS::Heap<JS::Object>, not a string.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marcos Caceres [:marcosc] from comment #11)
> 'mozilla::dom::payments::PaymentRequest *' with an rvalue of type
> 'mozilla::dom::PaymentRequest *'

I'm pretty sure this is a unified compilation error. The best fix for it is to add a forward declaration of PaymentRequest just before the class Promise in PaymentRequestUpdateEvent.h. The problem is that there are two PaymentRequest classes, one that's mozilla::dom::PaymentRequest and one that's mozilla::dom::payments::PaymentRequest. With the new configuration of files, PaymentRequestUpdateEvent.h is only seeing payments::PaymentRequest, which is the wrong one. We have to tell the file about the one in mozilla::dom so that it does the right thing.

Comment 14

[Marcos Caceres [:marcosc]]

Comment on attachment 8987158 [details] [diff] [review]
WIP - const, copy by value, etc.

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

couple of followup questions. Really appreciate the help so far - learning a lot!

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +14,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(PaymentMethodChangeEvent, PaymentRequestUpdateEvent)
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PaymentRequestUpdateEvent)
> +NS_INTERFACE_MAP_END_INHERITING(Event)

As I don't actually know what these macros do, I'm unsure about what values to plug into them. I've updated them to:

```
NS_IMPL_CYCLE_COLLECTION_INHERITED(PaymentMethodChangeEvent,
                                   PaymentRequestUpdateEvent)
NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(PaymentMethodChangeEvent,
                                               PaymentRequestUpdateEvent)
NS_IMPL_CYCLE_COLLECTION_TRACE_END

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PaymentMethodChangeEvent)
NS_INTERFACE_MAP_END_INHERITING(PaymentRequestUpdateEvent)
```

Is that correct?

::: dom/payments/PaymentMethodChangeEvent.h
@@ +39,5 @@
> +              ErrorResult& aRv);
> +
> +  void SetMethodName(const nsAString& aMethodName);
> +
> +  nsString GetMethodName();

Interesting (the void part), I thought this would have been like a regular getter/setter in JS or in Java. 

So, when `GetMethodName()` is called, how does a caller get the requested value?

@@ +49,5 @@
> +protected:
> +  ~PaymentMethodChangeEvent();
> +
> +private:
> +  nsString mMethodDetails;

Compiler claims there is no such thing as `JS::Heap<JS::Object>`. I'm guessing you mean `JS::Heap<JS::Value>`(I see that used for CustomEvent's `detail` object)?

Comment 15

[Marcos Caceres [:marcosc]]

(In reply to Blake Kaplan (:mrbkap) from comment #13)
> We have to tell the file about the one in
> mozilla::dom so that it does the right thing.

Worked a treat :)

Comment 16

[Marcos Caceres [:marcosc]]

I spend a bit of time reading:
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

it seems to suggest that the function declarations for the attributes should be (doesn't actually work for me): 

```
  void MethodName();
  // no set for readonly

  void GetMethodDetails(JSContext* cx, JS::MutableHandle<JSObject*> retval);
  void SetMethodDetails(JSContext* cx, JS::Handle<JSObject*> value);
```

When I try to set:

```
private:
  JS::Handle<JSObject*> mMethodDetails;
```

It gets upset:

> error: field of type 'JS::Handle<JSObject *>' has private default constructor

PS/General feedback for DOM Team: could we work on updating WebIDL_bindings wiki page? The Attribute section is missing examples, and it would be nice to have not just the header file description for each, but also the .cpp file part to.

The Attribute section states that that, "Get" prefix is needed if:

> 3. The return value of the attribute is returned via an out parameter in the C++.

However, it doesn't explain what an "out parameter" is.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marcos Caceres [:marcosc] from comment #14)
> As I don't actually know what these macros do, I'm unsure about what values
> to plug into them. I've updated them to:

It isn't great, but if you aren't sure, a glance at the source [1] never hurts. The first parameter to all of these macros is the C++ class that your invoking them for and the ones that have _INHERITING will always take the base class after that.

> 
> ```
> NS_IMPL_CYCLE_COLLECTION_INHERITED(PaymentMethodChangeEvent,
>                                    PaymentRequestUpdateEvent)

You'll want to pass your details object here eventually.

> Is that correct?

It looks good to me.

> So, when `GetMethodName()` is called, how does a caller get the requested
> value?

The caller will pass its own string in that you'll assign to. The important difference between the getter and the setter is a 'const'. the getter takes a non-const reference to an nsAString (`nsAString&`), so it can do

aMethodName.Assign(mMethodName);

The caller will have some code:

nsAutoString methodName;
methodChange->GetMethodName(methodName);

The reasons for this are mostly historical at this point, but we already have thousands of lines of code written this way, so consistency outweighs sanity, or something.

> Compiler claims there is no such thing as `JS::Heap<JS::Object>`. I'm
> guessing you mean `JS::Heap<JS::Value>`(I see that used for CustomEvent's
> `detail` object)?

Oops, I had too many ::s, I meant JS::Heap<JSObject*>. But JS::Value works too, it's just a bit more general: you could stick a string or number in the value instead of an object.


(In reply to Marcos Caceres [:marcosc] from comment #16)
> I spend a bit of time reading:
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
> 
> it seems to suggest that the function declarations for the attributes should
> be (doesn't actually work for me): 
> 
> ```
>   void MethodName();

I wasn't clear before, sorry. This will be:

void GetMethodName(nsAString& aMethodName);

and you'll assign to it from mMethodName in the implementation.

>   JS::Handle<JSObject*> mMethodDetails;

This stuff is unfortunately quite complicated. There are many classes to understand. You can see the code/source documentation at [2]. JS::Handle is only constructed by another one of the rooting class, it's a temporary reference that will only exist for the duration of a call. Because your class is on the heap (allocated by new), you'll want to use a JS::Heap<JSObject*>, which you can construct.

> PS/General feedback for DOM Team: could we work on updating WebIDL_bindings
> wiki page? The Attribute section is missing examples, and it would be nice
> to have not just the header file description for each, but also the .cpp
> file part to.

I don't know what the right forum for this request is. Maybe post to mozilla.dev.platform to get more eyes on it?

> 
> The Attribute section states that that, "Get" prefix is needed if:
> 
> > 3. The return value of the attribute is returned via an out parameter in the C++.
> 
> However, it doesn't explain what an "out parameter" is.

An out parameter is when you take a named parameter either by pointer (*) or by reference (&) and return your data through it rather than via the return value of the function. For strings, we will always use references (again, for historical reasons). It's kind of a general term used in C++ development.

[1] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/xpcom/base/nsCycleCollectionParticipant.h#945
[2] https://searchfox.org/mozilla-central/source/js/public/RootingAPI.h

Comment 18

[Marcos Caceres [:marcosc]]

Attachment: Addressed additional feedback

(ignoring white space - files are auto-formatted by my editor).

Ok, this is where I've ended up :) Starting to get it a bit more now. 

The thing about assigning by pointer or reference was the bit of information I was missing. I need to look at the actual API for assigning then. 

I'm still a bit unsure, does the binding layer handle the actual assignment or do I need to do it?

Lastly, getting one error now about `JS::Handle<JSObject *>`. 

```
 0:09.56 In file included from /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dom/payments/Unified_cpp_dom_payments0.cpp:38:
 0:09.56 /Users/marcos/gecko/dom/payments/PaymentMethodChangeEvent.cpp:51:27: error: field of type 'JS::Handle<JSObject *>' has private default constructor
 0:09.56 PaymentMethodChangeEvent::PaymentMethodChangeEvent(EventTarget* aOwner)
 0:09.56                           ^
 0:09.56 /Users/marcos/gecko/obj-x86_64-apple-darwin17.6.0/dist/include/js/RootingAPI.h:580:5: note: declared private here
 0:09.56     Handle() {}
 0:09.56     ^
 0:12.07 1 error generated.
```

Comment 19

[Marcos Caceres [:marcosc]]

> I wasn't clear before, sorry. This will be:
> void GetMethodName(nsAString& aMethodName);

Just wonder then, are the IDL docs wrong? They insist in a number of places that I don't need Get*. 

In parallel to this, I've started writing a WebIDL to C++ code generator (in JS, of course) so to avoid this dance in the future (or to get as much for free as possible). I'm basing the JS implementation on the MDN WebIDL docs... so want to make sure the docs are right.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marcos Caceres [:marcosc] from comment #19)
> Just wonder then, are the IDL docs wrong? They insist in a number of places
> that I don't need Get*. 

You quoted the reason for this in comment 16:

> The Attribute section states that that, "Get" prefix is needed if:
> 
> > 3. The return value of the attribute is returned via an out parameter in the C++.

(In reply to Marcos Caceres [:marcosc] from comment #18)
> Lastly, getting one error now about `JS::Handle<JSObject *>`. 

The member variable needs to be a JS::Heap<JSObject*>. Handles are only for function parameters.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8987705 [details] [diff] [review]
Addressed additional feedback

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

We're getting closer.

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +59,1 @@
>  {

Here's where you have to pass out the name:

aMethodName.Assign(mMethodName);

because aMethodName is passed by reference, this changes the caller's object too.

@@ +71,1 @@
>  {

And here's where you need to pass out the details object:

retval.set(mDetails.get());

::: dom/payments/PaymentMethodChangeEvent.h
@@ +44,4 @@
>    void SetMethodName(const nsAString& aMethodName);
>  
> +  void GetMethodDetails(JSContext* cx, JS::MutableHandle<JSObject*> retval);
> +  void SetMethodDetails(JSContext* cx, JS::Handle<JSObject*> value);

I just realized that you don't actually need either Set* function because both of the attributes are readonly.

@@ +49,5 @@
>  protected:
>    ~PaymentMethodChangeEvent();
>  
>  private:
> +  JS::Handle<JSObject*> mMethodDetails;

JS::Heap

Comment 22

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1468356-implement-onpaymentmethodchange-attribut.patch

Seems to work :)

Comment 23

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1468356-implement-onpaymentmethodchange-attribut.patch

Updated based on discussion with qDot about managing the JSObject.

However, it's still crashing when I try to access the methodName: 
http://w3c-test.org/payment-request/PaymentMethodChangeEvent/methodName-attribute.https.html 

just accessing the methodDetails is fine tho.
http://w3c-test.org/payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html

Any ideas of what I might be missing?

Comment 24

[Marcos Caceres [:marcosc]]

(sorry, I don't know where to get the crash log... which I know would help... I think my build is also not a debug build right now.)

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marcos Caceres [:marcosc] from comment #24)
> (sorry, I don't know where to get the crash log... which I know would
> help... I think my build is also not a debug build right now.)

It'll definitely be useful for you to build a debug build. If you did, it would help point almost directly at the problem:

Assertion failure: mIsSome, at /home/mrbkap/work/main/firefox/dist/include/mozilla/Maybe.h:589

Looking under a debugger:

#9  0x00007ff3fdbf148f in mozilla::dom::PaymentMethodChangeEvent::init (this=0x7ff3f34160d0, 
    aEventInitDict=...) at /home/mrbkap/work/main/mozilla/dom/payments/PaymentMethodChangeEvent.cpp:74
74	  mMethodDetails = aEventInitDict.mMethodDetails.Value();

and, indeed, methodDetails is an optional parameter, so we have to do something useful/sane in the case that !aEventInitDict.mMethodDetails.WasPassed() (but mMethodDetails is initialized to nullptr for you, so you don't actually have to do anything special).

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988049 [details] [diff] [review]
0001-Bug-1468356-implement-onpaymentmethodchange-attribut.patch

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

Try retesting with this fixed (preferably in a debug build) and let's see how it looks.

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +70,5 @@
> +PaymentMethodChangeEvent::init(
> +  const PaymentMethodChangeEventInit& aEventInitDict)
> +{
> +  mMethodName.Assign(aEventInitDict.mMethodName);
> +  mMethodDetails = aEventInitDict.mMethodDetails.Value();

mMethodDetails might not have been passed.

Comment 27

[Marcos Caceres [:marcosc]]

Attachment: Worky! needed member was passed check.

Worky now :) Do I need super review for the WebIDL additions from bholly or Bz?

Comment 28

[Marcos Caceres [:marcosc]]

Note to self, add so it remains pref'ed off for now:

[Func="mozilla::dom::PaymentRequest::PrefEnabled"]

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988615 [details] [diff] [review]
Worky! needed member was passed check.

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

r=me with the Func= added.

Comment 30

[Marcos Caceres [:marcosc]]

Attachment: Add the Func

Carrying over r+.

Comment 31

[Marcos Caceres [:marcosc]]

Attachment: Accidentally super reviewed r- myself, lol... forms are hard.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bad07995b93
Implement onpaymentmethodchange attribute. r=mrbkap

Comment 33

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5bad07995b93

Comment 34

[Eric Shepherd [:sheppy]]

I've updated the documentation for this. It would be great if I can get a review of these changes, but unless I hear otherwise, I consider this complete, barring any needed corrections to the two PRs.

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Payment_Request_API
https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest
https://developer.mozilla.org/en-US/docs/Web/API/PaymentMethodChangeEvent
https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/onpaymentmethodchange
https://developer.mozilla.org/en-US/docs/Web/API/PaymentResponse/methodName

Documentation added:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentMethodChangeEvent/methodName
https://developer.mozilla.org/en-US/docs/Web/API/PaymentMethodChangeEvent/methodDetails
https://developer.mozilla.org/en-US/docs/Web/Events/paymentmethodchange

Updated Firefox 63 for developers.

Submitted PR #807 to the MDN KumaScript repo to update the interface and group data JSON files: https://github.com/mdn/kumascript/pull/807

Submitted PR #2769 to the BCD repo to add the compatibility updates for this: https://github.com/mdn/browser-compat-data/pull/2769

Comment 35

[Marcos Caceres [:marcosc]]

Thanks for the update, :sheppy! I'll review them.

Comment 36

[Marcos Caceres [:marcosc]]

Ok, finished reviewing. Made some small editorial changes. Looking really good tho!

Comment 37

[Boris Zbarsky [:bzbarsky]]

Why does the webidl file added here not have a spec link in it?  All spec-derived webidl should have the relevant spec link in it.

Comment 38

[Marcos Caceres [:marcosc]]

Just an oversight. Will send a new patch to add it.

Comment 39

[Marcos Caceres [:marcosc]]

closing loop on Comment 37:
https://bugzilla.mozilla.org/show_bug.cgi?id=1491203

Comment 40

[[github robot]]

Commits pushed to master at https://github.com/mdn/kumascript

https://github.com/mdn/kumascript/commit/db390786db589825adb1d6215bbf4dd5da4b868a
Bug 1468356 - paymentmethodchange event
Adds various interfaces for Payment Request API to
help support the paymentchangeevent addition in Firefox
63. Also includes some interfaces and dictionaries
previously left out.

https://github.com/mdn/kumascript/commit/3f29f11e2c32d32855ed70f9f115eb5f1393ce2d
Merge pull request #807 from a2sheppy/bug1468356-paymentmethodchange

Bug 1468356 - paymentmethodchange event