[Spec] Add trustedBiddingSignalsSlotSizeMode

[MattMenke2]

These are the spec changes associated with #928.

---

Preview | Diff

[dmdabbs]

@MattMenke2 will the new sizemode setting be updateable? I'm thinking of executionMode which recently needed to have update logic tweaked.

[MattMenke2]

@MattMenke2 will the new sizemode setting be updateable? I'm thinking of executionMode which recently needed to have update logic tweaked.

Yes, it will (And updating should already work on Canary). I was the one who noticed the executionMode update issues when I was implementing updating for this, actually. :)

[domfarolino]

Before I take a look at this, can I ask for someone else on the team with recent spec experience to give it a first pass for me?

[MattMenke2]

Didn't realize "build an interest group passed to generateBid" was separate from GenerateBidInterestGroup, so I added the new field there.

Also, I pulled in the latest upstream - there was apparently a CL that changed indent of a code block, and the entire block was, unfortunately, marked as a conflict. There was more than the indent change, so I had to go through and try and fix everything. I think things are correct now, but fixing that sort of thing in a web-based text editor is very error prone.

[MattMenke2]

Note that there is a compile error, but it's because "[=UTF-8=]" no longer uniquely resolves (This CL doesn't touch that - looks like that was added over 8 months ago)

[qingxinwu]

Note that there is a compile error, but it's because "[=UTF-8=]" no longer uniquely resolves (This CL doesn't touch that - looks like that was added over 8 months ago)

Yeah, I saw the same error. Seems there are more [=UTF-8=] defined in different specs now. Sent a PR to fix it.

[qingxinwu]

Didn't realize "build an interest group passed to generateBid" was separate from GenerateBidInterestGroup, so I added the new field there.

Also, I pulled in the latest upstream - there was apparently a CL that changed indent of a code block, and the entire block was, unfortunately, marked as a conflict. There was more than the indent change, so I had to go through and try and fix everything. I think things are correct now, but fixing that sort of thing in a web-based text editor is very error prone.

yeah, sorry for the inconvience. Some of my readability improvement PRs got merged recently, and it's not that easy to resolve merge conflicts using the text editor. I'll take a look

[domfarolino]

It's a good catch! I treated the way we estimate IG size as an implemention detail and accepted some deviations.
We can change it to "4" to match our code exactly, but [=interest group/execution mode=] is [=string=] currently, and the size of enum is language-specific, right? @domfarolino How should we handle the difference here between code and spec? If we want to make them to match, is there an infra type "enum" so that we can define the fields' data types to enums instead of strings, and get a standard size of enum? I don't find one. The IDL enumeration's value is a list of strings, still different from C++'s enum which stores ints.

So if I understand correctly, you want to use an enum in the spec, to match what we do in the implementation, but don't want to just use a Web IDL enum because those are just DOMStrings and don't have a fixed integer length like our C++ enums do in the implementation, right?

If that's right, then I would recommend doing something that the HTML Standard does on occasion. Consider "cross origin isolation mode", which isn't explicitly a Web IDL enum, but is effectively a "spec enum" because it is "one of three values". Each value is descriptive in name, and in your spec, I would go a step further and say that each value represents an unsigned short or whatever size integer you need. So you'll effectively have a new "type" for each member inside the struct, and each of these types will have a range of possible/allowed descriptive values that are all actually Web IDL integer types under the hood, meaning they have a fixed size in bits.

Does that make sense? It can come after this PR possibly, since that's kind of an overhaul to some of that pre-existing infrastructure.

[domfarolino]

It looks like something to do with utf-8 is breaking the build again maybe? Does this need to be rebased or something?

[qingxinwu]

It looks like something to do with utf-8 is breaking the build again maybe? Does this need to be rebased or something?

Yeah, this PR hasn't pulled the fix yet. It needs to sync to get the fix.

[qingxinwu]

It's a good catch! I treated the way we estimate IG size as an implemention detail and accepted some deviations.
We can change it to "4" to match our code exactly, but [=interest group/execution mode=] is [=string=] currently, and the size of enum is language-specific, right? @domfarolino How should we handle the difference here between code and spec? If we want to make them to match, is there an infra type "enum" so that we can define the fields' data types to enums instead of strings, and get a standard size of enum? I don't find one. The IDL enumeration's value is a list of strings, still different from C++'s enum which stores ints.

So if I understand correctly, you want to use an enum in the spec, to match what we do in the implementation, but don't want to just use a Web IDL enum because those are just DOMStrings and don't have a fixed integer length like our C++ enums do in the implementation, right?

If that's right, then I would recommend doing something that the HTML Standard does on occasion. Consider "cross origin isolation mode", which isn't explicitly a Web IDL enum, but is effectively a "spec enum" because it is "one of three values". Each value is descriptive in name, and in your spec, I would go a step further and say that each value represents an unsigned short or whatever size integer you need. So you'll effectively have a new "type" for each member inside the struct, and each of these types will have a range of possible/allowed descriptive values that are all actually Web IDL integer types under the hood, meaning they have a fixed size in bits.

Does that make sense? It can come after this PR possibly, since that's kind of an overhaul to some of that pre-existing infrastructure.

yeah, I can do this for execution mode in a follow up PR (find another bug about it, will fix it as well).
For the size mode, maybe you can add a TODO in [=estimate size=] to add sizeMode's size which I can also add in the follow up PR. Does this look good to you, @MattMenke2?

[qingxinwu]

still LGTM % minor comments. Rebase looks correct, but need to pull again to get the fix.

[JensenPaul]

@domfarolino, it looks like @qingxinwu finished up his review and approved; could you review it now that the first review is complete?

[domfarolino]

LGTM % some nits

[MattMenke2]

Thanks for all the help, Dominic and Qingxin!