Closed Bug 1487295 Opened Last year Closed Last year

Add defaults to PaymentMethodChangeEventInit's members

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

No description provided.
Priority: -- → P3
Summary: PaymentMethodChangeEventInit methodName member should default to "" → Add defaults to PaymentMethodChangeEventInit's members
Assignee: nobody → mcaceres
Status: NEW → ASSIGNED
Priority: P3 → P1
Attachment #9005552 - Flags: review?(echuang)
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 :(
Attachment #9005552 - Flags: review?(echuang) → review+
Attached patch Fixed binding issue (obsolete) — Splinter Review
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?
Attachment #9005552 - Attachment is obsolete: true
Attachment #9006146 - Flags: review?(echuang)
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();
}
Attachment #9006146 - Flags: review?(echuang) → review-
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<>
Attachment #9006146 - Flags: review- → review+
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!!!! 💖
There were conflicts applying your patch. Please fix and attach the new version.
Flags: needinfo?(mcaceres)
Rebased patch
Attachment #9006146 - Attachment is obsolete: true
Flags: needinfo?(mcaceres)
Comment on attachment 9006146 [details] [diff] [review]
Fixed binding issue

Baku, can you review the IDL changes? Need DOM Peer ok to land.
Attachment #9006146 - Flags: superreview?(amarchesini)
Removing "checkin needed" until I get ok from Baku.
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d509458e63
Add defaults to PaymentMethodChangeEventInit's members. r=baku
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12832 for changes under testing/web-platform/tests
Backout by rgurzau@mozilla.com:
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
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Forgot to enable the the pref on the directory, which caused the wpt tests to fail.
Attachment #9006177 - Attachment is obsolete: true
Attachment #9007119 - Flags: review+
Attached patch Rebased (obsolete) — Splinter Review
Attachment #9007119 - Attachment is obsolete: true
Attachment #9007121 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
The __dir__.ini is in the wrong place.
Attached patch Move ini file to right place. (obsolete) — Splinter Review
Attachment #9007121 - Attachment is obsolete: true
Attachment #9007137 - Flags: review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5657f72bcb0
Add defaults to PaymentMethodChangeEventInit's members. r=baku
Keywords: checkin-needed
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33cb5cf879d8
Add defaults to PaymentMethodChangeEventInit's members: update wpt expectations. r=wpt-fix
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
Flags: needinfo?(mcaceres)
Upstream PR was closed without merging
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.
Flags: needinfo?(mcaceres) → needinfo?(james)
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
Flags: needinfo?(james)
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.
Attached patch Remove WTP testsSplinter Review
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.
Attachment #9007137 - Attachment is obsolete: true
Attachment #9007742 - Flags: review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bd71930373
Add defaults to PaymentMethodChangeEventInit's members. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45bd71930373
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Attachment #9006146 - Flags: superreview?(amarchesini)
You need to log in before you can comment on or make changes to this bug.