Spec: add some shared infra for reporting, and port forDebugOnly to it. #1296
Conversation
spec.bs
Outdated
| : [=reporting bid key/origin=] | ||
| :: |ig|'s [=interest group/owner=] | ||
| : [=reporting bid key/interest group name=] | ||
| :: A string representation of a new globably unique identifier. This is needed since |igName| |
There was a problem hiding this comment.
is this field not the IG name? But it seems to be IG name here: https://github.com/WICG/turtledove/pull/1296/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR3506
There was a problem hiding this comment.
It's interest group name for regular interest groups. For additional bids it can't be, since there is nothing saying that they have to provide a unique interest group name. (And now I am scared of something in our impl breaking because of that...).
There was a problem hiding this comment.
So renamed this. The new description is kinda vague, but it does I think clarify that one can't expect it to be an IG name.
There was a problem hiding this comment.
does impl rely on IG name? if so, we need to fix it?
There was a problem hiding this comment.
Impl generally uses BidState* as equivalent of "reporting bid key" for reporting. There are a bunch of other things that use InterestGroupKey which seem fine for what I looked, but there are too many for me to be certain none get confused with additionalBid stuff (especially since it's somehow wired into fenced frame?)
|
Anything else I need to do here? |
| context. | ||
| </dl> | ||
|
|
||
| A <dfn>reporting context</dfn> is a [=struct=] with the following [=struct/items=]: |
There was a problem hiding this comment.
might be useful to say that "is a [=struct=] which contains reports from bidding and scoring phases, with the following ...". PAgg also exists in reportWin()/reportResult(), but probably we don't want to put them into this
|
just this last comment, but I'm fine with adding that sentence when we start handling PAgg requests (from reportWin/reportResult) in this spec (then we know whether it's true or not). Otherwise, LGTM and just approved. |
…t. (WICG#1296) * Start the refactor on reporting... * Fix score invocations. * Don't do reporting from shadow auction. * Revert note move. * Rename field. * Apply feedback * More feedback. * origin -> bidder origin. * More feedback --------- Co-authored-by: Maks Orlovich <[email protected]>
This is intended to be used for moving our Private Aggregation support into our spec, as it provides:
vague on how the top-level scope should work still).
...for now, forDebugOnly is what's ported to it, and while at it, things were fixed to actually pass the argument needed for reporting to scoreAd invocations, and to fix up additional bids (and B&A) for it.
It may make sense to move realTimeContributions in there as well, though it doesn't really benefit from any functionality besides maybe avoiding a parameter sometimes (and the split is kinda harmful).
Preview | Diff