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

Explainer change for separate rate-limits for embedded site #1457

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

Explainer change for separate rate-limits for embedded site #1457

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6c6a38
Explainer change for separate rate-limits for embedded site
linnan-github Oct 28, 2024
be64e46
Update verbose_debugging_reports.md
linnan-github Oct 29, 2024
29e0579
Update verbose_debugging_reports.md
linnan-github Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions EVENT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,10 @@ were reported.
In order to achieve the privacy goals listed above the API has various rate limits, which can be found [here](https://github.com/WICG/attribution-reporting-api/blob/main/params/chromium-params.md) for Chromium.
Note: Browsers may choose to allow separate rate limits for the embedded site
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is embedded site standard terminology? If so, let's link to a definition of it. Otherwise, let's explain further. Same for the spec changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, IIUC the spec changes aren't limited to rate limits. Rather, they effectively replace the context origin in all uses across the entire spec.

For sources, this includes "max pending sources per source origin" and "max destinations covered by unexpired sources", which aren't explicitly considered "rate limits."

It also includes the source site in verbose debug report bodies.

For aggregatable reports, it affects the effective attribution destination and therefore "max aggregatable attribution reports per attribution destination".

IMO we should make it explicit that the context origin is entirely replaced, not just for rate-limit purposes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used “rate limits” to cover those limits as well. The destination limit is also discussed in this section, so I think it’s fine. I can mention storage limit explicitly if you feel strongly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The aforementioned limits aren't rate limits, as they do not expire after a specific window, and they are mentioned in different parts of the spec and explainer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason the explainer uses embedded site but the spec uses embedded origin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That’s just because the explainer mentions “top-level site” in that section, whereas the spec change is where we get the origin.

for specific use cases (for example, [AMP](https://amp.dev) pages rendered by
an AMP cache/viewer provider).
### Trigger Data
Trigger data, e.g. advertiser-side data, is extremely important for critical use
Expand Down
2 changes: 2 additions & 0 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,6 +1832,8 @@ To <dfn>check if cookie-based debugging is allowed</dfn> given a
To obtain the <dfn export for=node>context origin</dfn> of a [=node=] |node|, return |node|'s [=node navigable=]'s
[=navigable/top-level traversable=]'s [=navigable/active document=]'s [=origin=].

Note: The user agent MAY return the embedded origin for specific use cases.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this belongs in a note. We can instead phrase it as:

1. Optionally, return the embedded origin if it exists.
2. Return |node|'s ...

And then explain the reasoning or what an embedded origin is in the note.


<h3 id="obtaining-randomized-response">Obtaining a randomized response</h3>

To <dfn>obtain a randomized response</dfn> given |trueValue|, a [=set=] |possibleValues|, and a
Expand Down
Loading