Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Spec: Allow updating userBiddingSignals. #907

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Spec: Allow updating userBiddingSignals. #907

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b25313
update spec to reflect an update ig change.
HabibiYou Nov 15, 2023
e7be105
spec change
HabibiYou Nov 16, 2023
4b0ba72
add proper issue block
HabibiYou Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions spec.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3631,6 +3631,13 @@ The <dfn for=Navigator method>updateAdInterestGroups()</dfn> method steps are:
set |ig|'s [=interest group/trusted bidding signals keys=] to |value|.
1. Otherwise, jump to the step labeled <i><a href=#abort-update>Abort update</a></i>.

<dt>"`userBiddingSignals`"
<dd>
1. If |value| is a [=string=],
set |ig|'s [=interest group/user bidding signals=] to the result of [=parse a JSON
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem here is that parse a JSON string to an Infra value expects to be called within a valid ES realm (since it can throw errors, and it operates on JS objects). Unfortunately, this algorithm mostly runs in parallel (while at least one time, it does not). That doesn't seem great, although I realize this algorithm already has this bug from several lines above where we already call this algorithm.

This is in contrast with the Chromium implementation, which uses JSONStringValueSerializer which is vacuumed-off logic isolated from v8 and everything. I believe @jyasskin has raised this issue other times in this specification (maybe looking into a realm-agnostic method of doing the same thing), but I can't find any bugs on this spec, Web IDL, or Infra that represents this. Jeffrey, do you think we could benefit from JSON algorithms that aren't tied to a realm? In the meantime, I'm wondering if we should file a bug on this spec, and tag each instance of this with that bug for tracking.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a bug for this and noting it everywhere we parse JSON [=in parallel=] SGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infra should really let us do this: whatwg/infra#625.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jyasskin had a comment about we cannot create JS objects [=in parallel=]. For parsing JSON to infra value, "[=parse a JSON string to an Infra value=] involve creating JS objects also, but because those objects don't escape, it's clear how to implement it without actually creating them".

So we only moved [=parsing JSON bytes to a JavaScript value=] to algorithms that are within single realm, but kept [=parse a JSON string to an Infra value=] in algorithms that run [=in parallel=]. But a "realm-agnostic method" to parse json would be great.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing that, I couldn't remember if you had already filed something like that or not. OK I think we should make an Issue: block next to this algorithm's usage and point to whatwg/infra#625 from there.

string to an Infra value=] given |value|.
HabibiYou marked this conversation as resolved.
Show resolved Hide resolved
1. Otherwise, jump to the step labeled <i><a href=#abort-update>Abort update</a></i>.

<dt>"`ads`"
<dt>"`adComponents`"
<dd>
Expand Down