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.

Sign up for GitHub

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

Jump to bottom

[Spec] Add size API surface to joinAdInterestGroup #1210

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

[Spec] Add size API surface to joinAdInterestGroup #1210

wants to merge 32 commits into from

Conversation

gtanzer
Copy link
Contributor

@gtanzer gtanzer commented Jun 20, 2024 •
edited by pr-preview bot
Loading

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

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 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.
dmdabbs
dmdabbs reviewed Jun 28, 2024
View reviewed changes
spec.bs Show resolved Hide resolved
qingxinwu
qingxinwu reviewed Jun 28, 2024
View reviewed changes
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
gtanzer and others added 2 commits July 8, 2024 13:19
@gtanzer
Fix strings -> lists of strings
3999696
@gtanzer @qingxinwu
Add default null for size group
a7a7615 
Co-authored-by: qingxinwu <6334674+qingxinwu@users.noreply.github.com>
gtanzer and others added 4 commits July 8, 2024 13:20
@gtanzer @qingxinwu
Change 1. to *
804d13b 
Co-authored-by: qingxinwu <6334674+qingxinwu@users.noreply.github.com>
@gtanzer @qingxinwu
Change 1. to *
3385b8f 
Co-authored-by: qingxinwu <6334674+qingxinwu@users.noreply.github.com>
@gtanzer
Flatten 2 lines
6746464
@gtanzer
Flatten 2 lines
26a4355
domfarolino
domfarolino reviewed Jul 10, 2024
View reviewed changes
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
@@ -185,13 +185,19 @@ partial interface Navigator {

dictionary AuctionAd {
required USVString renderURL;
USVString sizeGroup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

DOMString? I guess it depends, what does this contain? See https://w3ctag.github.io/design-principles/#idl-string-types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason to use USVStrings; these are treated as opaque identifiers. Fixed.
Copy link
Collaborator

@qingxinwu qingxinwu Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

but the implementation (AuctionAd.sizeGroup, AuctionAdInterestGroupSize) uses USVString. We need to keep the spec in sync with the implementation. If we want to change it to DOMString, we need to change the implementation as well, which I hope don't need to request some approvals first.
spec.bs Outdated Show resolved Hide resolved
@gtanzer
Copy link
Contributor Author

gtanzer commented Jul 12, 2024

@domfarolino @qingxinwu All comments addressed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
5 participants
@gtanzer @qingxinwu @dmdabbs @domfarolino @JensenPaul