Fix parsing of our promises to note they're nullable. #1293
Conversation
|
Note: the renderered diff link doesn't show the WebIDL changes. |
spec.bs
Outdated
| @@ -1346,13 +1353,14 @@ To <dfn>validate and convert auction ad config</dfn> given an {{AuctionAdConfig} | |||
| * To parse the value |result|: | |||
| 1. Set |auctionConfig|'s [=auction config/per buyer signals=] to a new [=ordered map=] whose | |||
There was a problem hiding this comment.
so it always sets |auctionConfig|'s [=auction config/per buyer signals=] to a map, and not setting it to null when |result| is null. In our code, we have optional for these fields (example: per_buyer_currencies), but seems no much difference, right? (if so, seems currently in spec these map fields can never be null, where their definitions say they could)
There was a problem hiding this comment.
I think I am just wrong here, and we do pass null through --- not sure why I misread code that way. Thanks!
(Will get back to this in a bit, need to finish other task first)
There was a problem hiding this comment.
Changed. Looks like deprecatedRenderURLReplacement doesn't do nullability, though.
The difference is whether we add the field when we serialize the config back to seller worklet or not.
So observable, but somewhat less important than what motivated this CL --- saying that resolving these promises with null or undefined (rather than empty objects) when you want nothing is acceptable.
There was a problem hiding this comment.
is it worthwhile changing deprecatedRenderURLReplacement's implementation?
spec.bs
Outdated
| given |value|. | ||
| 1. [=map/Set=] |auctionConfig|'s [=auction config/per buyer signals=][|buyer|] to | ||
| |signalsString|. | ||
| 1. If |result| is not null: |
There was a problem hiding this comment.
Seems we need to set |auctionConfig|'s [=auction config/per buyer signals=] to null when |result| is null explicitly, since it has no default value.
There was a problem hiding this comment.
... probably need to set it to that on all regardless of default because we set it to the promise first.
SHA: feb9171 Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Fix parsing of our promises to note they're nullable. * Propagate nulls further. * Actually set the nulls. --------- Co-authored-by: Maks Orlovich <[email protected]>
(That we were using the wrong types here got exposed by some improvements to Chrome's WebIDL impl;
see also https://chromium-review.googlesource.com/c/chromium/src/+/5907125)
Preview | Diff