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 update for ad size entries in ScoringBrowserSignals and BiddingBrowserSignals. #1141

Merged
merged 19 commits into from
Jun 3, 2024
Merged

Spec update for ad size entries in ScoringBrowserSignals and BiddingBrowserSignals. #1141

merged 19 commits into from
Jun 3, 2024

Conversation

xiaochen-z
Copy link
Contributor

@xiaochen-z xiaochen-z commented Apr 17, 2024 •
edited by pr-preview bot
Loading

  1. The explainer states the browserSignals argument of scoreAd function contains an optional member renderSize. However, this has not been spec'd. This PR adds renderSize to the spec. See clarification regarding renderSize in scoring browser signals #1088.

  2. Similarly, the requestedSize for biddingBrowserSignals is also missing from the spec.

Both renderSize and requestedSize code examples in explainer are missing size units. See explainer update PR: #1145

Preview | Diff

@JensenPaul JensenPaul requested a review from qingxinwu April 18, 2024 13:38
@JensenPaul JensenPaul added the spec Relates to the spec label Apr 18, 2024
qingxinwu
qingxinwu reviewed Apr 18, 2024
View reviewed changes
spec.bs Outdated Show resolved Hide resolved
@qingxinwu
Copy link
Collaborator

qingxinwu commented Apr 18, 2024 •
edited
Loading

The spec also misses the requestedSize for biddingBrowserSignals. I think we should add that to the spec as well.

@qingxinwu qingxinwu mentioned this pull request Apr 19, 2024
Merged
@xiaochen-z xiaochen-z changed the title Spec renderSize in ScoringBrowserSignals of scoreAd function. Spec update for ad size entries in ScoringBrowserSignals and BiddingBrowserSignals. Apr 19, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 19, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
c1dce39 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Apr 19, 2024
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 22, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
e88005e 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 22, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
024dd4f 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 22, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
125c555 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
@xiaochen-z
Copy link
Contributor Author

The spec also misses the requestedSize for biddingBrowserSignals. I think we should add that to the spec as well.

Add requestedSize and add the algorithm to convert an AdSize to a dict. This PR is ready for another look, thanks! @qingxinwu

Explainer update is done in #1145.

qingxinwu
qingxinwu reviewed Apr 22, 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
@xiaochen-z
Copy link
Contributor Author

Review comments resolved, thanks!

qingxinwu
qingxinwu approved these changes Apr 23, 2024
View reviewed changes
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.

@domfarolino Ready for review. Thanks!

@qingxinwu qingxinwu requested a review from domfarolino April 23, 2024 14:49
domfarolino
domfarolino approved these changes Apr 24, 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
xiaochen-z and others added 4 commits April 25, 2024 09:46
@xiaochen-z @domfarolino
Update spec.bs
71f68f7 
Co-authored-by: Dominic Farolino <[email protected]>
@xiaochen-z @domfarolino
Update spec.bs
37508e4 
Co-authored-by: Dominic Farolino <[email protected]>
@xiaochen-z @domfarolino
Update spec.bs
f05494e 
Co-authored-by: Dominic Farolino <[email protected]>
@xiaochen-z
Remove redundant empty lines
959538a
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 29, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
d1326b5 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 1, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
0f6d60f 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 1, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
01c25c2 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5447954
Reviewed-by: Caleb Raitto <[email protected]>
Commit-Queue: Xiaochen Zhou <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1294870}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 1, 2024
@xiaochen-z @chromium-wpt-export-bot
Protected Audience: Add missing renderSize field to browserSignals of
3c8f88b 
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5447954
Reviewed-by: Caleb Raitto <[email protected]>
Commit-Queue: Xiaochen Zhou <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1294870}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 6, 2024
@xiaochen-z @moz-wptsync-bot
Bug 1892451 [wpt PR 45802] - Protected Audience: Add missing renderSi…
bcc2143 
…ze field to browserSignals of scoreAd., a=testonly

Automatic update from web-platform-tests
Protected Audience: Add missing renderSize field to browserSignals of
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5447954
Reviewed-by: Caleb Raitto <[email protected]>
Commit-Queue: Xiaochen Zhou <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1294870}

--

wpt-commits: 3c8f88bb2b952e67e70a7a896ef6b0cf259603f0
wpt-pr: 45802
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request May 10, 2024
@xiaochen-z @moz-wptsync-bot
Bug 1892451 [wpt PR 45802] - Protected Audience: Add missing renderSi…
2fcd31e 
…ze field to browserSignals of scoreAd., a=testonly

Automatic update from web-platform-tests
Protected Audience: Add missing renderSize field to browserSignals of
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5447954
Reviewed-by: Caleb Raitto <[email protected]>
Commit-Queue: Xiaochen Zhou <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1294870}

--

wpt-commits: 3c8f88bb2b952e67e70a7a896ef6b0cf259603f0
wpt-pr: 45802
@JensenPaul JensenPaul merged commit 3b98c4f into WICG:main Jun 3, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jun 3, 2024
@xiaochen-z @github-actions
Spec update for ad size entries in ScoringBrowserSignals and BiddingB…
d8d78bd 
…rowserSignals. (#1141)

SHA: 3b98c4f
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 6, 2024
@xiaochen-z @moz-wptsync-bot
Bug 1892451 [wpt PR 45802] - Protected Audience: Add missing renderSi…
79bb0a1 
…ze field to browserSignals of scoreAd., a=testonly

Automatic update from web-platform-tests
Protected Audience: Add missing renderSize field to browserSignals of
scoreAd.

The Protected Audience explainer claims the browserSignals argument of
scoreAd function should contain an optional field of renderSize.
However, it is found that this has not been implemented. See:
https://github.com/WICG/turtledove/blob/main/FLEDGE.md#:~:text=%27renderSize%27%3A%20%7Bwidth%3A%20100%2C%20height%3A%20200%7D%2C%20/*%20if%20specified%20in%20the%20bid%20*/

This CL implements this optional field to match what the explainer
claims.

See: WICG/turtledove#1088

Spec PR: WICG/turtledove#1141
Explainer PR: WICG/turtledove#1145

Bug: 333628467
Change-Id: I95d5ebb7ddbbeb50d0ac3618e22ed1e228ec32a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5447954
Reviewed-by: Caleb Raitto <[email protected]>
Commit-Queue: Xiaochen Zhou <[email protected]>
Reviewed-by: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1294870}

--

wpt-commits: 3c8f88bb2b952e67e70a7a896ef6b0cf259603f0
wpt-pr: 45802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domfarolino domfarolino domfarolino approved these changes

@qingxinwu qingxinwu qingxinwu approved these changes

Assignees
No one assigned
Labels
spec Relates to the spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xiaochen-z @qingxinwu @domfarolino @JensenPaul