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

Move lifetimeMs from GenerateBidInterestGroup to AuctionAdInterestGroup #843

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

MattMenke2
Copy link
Contributor

@MattMenke2 MattMenke2 commented Oct 5, 2023

Move lifetimeMs from GenerateBidInterestGroup to AuctionAdInterestGroup because of concerns about ambiguity and providing a high accuracy timer when executionMode allows multiple same-origin interest groups to share state.

Note that lifetimeMs was never provided to generateBid() in Chrome, anyways, I believe, so no consumers should be broken by this change.


Preview | Diff

Move lifetimeMs from GenerateBidInterestGroup to AuctionAdInterestGroup because of concerns about ambiguity and providing a high accuracy timer when executionMode allows multiple same-origin interest groups to share state.

Note that lifetimeMs was never provided to generateBid() in Chrome, anyways, so no consumers should be broken by this change.
@MattMenke2
Copy link
Contributor Author

If there's a need for generateBid() to know the expiry, I think it would be better to provide it browserSignals, where there's no ambiguity and likely to be no confusion over its meaning.

More generally, I wonder if we should default to putting fields in AuctionAdInterestGroup unless they're really needed in GenerateBidInterestGroup (e.g., I'm skeptical the priorityVector or enableBiddingSignalsPrioritization are useful).

@JensenPaul JensenPaul added the spec Relates to the spec label Oct 9, 2023
@qingxinwu
Copy link
Collaborator

qingxinwu commented Nov 6, 2023

If there's a need for generateBid() to know the expiry, I think it would be better to provide it browserSignals, where there's no ambiguity and likely to be no confusion over its meaning.

At least for now expiry is not used in generateBid(). Currently expiry is only used for IG maintanance (screenshot), so I think we're good here, before we want to pass expiry to generateBid().

More generally, I wonder if we should default to putting fields in AuctionAdInterestGroup unless they're really needed in GenerateBidInterestGroup (e.g., I'm skeptical the priorityVector or enableBiddingSignalsPrioritization are useful).

Good catch! Yes, that's desirable. I think I put priorityVector in GenerateBidInterestGroup just because I thought it was needed in generate bid. So feel free to fix these. Thanks!

@MattMenke2
Copy link
Contributor Author

Good catch! Yes, that's desirable. I think I put priorityVector in GenerateBidInterestGroup just because I thought it was needed in generate bid. So feel free to fix these. Thanks!

I had assumed you put it there because that's what the explainer calls for, and that's what the code currently does. :)

spec.bs Show resolved Hide resolved
@qingxinwu
Copy link
Collaborator

Good catch! Yes, that's desirable. I think I put priorityVector in GenerateBidInterestGroup just because I thought it was needed in generate bid. So feel free to fix these. Thanks!

I had assumed you put it there because that's what the explainer calls for, and that's what the code currently does. :)

Yeah your assumption sounds more aligned with why I did that :)

@MattMenke2
Copy link
Contributor Author

Looking at the code, the only other thing we don't currently set is executionMode, I believe.

@JensenPaul JensenPaul merged commit 46d9eb5 into WICG:main Aug 13, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Aug 13, 2024
…up (#843)

SHA: 46d9eb5
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
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants