-
Notifications
You must be signed in to change notification settings - Fork 229
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.
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
Add spec for Bidding and Auction Services API #1200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial comments.
spec.bs
Outdated
@@ -5677,6 +5873,9 @@ An <dfn>interest group ad</dfn> is a [=struct=] with the following [=struct/item | |||
with registered macros. Each origin's [=origin/scheme=] must be "`https`" and each origin must be | |||
<a href="https://github.com/privacysandbox/attestation">enrolled</a>. Only meaningful in | |||
[=interest group/ads=], but ignored in [=interest group/ad components=]. | |||
: <dfn> ad render ID</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems need to update https://github.com/WICG/turtledove/blob/main/spec.bs#L1419, since ad render id is in prevWin's ad json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though perhaps that can be in another CL. On a related note, is there a reason why we are specifying that we are using JSON to save previous wins? Perhaps just removing the note that we store it as JSON would be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the "ad JSON" field should probably be an "interest group ad" instead, since we need access to the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was following https://crsrc.org/c/content/services/auction_worklet/public/mojom/bidder_worklet.mojom;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;l=37. Also, the field in browser signals should be json as well?
If we need, we can change it to a struct when it's passed around, but convert it to json at some point when a json is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, internally we store it as JSON, but we convert it to a JavaScript Object before providing it in generateBid (see https://crsrc.org/c/content/services/auction_worklet/bidder_lazy_filler.cc;l=68 ).
So it being stored as JSON doesn't really affect the public interface -- it seems more like an implementation detail that doesn't belong in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the spec is wrong then, because it passes a DOMString to generateBid for this field in browser signals (converted to ECMAScript values converts ad json DOMString to a JS string, not a JS object) . We need to change the previous win struct's ad json field to a struct, and PreviousWin IDL's adJson to a dictionary of several fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be a separate PR and I can do that if you'd like. And then I'm good with landing this one.
Co-authored-by: qingxinwu <[email protected]>
Co-authored-by: qingxinwu <[email protected]> Co-authored-by: Paul Jensen <[email protected]>
Co-authored-by: qingxinwu <[email protected]>
SHA: 4d02ec0 Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Preview | Diff