1487295 - Add defaults to PaymentMethodChangeEventInit's members

Status: RESOLVED FIXED

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

Comment 1

[Marcos Caceres [:marcosc]]

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

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: Adds defaults and additional WPT tests

Comment 3

[Marcos Caceres [:marcosc]]

Oh, looks like I need to update some stuff in the c++. I'll send another patch on monday, as I'm out of time now :(

Comment 4

[Marcos Caceres [:marcosc]]

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

Comment 5

[Marcos Caceres [:marcosc]]

Attachment: Fixed binding issue

Eden, could you quickly just check this again. I was a little unsure if assigning `aEventInitDict.mMethodDetails` like this is ok:

```
void
PaymentMethodChangeEvent::init(
  const PaymentMethodChangeEventInit& aEventInitDict)
{
  mMethodName.Assign(aEventInitDict.mMethodName);
  mMethodDetails = aEventInitDict.mMethodDetails;
}
```

Or do I need to copy the passed mMethodDetails?

Comment 6

[Eden Chuang[:edenchuang]]

Comment on attachment 9006146 [details] [diff] [review]
Fixed binding issue

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

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +72,3 @@
>    const PaymentMethodChangeEventInit& aEventInitDict)
>  {
>    mMethodName.Assign(aEventInitDict.mMethodName);

Since required keyword is removed in webidl. mMethodName might also be wrapped by Optonal<>.

That means aEventInitDict.mMethodName is not a nsString anymore. you should do like this

if (aEventInitDict.mMethodName.WasPassed()) {
  mMethodName.Assign(aEventInitDict.mMethodName.Value());
// or mMethodName = aEventInitDict.mMethodName.Value();
}

@@ +72,4 @@
>    const PaymentMethodChangeEventInit& aEventInitDict)
>  {
>    mMethodName.Assign(aEventInitDict.mMethodName);
> +  mMethodDetails = aEventInitDict.mMethodDetails;

Maybe compile error here.
According to the declaration of PaymentMethodChangeEventInit, mMethodDetails is still an optional value. That means data would be wrapped by Optional<> structure.

So this assignment might cause compile error by the type difference between lhs and rhs of assignment.

Might keep the original way to assign and assign null value in the else statement for the default condition.

if (aEventInitDist.mMethodDetails.WasPassed()) {
  mMethodDetails = aEventInitDict.mMethodDetails.Value();
} else {
  mMethodDetails = JS::NullValue();
}

Comment 7

[Eden Chuang[:edenchuang]]

Comment on attachment 9006146 [details] [diff] [review]
Fixed binding issue

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

::: dom/payments/PaymentMethodChangeEvent.cpp
@@ +72,4 @@
>    const PaymentMethodChangeEventInit& aEventInitDict)
>  {
>    mMethodName.Assign(aEventInitDict.mMethodName);
> +  mMethodDetails = aEventInitDict.mMethodDetails;

Didn't notice the default value is set in webidl. the member is not wrapped by Optional<>

Comment 8

[Marcos Caceres [:marcosc]]

DevDocs folks, the things that need to be documented on MDN are:

PaymentMethodChangeEvent interface:
https://w3c.github.io/payment-request/#dom-paymentmethodchangeevent

PaymentMethodChangeEventInit Dictionary:
https://w3c.github.io/payment-request/#dom-paymentmethodchangeeventinit

Thanks so much!!!! 💖

Comment 9

[Narcis Beleuzu [:NarcisB]]

There were conflicts applying your patch. Please fix and attach the new version.

Comment 10

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1487295-Add-defaults-to-PaymentMethodChangeEvent.patch

Rebased patch

Comment 11

[Marcos Caceres [:marcosc]]

Comment on attachment 9006146 [details] [diff] [review]
Fixed binding issue

Baku, can you review the IDL changes? Need DOM Peer ok to land.

Comment 12

[Marcos Caceres [:marcosc]]

Removing "checkin needed" until I get ok from Baku.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d509458e63
Add defaults to PaymentMethodChangeEventInit's members. r=baku

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12832 for changes under testing/web-platform/tests

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63bcec434ac
Backed out changeset f7d509458e63 for failing at PaymentMethodChangeEvent/methodDetails-attribute.https.html on a CLOSED TREE

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 18

[Marcos Caceres [:marcosc]]

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

Comment 19

[Marcos Caceres [:marcosc]]

Attachment: Enable web payments on the PaymentMethodChangeEventInit directory

Forgot to enable the the pref on the directory, which caused the wpt tests to fail.

Comment 20

[Marcos Caceres [:marcosc]]

Attachment: Rebased

Comment 21

[Marcos Caceres [:marcosc]]

The __dir__.ini is in the wrong place.

Comment 22

[Marcos Caceres [:marcosc]]

Attachment: Move ini file to right place.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5657f72bcb0
Add defaults to PaymentMethodChangeEventInit's members. r=baku

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33cb5cf879d8
Add defaults to PaymentMethodChangeEventInit's members: update wpt expectations. r=wpt-fix

Comment 26

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1487295) for WPT failres in payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=197991139&repo=mozilla-inbound&lineNumber=33954

INFO - TEST-START | /payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html
09:22:41     INFO - Setting pref dom.payments.request.enabled (true)
09:22:41     INFO - PID 2844 | ++DOMWINDOW == 5 (000001340DB6D400) [pid = 1340] [serial = 5] [outer = 0000013405A13800]
09:22:42     INFO - PID 2844 | ++DOCSHELL 000001340D29A000 == 2 [pid = 1340] [id = {d969d1b9-471d-4788-ad74-f1f88e4cf851}]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 6 (0000013405A19C00) [pid = 1340] [serial = 6] [outer = 0000000000000000]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 7 (000001340DB6C400) [pid = 1340] [serial = 7] [outer = 0000013405A19C00]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 8 (000001340DB71C00) [pid = 1340] [serial = 8] [outer = 0000013405A19C00]
09:22:42     INFO - PID 2844 | ++DOCSHELL 000001340DBDB800 == 3 [pid = 1340] [id = {2ad150ea-c818-48b3-8db1-557b08e87b9d}]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 9 (000001340DBB5600) [pid = 1340] [serial = 9] [outer = 0000000000000000]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 10 (000001340DB75C00) [pid = 1340] [serial = 10] [outer = 000001340DBB5600]
09:22:42     INFO - PID 2844 | ++DOMWINDOW == 11 (000001340E186400) [pid = 1340] [serial = 11] [outer = 000001340DBB5600]
09:22:42     INFO - 
09:22:42     INFO - TEST-UNEXPECTED-PASS | /payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html | Must have a methodDetails IDL attribute, which is initialized with to the methodName dictionary value - expected FAIL
09:22:42     INFO - TEST-INFO | expected FAIL
09:22:42     INFO - 
09:22:42     INFO - TEST-UNEXPECTED-PASS | /payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html | The methodDetails member defaults to null - expected FAIL
09:22:42     INFO - TEST-INFO | expected FAIL
09:22:42     INFO - TEST-OK | /payment-request/PaymentMethodChangeEvent/methodDetails-attribute.https.html | took 1057ms

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b5657f72bcb0e61975c9976bfde86063c96b2159

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81df02700c05c63cd2590a6d06a7b2d5ead0ec26

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 28

[Marcos Caceres [:marcosc]]

Hi James, can you provide me with some guidance here. Test passes locally, but then times out? I tried following the mdn page about wpt tests but clearly I’ve missed something.

Comment 29

[James Graham [:jgraham]]

I can't see the test timing out locally; are you sure you're not confused because if you only run a single test the browser window remains open for debugging by default? In any case the failures on inbound were UNEXPECTED-PASS, not UNEXPECTED-TIMEOUT.

Unpicking what happened here; [1] is a full run with both the original patch and Aryx's fixup, without SETA obscuring things. It looks like these tests are passing everywhere except linux32 debug. I have no idea why that would be but you either need to figure it out and fix the problem, or update the metadata so that the tests are marked as

expected:
  if debug and os == "linux" and bits == 32: FAIL

In either case I recommend doing a try run to validate your changes ./mach try fuzzy testing/web-platform/tests/payment-request/ --artifact -q '!pgo !asan' will probably give the right jobs.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=69f608b5cdc67f0af6b8a65285fce0f360370dcc&filter-searchStr=web-platform-tests

Comment 30

[Marcos Caceres [:marcosc]]

Thanks, James. Apologies, I should have spotted to Linux fails... that was not expected at all given that this are only small IDL changes, and not platform related things.

Will try what you suggested.

Comment 31

[Marcos Caceres [:marcosc]]

Attachment: Remove WTP tests

Discussed with :edenchuang and we decided it was simpler to go through the traditional WPT channel. I sent a PR for the tests here:

https://github.com/web-platform-tests/wpt/pull/12927

We can then enable the WP tests incrementally in Gecko.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bd71930373
Add defaults to PaymentMethodChangeEventInit's members. r=baku

Comment 33

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/45bd71930373

Comment 34

[Eric Shepherd [:sheppy]]

Documentation updated:

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