prevWins Clarity #852
Merged
prevWins Clarity #852
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While digging in on the nature of `prevWins`, I wasn't sure whether there's a gap between my understanding of the explainer (updated here) and the [full spec](https://wicg.github.io/turtledove/), in particular bullets 5-7 of the "To generate a bid" section (search for "Let prevWins be a new sequence<PreviousWin>."). Based on the spec I think it's saying: * The field in `browserSignals` is `prevWinsMs`. * `prevWins` as a whole is a list (sequence) and each element is a `PreviousWin` dictionary that has the `timeDelta` btw the win and now, and full serialized JSON from the `ad` component of what was returned by `generateBid` for that win. This is a little different than what is written and maybe implied, or at least not clear to me, in the explainer: * The field in `browserSignals` is `prevWins`. * The value is a list, with element being itself a list of 2 elements, the first being the "time" and the second being the "ad". My initial assumption here had been that the "time" was the timestamp of the won auction, and the ad was the creative from the IGs ads list. So my PR here hopes to reconcile this with the following hopes: * That the name of the field in `browserSignals` is indeed `prevWins` rather than `prevWinsMs`, and that the spec needs a minor update here (or there's an alias I'm missing). * That the contents of `prevWins` has the detail indicated in the spec.
|
Also the "all wins on the browser" part is intended to add some certainty to what I'm fairly sure (sure hoping) is a misquote of @alextcone-google here on capping. |
It was indeed a misquote. We've asked for a correction. |
JensenPaul
reviewed
Nov 7, 2023
|
Isaac, what do you think of the edits I proposed? I'd like to merge this sooner than later as this is an area of frequent questions so a clarification will really help folks. |
Co-authored-by: Paul Jensen <[email protected]>
|
Sorry I was out the last 3 weeks paying limited attention, just accepted. |
Isaac seemed to only accept my last comment but I think he meant to accept this one too, so merging it.
github-actions bot
added a commit
that referenced
this pull request
Dec 8, 2023
SHA: 820763e Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While digging in on the nature of
prevWins, I wasn't sure whether there's a gap between my understanding of the explainer (updated here) and the full spec, in particular bullets 5-7 of the "To generate a bid" section (search for "Let prevWins be a new sequence.").Based on the spec I think it's saying:
browserSignalsisprevWinsMs.prevWinsas a whole is a list (sequence) and each element is aPreviousWindictionary that has thetimeDeltabtw the win and now, and full serialized JSON from theadcomponent of what was returned bygenerateBidfor that win.This is a little different than what is written and maybe implied, or at least not clear to me, in the explainer:
browserSignalsisprevWins.So my PR here hopes to reconcile this with the following hopes:
browserSignalsis indeedprevWinsrather thanprevWinsMs, and that the spec needs a minor update here (or there's an alias I'm missing).prevWinshas the detail indicated in the spec.