Skip to content
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 iframe support for adAuctionHeaders to spec. #918

Merged
merged 7 commits into from
Dec 20, 2023
Merged

Conversation

orrb1
Copy link
Collaborator

@orrb1 orrb1 commented Nov 17, 2023

This support is provided by a new adAuctionHeaders attribute on the
iframe element that will have the same functional behavior as the
adAuctionHeaders fetch flag, in that it will trigger the user agent to
send the Sec-Ad-Auction-Fetch request header, and to remove
Ad-Auction-Signals and Ad-Auction-Additional-Bid response headers,
providing their values to the Protected Audience auction.

This is a continuation of #883,
which needed to be closed due to an merge conflict on rebase.


Preview | Diff

This support is provided by a new adAuctionHeaders attribute on the
iframe element that will have the same functional behavior as the
adAuctionHeaders fetch flag, in that it will trigger the user agent to
send the Sec-Ad-Auction-Fetch request header, and to remove
`Ad-Auction-Signals` and `Ad-Auction-Additional-Bid` response headers,
providing their values to the Protected Audience auction.
This support is provided by a new `adAuctionHeaders` attribute on the
iframe element that will have the same functional behavior as the
`adAuctionHeaders` fetch flag, in that it will trigger the user agent to
send the `Sec-Ad-Auction-Fetch` request header, and to remove
`Ad-Auction-Signals` and `Ad-Auction-Additional-Bid` response headers,
providing their values to the Protected Audience auction.

This is a continuation of WICG#883,
which needed to be closed due to an merge conflict on rebase.
@orrb1 orrb1 added the spec Relates to the spec label Nov 17, 2023
@orrb1 orrb1 requested a review from caraitto November 17, 2023 16:00
Copy link
Collaborator

@caraitto caraitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, some thoughts.

spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@orrb1
Copy link
Collaborator Author

orrb1 commented Nov 22, 2023

Thanks for the helpful feedback, Caleb!

@orrb1 orrb1 requested a review from domfarolino November 22, 2023 01:38
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
request with the <{iframe/adAuctionHeaders}> <a spec=html>content attribute</a> set to `true`.
In this case, the JavaScript code is retrieved as part of the iframe navigation response,
at which point the JavaScript code makes the call to {{Navigator/runAdAuction()}},
and |config|["{{AuctionAdConfig/directFromSellerSignalsHeaderAdSlot}}"] can be immediately resolved.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is pretty much all a duplicate note, then I think I might just point to the one above manually, instead of copy it all. Not a big deal, up to you, but it'd be easier to maintain that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that initially, but ended up deciding it was clearer to keep them separate. They're close to the same, but with a few salient differences, most notably the name of the config field, and the more subtle difference that config["directFromSellerSignalsHeaderAdSlot"] can be can be specified directly without a Promise, whereas config["additionalBids"] cannot, and so needs to be a Promise that's immediately resolved. Especially if there's later divergence, I suspect it's easier to start these out as separate. Thanks.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Changing the adauctionheaders content attribute to be all-lowercase,
adjusting the language intended to describe the response returned from
the fetch call, and using the dfn autolink for `reflects`.
@orrb1
Copy link
Collaborator Author

orrb1 commented Dec 14, 2023

Thanks for the thorough review, Dom!

@orrb1 orrb1 requested a review from domfarolino December 14, 2023 15:21
The section for `Sec-Ad-Auction-Fetch` currently says that the
`Ad-Auction-Signals` response header will be removed from the response and
will instead only be used in Protected Audiences auctions. This change
adds `Ad-Auction-Additional-Bid`, which is treated the same way.
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % one nit

<div algorithm="iframe navigation patch">
The following step will be added to the
<a spec="html">create navigation params by fetching</a> steps
after step "Let |request| be a new [=Request/request=], with ...":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really matter much since I'm commenting on the monkey patch, but can [=Request/request=] be [=request=], or <a for=/>request</a>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to point to something different: https://fetch.spec.whatwg.org/#concept-request-request (for [=Request/request=]) vs https://fetch.spec.whatwg.org/#concept-request (for [=request=]), so have to leave this as is. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we want to point to https://fetch.spec.whatwg.org/#concept-request, not https://fetch.spec.whatwg.org/#concept-request-request, so I think we should probably change this in a small follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I got confused as to which line in the referenced spec we were matching. I had convinced myself that we were referencing step 28 of the Request constructor in the Fetch Standard ("Set this’s request to request."), which points to #concept-request-request, instead of the line we're actually referencing - step 3 of the "create navigation params by fetching" in the HTML standard ("Let request be a new request..."

I'll send a follow-up PR to fix this. I based this part on the equivalent section in the Topics API draft spec. I'll follow up with them to recommend that change there as well.

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #964. Thanks again!

@JensenPaul JensenPaul merged commit d512d45 into WICG:main Dec 20, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Dec 20, 2023
SHA: d512d45
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
orrb1 added a commit to orrb1/turtledove that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants