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] Introduce StorageInterestGroup and "get storage interest group… #1299

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

xyaoinum
Copy link
Contributor

@xyaoinum xyaoinum commented Oct 10, 2024

…s for owner" algorithm

This is the Protected Audience side's spec update to support interestGroups() within shared storage worklet (WICG/shared-storage#180).


Preview | Diff

…s for owner" algorithm

This is the Protected Audience side's spec update to support interestGroups() within shared storage worklet (WICG/shared-storage#180).
xyaoinum added a commit to WICG/shared-storage that referenced this pull request Oct 10, 2024
This is the Shared Storage side's spec update to support interestGroups() within shared storage worklet (#180). Sibling spec update PR for Protected Audience: WICG/turtledove#1299.
@xyaoinum
Copy link
Contributor Author

@brusshamilton PTAL. Thanks!

@JensenPaul JensenPaul added the spec Relates to the spec label Oct 11, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 16, 2024
Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 16, 2024
Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 16, 2024
Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 16, 2024
Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Oct 21, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Oct 22, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 22, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <dchengchromium.org>
Commit-Queue: Yao Xiao <yaoxiachromium.org>
Cr-Commit-Position: refs/heads/main{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650

UltraBlame original commit: 94a35dfacf6d42d04039e6c44dc8f2c1c3ff56c3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 22, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <dchengchromium.org>
Commit-Queue: Yao Xiao <yaoxiachromium.org>
Cr-Commit-Position: refs/heads/main{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650

UltraBlame original commit: 94a35dfacf6d42d04039e6c44dc8f2c1c3ff56c3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 22, 2024
…(), a=testonly

Automatic update from web-platform-tests
[shared storage] Implement interestGroup()

Add interestGroups() to the shared storage worklet, to return the
Protected Audience interest groups associated with the
shared storage origin's owner, with some additional metadata.

Implement this behind a runtime feature, which is implicitly
controlled by a Finch flag.

Explainer PR: WICG/shared-storage#180

Spec PR(s):
1) WICG/turtledove#1299
2) WICG/shared-storage#203

Bug: 367992703
Binary-Size: Size increase is unavoidable.
Fuchsia-Binary-Size: Size increase is unavoidable.
Change-Id: I5fc5767fa53a91f021d64a871a6dd9cb88f4431c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5696046
Reviewed-by: Daniel Cheng <dchengchromium.org>
Commit-Queue: Yao Xiao <yaoxiachromium.org>
Cr-Commit-Position: refs/heads/main{#1369483}

--

wpt-commits: eb87bbbacf996ab46607538a72ea0adaee229e7a
wpt-pr: 48650

UltraBlame original commit: 94a35dfacf6d42d04039e6c44dc8f2c1c3ff56c3
@xyaoinum
Copy link
Contributor Author

@brusshamilton friendly ping :)

spec.bs Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
1. [=map/For each=] |origin| → |originSellerCapabilities| of |ig|'s [=interest group/seller capabilities=]:
1. Let |serializedOrigin| be the [=origin/serialization=] of |origin|.
1. [=map/Set=] |resultSellerCapabilities|[|serializedOrigin|] to |originSellerCapabilities|.
1. [=map/Set=] |resultIg|["{{GenerateBidInterestGroup/sellerCapabilities}}"] to |resultSellerCapabilities|.
Copy link
Contributor

Choose a reason for hiding this comment

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

{{GenerateBidInterestGroup/sellerCapabilities}} is not an [=ordered map=] of byte sequences, it's a record<USVString, sequence<DOMString>>. You need to convert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, record and ordered_map (== map) should be "implicitly compatible". And the following implicitly conversions should also hold: set (== ordered set) == list == sequence

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, record and ordered_map are "implicitly compatible", but the actual capabilities are stored internally as a byte sequence, not a sequence<DOMString>. So we need some conversion here. That might be kind of difficult since I don't think we actually specify how the enum is encoded into a byte sequence other than that it only takes a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filed #1316, as I feel the right approach is to define those enum fields as strings directly. Besides, in the current specification, when these fields are being set, there's no conversion either: "If capabilityString is "interest-group-counts" or "latency-stats", then append capabilityString to sellerCapabilities."

Thus, it looks like it's best to treat these fields as strings now? This improves readability and aligns with the direction of the proposed specification.

spec.bs Show resolved Hide resolved
1. [=map/Set=] |resultIg|["{{GenerateBidInterestGroup/trustedBiddingSignalsSlotSizeMode}}"] to |ig|'s [=interest group/trusted bidding signals slot size mode=].
1. [=map/Set=] |resultIg|["{{GenerateBidInterestGroup/maxTrustedBiddingSignalsURLLength}}"] to |ig|'s [=interest group/max trusted bidding signals url length=].
1. If |ig|'s [=interest group/user bidding signals=] is not null:
1. Let |parsedUserBiddingSignals| be |ig|'s [=interest group/user bidding signals=] [=parsing a JSON string to a JavaScript value|parsed to a JavaScript value=].
Copy link
Contributor

Choose a reason for hiding this comment

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

How are you handling failure here? (and other conversion failures in this algorithm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming the data storage only contains valid serialized values, so we shouldn't expect any deserialization/conversion failures. Perhaps technically it's still better to handle failures due to database corruption? but even so, I assume we don't have to include this in the specification. Please correct me if I made any wrong assumptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think specifying how failures are handled is probably one of the most important aspects of a specification. If you look at all the issues with undefined behavior in C/C++...

Please specify how failures are handled. I agree that the user bidding signals should be valid JSON, but there are still failure modes where it isn't (such as when the encoder and decoder are using a different JSON version).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added "return failure" for failed JSON parsing and base64 encode.

@xyaoinum
Copy link
Contributor Author

@brusshamilton I've addressed / responded to your feedback. PTAL again. Thanks!

@xyaoinum
Copy link
Contributor Author

@brusshamilton: PTAL again. Thanks!

@JensenPaul JensenPaul merged commit 665403d into WICG:main Oct 31, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 31, 2024
#1299)

SHA: 665403d
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 morlovich/turtledove that referenced this pull request Nov 1, 2024
WICG#1299)

SHA: 665403d
Reason: push, by morlovich

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