-
Notifications
You must be signed in to change notification settings - Fork 60
Add top-level site and cross site ancestry to storage key #182
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Anne van Kesteren <[email protected]>
Co-authored-by: Anne van Kesteren <[email protected]>
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.
This looks good to me modulo some minor nits. Thanks again for tackling it!
You can consider WebKit interested.
What's remaining:
- Filing bugs against Gecko and WebKit and linking them from OP.
- You can mark the MDN checkbox and say N/A.
- Have you checked whether all the dependencies of this specification are holding the storage key correctly or are any doing something special with the origin field, for instance?
- Is there a PR to rename the tests away from .tentative? (Assuming that there's nothing further that needs updating in the relevant specifications.)
- The HTML PR needs to land first. I haven't really checked what's blocking that one yet.
cc @asutherland
storage.bs
Outdated
<a href="https://privacycg.github.io/storage-partitioning/">Client-Side Storage Partitioning</a>. | ||
<dfn for="storage key">origin</dfn> (an <a for=/>origin</a>), a | ||
<dfn for="storage key">top-level site</dfn> (a <a for=/>site</a>), and a | ||
<dfn for="storage key">cross-site ancestry</dfn> (a boolean). [[!HTML]] |
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.
Let's rename this to "has cross-site ancestor" to align with what HTML ended up with.
storage.bs
Outdated
<var>topLevelOrigin</var>. | ||
|
||
<li><p>Let <var>crossSiteAncestry</var> be <var>environment</var>'s | ||
<a for=environment>cross-site ancestry</a>. |
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.
@domenic @bvandersloot-mozilla now that has cross-site ancestor is no longer on environment, how is this going to work here?
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.
Um. I think it just never came up in whatwg/html#11133, when I convinced @bvandersloot-mozilla to switch to ESO, that we were hoping to use this for storage keys.
I noticed that we already have a branch in https://storage.spec.whatwg.org/#obtain-a-storage-key-for-non-storage-purposes which treats ESOs specially. Would that work here? Probably not. It would give the wrong answer during the phase when only an environment exists, but not an ESO, such as when navigation has started but not yet gotten as far as creating a Window
object. I could see that phase as needing to look at the storage key, e.g., for service worker lookups.
So... unless there's something that's missing, I think I really screwed up here, and we should go back to @bvandersloot-mozilla's original approach. It is much more complex, but it seems to be necessary, if we're planning to use this during the liminal pre-ESO time.
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's been a long time since I looked at all this, but I think pre-ESO is important for service worker allocation.
Add top-level site and cross-site ancestry to Storage Key in the Storage spec.
This is taking over #144, and should be landed with or after the "cross site ancestry" is added to the environment in whatwg/html/pull/11133.
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff