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

[Spec] Add size API surface to joinAdInterestGroup #1210

Merged
merged 39 commits into from
Aug 19, 2024

Conversation

gtanzer
Copy link
Contributor

@gtanzer gtanzer commented Jun 20, 2024

Add size fields to the API surface for interest group joining / updating, and propagate those fields into interest group browser signals.

Note: Currently, the IDL fields sizeGroup in AuctionAd and width/height AuctionAdInterestGroupSize are specified to use USVString rather than DOMString, only because the Chromium implementation currently does. This contradicts the recommended use of DOMString for anything that won't appear in a UI. This issue is tracked at #1250


Preview | Diff

@gtanzer
Copy link
Contributor Author

gtanzer commented Jun 20, 2024

@domfarolino PTAL. @JensenPaul should officially add you as a reviewer soon.

@gtanzer gtanzer changed the title Add size API surface to joinAdInterestGroup [Spec] Add size API surface to joinAdInterestGroup Jun 21, 2024
@JensenPaul JensenPaul added the spec Relates to the spec label Jun 21, 2024
@qingxinwu
Copy link
Collaborator

Seems this will resolve issue #495 ? If so, please add reference to this issue in the description.

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
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 Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@gtanzer
Copy link
Contributor Author

gtanzer commented Jul 12, 2024

@domfarolino @qingxinwu All comments addressed.

Copy link
Collaborator

@qingxinwu qingxinwu left a comment

Choose a reason for hiding this comment

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

LGTM, expecting the implementation to match the spec on DOMString soon. If we cannot change the impl in near future, then change spec to USVString and update both in the future might be more preferable.

spec.bs Outdated Show resolved Hide resolved
@JensenPaul JensenPaul merged commit 8721b57 into WICG:main Aug 19, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Aug 19, 2024
SHA: 8721b57
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Aug 20, 2024
SHA: 8721b57
Reason: push, by qingxinwu

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.

5 participants