-
Notifications
You must be signed in to change notification settings - Fork 178
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
Explainer change for separate rate-limits for embedded site #1457
base: main
Are you sure you want to change the base?
Conversation
@jolynyao Could you PTAL, thanks! |
5144c38
to
f6c6a38
Compare
LGTM thanks @linnan-github! |
29e0579
to
22edc9a
Compare
22edc9a
to
8a0e156
Compare
index.bs
Outdated
[=navigable/top-level traversable=]'s [=navigable/active document=]'s [=origin=]. | ||
To obtain the <dfn export for=node>context origin</dfn> of a [=node=] |node|: | ||
|
||
1. Optionally, return the embedded origin if it exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be helpful for people reading the spec to have some definition of embedded origin
here, either in a note or in a link.
index.bs
Outdated
[=navigable/top-level traversable=]'s [=navigable/active document=]'s [=origin=]. | ||
To obtain the <dfn export for=node>context origin</dfn> of a [=node=] |node|: | ||
|
||
1. Optionally, return the embedded origin if it exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does optionally
here mean that different implementations can choose whether they care about embedded origins and still be compliant with the specification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the intention to leave it defined by the user agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should clarify that. Otherwise, it could be interpreted as "some of the time, do this," when it really means that the implementation can choose whether it supports the notion of embedded origins, but if it does, this algorithm should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also apply to the case "some of the time, do this". For example, if we only want to do this for sources but not triggers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the point is that the algorithm should be consistent under particular static circumstances. For example, it is not in the spirit of this specification for the implementation to generate a random number to decide whether to return the embedded origin or actual origin.
index.bs
Outdated
Note: The user agent may return the embedded origin to allow separate limits | ||
for specific use cases. Embedded origin could be the effective top-level origin, | ||
e.g. the origin of the website displayed in the AMP viewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think someone reading this is going to understand what an embedded origin is, as it's not a standard term as far as I know. An example would definitely help, but we should also avoid referring to AMP in the spec if possible. Maybe we can give a neutral example like comparing https://foo.example/
vs https://foo.example/bar.example
?
[=navigable/active document=]'s [=origin=]. | ||
|
||
Note: The user agent may return the embedded origin to allow separate limits | ||
for specific use cases. Embedded origin could be the effective top-level origin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for specific use cases. Embedded origin could be the effective top-level origin. | |
for specific use cases. The embedded origin could be the effective top-level origin. |
For example, https://foo.example/bar.example is embedded within https://foo.example, | ||
where https://foo.example is the top-level origin and https://bar.example is the embedded origin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, https://foo.example/bar.example is embedded within https://foo.example, | |
where https://foo.example is the top-level origin and https://bar.example is the embedded origin. | |
For example, a URL like `https://foo.example/bar.example` could be interpreted as embedding the origin `https://bar.example`. |
Fixes #1452
Preview | Diff