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.
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 optional slot-size parameters to trustedBiddingSignalsURL requests #928
Add optional slot-size parameters to trustedBiddingSignalsURL requests #928
Changes from 1 commit
ce4855a
ce59178
f229705
4eefec4
c9af021
5bdac81
dc11454
1a4d6ae
462c55d
6be6a4d
6197823
e0c924b
edb343e
eeedda9
fa000ea
6f45465
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
To what does normailzed refer?
Nits:
s/auction/auction's
s/allSlotsRequsetedSizes/allSlotsRequestedSizes
Params have been camel cased, so perhaps
@JensenPaul provided this caveat in the #869 thread:
Verifying that this proposes to alter the coalescing scope (emphasis mine)...
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.
Sizes are strings with case suffixes, and parsing rules I haven't fully grokked yet. If this allows multiple variants of the same value (e.g., "100 PX", "100px", I don't think we want to preserve the exact details of how the string was formatted, to minimize side channels, and URL variation based on who created the AuctionConfig.
Both fixed.
Thanks for catching that - also a typo.
I'm concerned about ambiguities here - in the unlikely case one buyer uses single slots for one IG and all slots for others, it would potentially be a coin flip which request we merged with (and would also depend on which other IGs the user has been added to). I'm not really comfortable with that. Particularly if we add more things like this, it just seems like things can get worse and worse. I'd really rather not merge. The only argument I've seen for merging is for the 1-30 day ramp up period when a site starts adding these (if it uses update URLs, can update old IGs pretty quickly), and I think that case is niche enough to not be worth adding more ambiguity to auction behavior.
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. Not contesting, just verifying.
Not following the case.
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.
The use case is if a buyer starts adding one of these params to its IGs (because it decided it wants to use the feature, either because it was just added, or if a buyer just decides it wants information it didn't need before), there's a ramp up period where some IGs have the new option and some don't, when we aren't merging the trusted signals requests, but doing so would probably be harmless to the buyer, and would improve performance. That seems the main case where merging them would be useful.