fix: convert PaymentMethodData.data#812
Conversation
|
Both Chrome and Firefox do the IDL conversion already. |
|
@domenic hopefully good now. |
cca5c77 to
e473522
Compare
|
@domenic, because in implementations,
This also fixes what would otherwise be a mess when So, with this PR, when a payment handler shows or gets updated, it always operates on its expected IDL type, without needing to redo any redundant JSON parsing, re-conversions, etc. |
domenic
left a comment
There was a problem hiding this comment.
Can you separate the editorial changes (removing lots of data-lts) from the normative ones, to make this easier to review? I think I found the big normative change and the issue with it, but it's hard to tell what else might be there.
| <var>serializedData</var>) to | ||
| <var>serializedMethodData</var>. | ||
| <li>Append <var>paymentMethod</var> to | ||
| <var>paymentMethods</var>. |
There was a problem hiding this comment.
So the problem with not storing the JSON serialized version is that the JSON serialization now becomes meaningless. Consider the following:
const bcr1 = { supportedNetworks: new Set("foo", "bar") };With the new spec, the JSON-serialization will be { "supportedNetworks": {} }. But the value stored in paymentMethods will be { supportedNetworks: ["foo", "bar"] } since Web IDL dictionary conversion converts all iterables to sequences.
This means that if someone, e.g., passed the JSON serialization across a process boundary and used it to attempt to reconstruct the result on the other side, they would not be able to match the spec. I thought our original spec was motivated exactly by that design.
There was a problem hiding this comment.
I believe the original design was motivated by some kind of security concern that was supposed to be mitigated by doing a JSON serialization on .data whose type is object to remove things like Function being added as properties.
There was a problem hiding this comment.
It may be good to check with implementations then. But a spec that JSON serializes then disregards the result seems incoherent.
There was a problem hiding this comment.
Gecko only does the IDL conversion, because we don't care about any payment method apart from "Basic Card". So, we don't store any [[serializedMethodData]] in our code... in fact, I don't see is doing any we JSON serialization of data at all in Gecko's code (@edenchuang?).
From the constructor, we check the method's details:
https://github.com/mozilla/gecko-dev/blob/master/dom/payments/PaymentRequest.cpp#L574
We then just check IsValidBasicCardRequest:
https://github.com/mozilla/gecko-dev/blob/master/dom/payments/PaymentRequest.cpp#L293
And then we convert it (to check throw if it fails):
https://github.com/mozilla/gecko-dev/blob/master/dom/payments/BasicCardPayment.cpp#L53
WebKit does do the serialization and stores it. However, @aestes also seems to suggest a preference to not store the JSON serialization: "Browsers could convert from a JS object directly to a IDL dictionary rather than the current JS object > JSON string > JS object > IDL dictionary conversion algorithm".
|
Went back to look at #346 ... I'd forgotten that the ideas was to support actually sending the JSON-serialized data no just across process boundaries (which I was imagining was just as per-origin process isolation) - but to other native applications acting as payment handlers. As such, I concur that it still makes sense to keep the JSON serialization. |
Closes #813
Part 1 of #753
Converts PaymentMethodData.data to IDL type or object.
The following tasks have been completed:
Implementation commitment:
Optional, Impact on Payment Handler spec?
Preview | Diff