-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add draft of requestStorageAccessForSite spec #109
Conversation
tagging @johannhof to start with |
storage-access.bs
Outdated
@@ -163,6 +172,7 @@ To <dfn type="abstract-op">save the storage access flag set</dfn> for a [=partit | |||
partial interface Document { | |||
Promise<boolean> hasStorageAccess(); | |||
Promise<undefined> requestStorageAccess(); | |||
Promise<undefined> requestStorageAccessForSite(USVString site); |
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.
Instead of requestStorageAccessForSite(site)
, which introduces a new API and uses positional parameters, what if we added an options object to requestStorageAccess
?
For example:
await document.requestStorageAccess({
site: "https://example.com"
})
This would help reduce potential confusion between requestStorageAccess
and requestStorageAccessForSite
(especially considering requestStorageAccess
always autogrants in the top frame), and makes it so that future enhancements can be easily added without relying on positional parameters.
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.
Thanks for the feedback! I was viewing this as similar to document.querySelector
vs document.querySelectorAll
. A benefit I see of specifying a separate function is that it is easy to feature detect a new function, whereas it is less simple to detect whether that function would accept a parameter. There’s also the classic concern about getting property names right on a given call (i.e., document.requestStorageAccess({site:'a.com'})
vs document.requestStorageAccess({url:'a.com'})
), though this is probably minor. I think it might also be simpler to have separate documentation for the two call patterns; requestStorageAccess
is already somewhat complicated in how it works, and this would be one more thing for developers to remember. I think not specifying a site doesn't really make sense at the top level anyway, which might help with your concern about mixing up the two calls. All this said, I don’t feel strongly. I'd definitely love to hear counterarguments or other opinions.
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.
A quick note that we're still discussing general API shape, hopefully at TPAC, and are thus not looking at merging this yet. However, I left some comments from an initial review :)
storage-access.bs
Outdated
@@ -77,7 +77,7 @@ urlPrefix: https://w3c.github.io/webdriver/webdriver-spec.html#; spec: webdriver | |||
|
|||
User Agents sometimes prevent content inside certain <{iframe}>s from accessing data stored in client-side storage mechanisms like cookies. This can break embedded content which relies on having access to client-side storage. | |||
|
|||
The Storage Access API enables content inside <{iframe}>s to request and be granted access to their client-side storage, so that embedded content which relies on having access to client-side storage can work in such User Agents. [[STORAGE-ACCESS-INTRO]] | |||
The Storage Access API enables content inside <{iframe}>s to request and be granted access to their client-side storage, so that embedded content which relies on having access to client-side storage can work in such User Agents. It also provides a mechanism to allow top-level sites to request such access on behalf of embedded sites. [[STORAGE-ACCESS-INTRO]] |
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 feel like we could try to refactor this sentence slightly so that it starts with a more generic definition such as "enables developers to request access to client-side storage for embedded resources such as iframes, scripts or images. It does this through two mechanisms:..."
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.
attempted to make it sound like this in the latest...let me know how it sounds
storage-access.bs
Outdated
@@ -139,6 +139,15 @@ To <dfn type="abstract-op">generate a partitioned storage key</dfn> for a {{Docu | |||
1. Let |top-level site| be the result of [=obtain a site|obtaining a site=] from |settings|' [=top-level origin=]. | |||
1. Return the [=partitioned storage key=] (|top-level site|, |site|). | |||
|
|||
To <dfn type="abstract-op">generate an overridden partitioned storage key</dfn> for a {{Document}} |doc| and an [=url/origin=] |override|, run the following steps: |
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.
Hmm "overridden" as a word feels a bit unintuitive here but I struggle to come up with something better. Was wondering if you had any alternatives you were considering while writing this.
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.
In the latest commit, I tried delegated
, though I am not sure it's better.
Other options:
- split this into steps that are passed both URL arguments, i.e., the equivalent of
generate_partitioned_key(top_level_site, embedded_origin)
, and then have both methods use the new function, rather than taking a document. The downside would be that bothrequestStorageAccess
andrequestStorageAccessForOrigin
would have steps to extract the URLs, whereas they can be combined somewhat this way. - besides
delegated
andoverridden
, I suppose we could considerforwarded
or something. My thesaurus lookups didn't help much, unfortunately.
storage-access.bs
Outdated
To <dfn type="abstract-op">generate an overridden partitioned storage key</dfn> for a {{Document}} |doc| and an [=url/origin=] |override|, run the following steps: | ||
<!--TODO: type of |override|...could be parsed url, USVString, or Origin. The settings objects referenced above appear to use origin and then obtain a site from it--> | ||
1. Let |settings| be |doc|'s [=relevant settings object=]. | ||
<!--TODO: use top-level here, trusting the prior steps to have verified the caller is itself top-level? --> |
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.
You could use https://infra.spec.whatwg.org/#assert?
This has moved to its own proposal in https://github.com/privacycg/requestStorageAccessForOrigin |
This is an early draft; the pull request is therefore being created in draft mode.
would resolve #107
also see the explainer proposal:
https://github.com/mreichhoff/requestStorageAccessForSite
Preview | Diff