-
Notifications
You must be signed in to change notification settings - Fork 254
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
Parse Ad-Auction-Additional-Bid response header #888
Conversation
@caraitto PTAL. Feel free to suggest other ways to orgnize the algorithms. Plan to move the "Fetch Patch for Auction Headers" section right after "Additional Bids" section, but will do it in a future PR. |
{{WindowOrWorkerGlobalScope/fetch()}} call (using the {{RequestInit/adAuctionHeaders}} option) | ||
initiated by any *other* {{Document}} in the *same* [=traversable navigable=]. | ||
derived from JSON from an [:Ad-Auction-Signals:] header, or [=additional bids=] derived from an | ||
[:Ad-Auction-Additional-Bid:] header, captured by a {{WindowOrWorkerGlobalScope/fetch()}} call |
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.
captured by a {{WindowOrWorkerGlobalScope/fetch()}} call (using the {{RequestInit/adAuctionHeaders}} option)
There is also pending support for an iframe attribute mechanism.
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'll need to handle the merge of this PR with #883, depending on which one lands first.
{{WindowOrWorkerGlobalScope/fetch()}} call (using the {{RequestInit/adAuctionHeaders}} option) | ||
initiated by any *other* {{Document}} in the *same* [=traversable navigable=]. | ||
derived from JSON from an [:Ad-Auction-Signals:] header, or [=additional bids=] derived from an | ||
[:Ad-Auction-Additional-Bid:] header, captured by a {{WindowOrWorkerGlobalScope/fetch()}} call |
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'll need to handle the merge of this PR with #883, depending on which one lands first.
Co-authored-by: caraitto <caraitto@google.com>
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.
Generally looks good % comments
@domfarolino Mind taking a look? The real meaningful change is in algorithm [=update captured headers=]. Other changes are just moving existing contents around to reorgnize them. |
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.
Thanks! Please wait for Dominic's review.
SHA: 3de7fde Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 3de7fde Reason: push, by MattMenke2 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Parse
Ad-Auction-Additional-Bid
response header to get additional bids.Since it has the same way with direct from seller signals, so makes the existing algorithms more general for the two use cases, and changed the section header for "Direct from seller signals" to something more general "Fetch Patch for Auction Headers"
Plan to move the "Fetch Patch for Auction Headers" section right after "Additional Bids" section, but to make this PR easier to review, going to do that in a future PR.
Preview | Diff