- From: Anne van Kesteren <notifications@github.com>
- Date: Tue, 15 Nov 2022 08:00:52 -0800
- To: w3c/permissions <permissions@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/permissions/pull/390/review/1179601969@github.com>
@annevk commented on this pull request.
Thanks @johannhof for this draft. The general shape of this looks right, but there's a lot of details that need to be written out. I left a bunch of inline comments that I hope are helpful.
> @@ -278,6 +278,93 @@ <h3>
reset because its [=permission/lifetime=] has expired.
</p>
</section>
+ <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
```suggestion
The user agent maintains a single <dfn class="export">permission store</dfn> which is a [=/list=] of [=permission store entries=].
```
Since there's no non-global store I think we can simplify a bit.
> @@ -278,6 +278,93 @@ <h3>
reset because its [=permission/lifetime=] has expired.
</p>
</section>
+ <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
+ The user agent removes [=entries=] from the list when their respective [=permission=]'s [=permission/lifetime=] has expired.
This is a statement of fact, is there a corresponding requirement somewhere?
(It's also news to me that permissions have a lifetime.)
> @@ -278,6 +278,93 @@ <h3>
reset because its [=permission/lifetime=] has expired.
</p>
</section>
+ <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
+ The user agent removes [=entries=] from the list when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p class="issue">
+ We should still allow UAs flexibility in giving out permissions with smaller scope and lifetime (e.g. per-tab) in some form. There should generally still be enough room for exploration here.
Yeah, `[=implementation-defined=]` is our friend here.
> @@ -278,6 +278,93 @@ <h3>
reset because its [=permission/lifetime=] has expired.
</p>
</section>
+ <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
+ The user agent removes [=entries=] from the list when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p class="issue">
+ We should still allow UAs flexibility in giving out permissions with smaller scope and lifetime (e.g. per-tab) in some form. There should generally still be enough room for exploration here.
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] name, [=permission store key=] key, {{PermissionDescriptor}} descriptor, and [=permission/state=] state.
Although a bit painful markup-wise to do, we should `<dfn>` all of these members so they can be linked.
> + <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
+ The user agent removes [=entries=] from the list when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p class="issue">
+ We should still allow UAs flexibility in giving out permissions with smaller scope and lifetime (e.g. per-tab) in some form. There should generally still be enough room for exploration here.
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] name, [=permission store key=] key, {{PermissionDescriptor}} descriptor, and [=permission/state=] state.
+ </p>
+ <p>
+ To <dfn class="export">get a permission store entry</dfn> from the [=global permission store=] given a |name|, [=permission store key=] key and descriptor, run these steps:
Since the global permission store is not an algorithm I don't think we should reference it here. And instead we should use
> ... the user agent's permission store ...
inside the algorithm to make it clear we're grabbing a global variable.
> + </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">global permission store</dfn> which is a list of [=permission store entries=].
+ The user agent removes [=entries=] from the list when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p class="issue">
+ We should still allow UAs flexibility in giving out permissions with smaller scope and lifetime (e.g. per-tab) in some form. There should generally still be enough room for exploration here.
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] name, [=permission store key=] key, {{PermissionDescriptor}} descriptor, and [=permission/state=] state.
+ </p>
+ <p>
+ To <dfn class="export">get a permission store entry</dfn> from the [=global permission store=] given a |name|, [=permission store key=] key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ If the [=global permission store=] [=list/contains=] an [=entry=] with the name |name|, the key |key| and the descriptor |descriptor|, return that entry.
> an entry whose name is _name_, key is _key_, and descriptor is _descriptor_
Although can you really compare a key and a descriptor in that way? They probably need some equals operation?
> + <p>
+ To <dfn class="export">get a permission store entry</dfn> from the [=global permission store=] given a |name|, [=permission store key=] key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ If the [=global permission store=] [=list/contains=] an [=entry=] with the name |name|, the key |key| and the descriptor |descriptor|, return that entry.
+ </li>
+ <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the [=global permission store=] given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with |name|, |key|, |descriptor|, and |state|.
Nit: you actually need to set the individual members here (or define some kind of constructor algorithm that takes these as parameters).
> + <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the [=global permission store=] given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with |name|, |key|, |descriptor|, and |state|.
+ </li>
+ <li>
+ If the [=global permission store=] [=list/contains=] an [=entry=] with the name |name|, the key |key| and the descriptor |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
+ </li>
+ <li>
+ Otherwise, [=list/append=] |newEntry| to the [=global permission store=].
No need to say Otherwise.
> + </li>
+ <li>
+ Otherwise, [=list/append=] |newEntry| to the [=global permission store=].
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">remove a permission store entry</dfn> from the [=global permission store=] given a name, key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ [=list/Remove=] the [=entry=] with the name |name|, key |key| and descriptor |descriptor| from the [=global permission store=].
+ </li>
+ </ol>
+ </p>
+ <p>
+ A <dfn class="export">permission store key</dfn> is a [=tuple=] of ([=origin=] top-level origin, [=origin=] embedded origin).
Should this be top-level site? This should probably also have an implementation-defined field user agents can use for finer granularity?
> + </ol>
+ </p>
+ <p>
+ To <dfn class="export">remove a permission store entry</dfn> from the [=global permission store=] given a name, key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ [=list/Remove=] the [=entry=] with the name |name|, key |key| and descriptor |descriptor| from the [=global permission store=].
+ </li>
+ </ol>
+ </p>
+ <p>
+ A <dfn class="export">permission store key</dfn> is a [=tuple=] of ([=origin=] top-level origin, [=origin=] embedded origin).
+ </p>
+
+ <p>
+ To <dfn class="export">generate a permission store key</dfn> `key` given [=environment settings object=] |settings|, run these steps:
Nit: `key` isn't part of this:
```suggestion
To <dfn class="export">generate a permission store key</dfn> given [=environment settings object=] |settings|, run these steps:
```
> + [=list/Remove=] the [=entry=] with the name |name|, key |key| and descriptor |descriptor| from the [=global permission store=].
+ </li>
+ </ol>
+ </p>
+ <p>
+ A <dfn class="export">permission store key</dfn> is a [=tuple=] of ([=origin=] top-level origin, [=origin=] embedded origin).
+ </p>
+
+ <p>
+ To <dfn class="export">generate a permission store key</dfn> `key` given [=environment settings object=] |settings|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |top-level origin| be |settings|' [=environment/top-level origin=].
+ </li>
+ <li>
+ Return (|top-level origin|, null)
```suggestion
Return (|top-level origin|, null).
```
> + To <dfn class="export">remove a permission store entry</dfn> from the [=global permission store=] given a name, key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ [=list/Remove=] the [=entry=] with the name |name|, key |key| and descriptor |descriptor| from the [=global permission store=].
+ </li>
+ </ol>
+ </p>
+ <p>
+ A <dfn class="export">permission store key</dfn> is a [=tuple=] of ([=origin=] top-level origin, [=origin=] embedded origin).
+ </p>
+
+ <p>
+ To <dfn class="export">generate a permission store key</dfn> `key` given [=environment settings object=] |settings|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |top-level origin| be |settings|' [=environment/top-level origin=].
Not sure what is locally consistent, but WHATWG in general uses _topLevelOrigin_ for variable casing.
> + embedded origin field.
+ </p>
+ <p class="note">
+ This assumes that passed in settings is from the embedded document.
+ </p>
+ </ol>
+ </p>
+
+ <p>
+ To <dfn class="export">compare [=permission store keys=]</dfn> |key1| and |key2|, run these steps:
+ <ol class="algorithm">
+ <li>
+ </li>
+ </ol>
+ <p class="issue">
+ Do we have to define this? If we can compare origins we can probably implicitly compare a tuple of origins?
To compare origins you have to invoke a particular comparison operation (there's multiple) so there's nothing implicit about it. It's not hard, but you do have to write the algorithm.
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/permissions/pull/390#pullrequestreview-1179601969
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/permissions/pull/390/review/1179601969@github.com>
Received on Tuesday, 15 November 2022 16:01:06 UTC