-
Notifications
You must be signed in to change notification settings - Fork 235
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
Fix reporting's return value handling. #642
Conversation
Return string "null" instead of null for signals_for_winner from `reportResult()`, which will then be passed to `reportWin()` as the string "null".
spec.bs
Outdated
|reportResultOutputIDL|["{{ReportResultOutput/reportingBeaconMap}}"] in the | ||
{{FencedFrameConfig}} as appropriate. | ||
1. Return « |reportResultOutputIDL|["{{ReportResultOutput/signalsForWinner}}"], |browserSignals| ». | ||
1. TODO: Store |reportUrl| and |reportingBeaconMap| in the {{FencedFrameConfig}} as appropriate. |
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.
We're in parallel at this moment, right? Let's make this [=fenced frame config=]
which is an infra struct, since I think we won't be constructing the platform object {{FencedFrameConfig}}
here
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, we're in parallel, since its caller [=generate and score bids=] is in parallel. So we cannot construct IDL objects when it's in parallel? I wanted to define and construct a browserSignals IDL object (already did that for generateBid's, and scoreAd's is under review) in methods like this that runs in parallel. If we cannot do that, we need to pass an ordered map to where we create a new |agent| and convert it to IDL object there? It adds more complexity.
Now I think I need to reconsider those in-parallels, for example, the top level caller. What I wanted to spec was that different component auctions can run in parallel without blocking each other, and script/signal fetches of different buyers can be run in parallel, etc., but it seems its scope is too wide in current 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.
You can construct dictionaries in parallel. Not platform objects: https://webidl.spec.whatwg.org/#es-platform-objects
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.
Done. Didn't notice it's an interface :)
Now it makes sense.
SHA: 7cc77db Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 7cc77db Reason: push, by qingxinwu Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 7cc77db Reason: push, by morlovich Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Preview | Diff