- From: Chris Fredrickson <notifications@github.com>
- Date: Thu, 14 Aug 2025 10:46:36 -0700
- To: w3c/permissions <permissions@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/permissions/pull/468/review/3121426419@github.com>
@cfredric commented on this pull request.
> @@ -805,7 +805,8 @@ <h3 id="reading-current-states">
</ol>
</li>
<li>Let |key| be the result of [=powerful feature/permission key generation
- algorithm|generating a permission key=] for |descriptor| with |settings|.
+ algorithm|generating a permission key=] with |settings|'s [=origin=] and |settings|'s
We need to keep `for descriptor` here, since that's how the algorithm knows *which* `permission key generation algorithm` to invoke.
(Think of this as if we are invoking a virtual method: we need to specify the class instance on which to invoke the method, otherwise there's no way to tell which method override should execute.)
> <li>Let |key| be the result of [=powerful feature/permission key generation
- algorithm|generating a permission key=] with the [=current settings object=].
+ algorithm|generating a permission key=] with |settings|'s [=origin=] and
Similarly here - we should include `for descriptor` to disambiguate which particular `permission key generation algorithm` we're invoking, since there are many. (This is a pre-existing problem in the spec, though.)
> </p>
<ol>
- <li>Let |target origin| be [=current settings object=]'s [=environment settings
- object/origin=] if |origin| is null, or |origin| otherwise.
+ <li>Let |target key| be the result of [=powerful feature/permission key generation
+ algorithm|generating a permission key=] with [=current settings object=]'s [=environment
+ settings object/origin=] and [=current settings object=]'s [=environment/top-level
+ origin=] if |key| is null, or |key| otherwise.
+ </li>
+ <li>Let |settings list| be a <a>list</a> containing all [=environment settings objects=]
+ which belong to the |user agent| if provided, or all user agents otherwise.
+ </li>
+ <li>Let |targets| be an empty <a>list</a>.
+ </li>
+ <li>For each [=environment settings object=] |settings| in |settings list|:
Technically we're supposed to define how to do this loop (https://infra.spec.whatwg.org/#list-iterate).
```suggestion
<li>[=For each=] [=environment settings object=] |settings| in |settings list|:
```
> - <li>Let |target origin| be [=current settings object=]'s [=environment settings
- object/origin=] if |origin| is null, or |origin| otherwise.
+ <li>Let |target key| be the result of [=powerful feature/permission key generation
+ algorithm|generating a permission key=] with [=current settings object=]'s [=environment
+ settings object/origin=] and [=current settings object=]'s [=environment/top-level
+ origin=] if |key| is null, or |key| otherwise.
+ </li>
+ <li>Let |settings list| be a <a>list</a> containing all [=environment settings objects=]
+ which belong to the |user agent| if provided, or all user agents otherwise.
+ </li>
+ <li>Let |targets| be an empty <a>list</a>.
+ </li>
+ <li>For each [=environment settings object=] |settings| in |settings list|:
+ <ol>
+ <li>Let |settings key| be be the result of [=powerful feature/permission key generation
+ algorithm|generating a permission key=] with |settings|'s [=origin=] and |settings|'s
```suggestion
algorithm|generating a permission key=] for |descriptor| with |settings|'s [=origin=] and |settings|'s
```
> + <li>[=list/Append=] |settings| to |targets| if |settings key| matches
+ |target key| according to the [=powerful feature/permission key comparison
+ algorithm=].
I'd reorder this so that the condition is first, for clarity. (Some languages do support postfix if statements (E.g. Perl: `print "you win!" if $score > 100;`), but that's pretty unusual.) Some people are stricter than others about this sort of thing in specs.
The spec should also be precise about what it means to "match" here, since that's not defined anywhere yet; i.e., we have to specify exactly how to invoke the algorithm and what to do with the result.
```suggestion
<li>Let |matches| be the result of running the [=permission key/permission key comparison algorithm=] for |descriptor|, given |settings key| and |key|.
<li>If |matches|, then [=list/append=] |settings| to |targets|.
```
> @@ -1437,6 +1457,7 @@ <h6 id="webdriver-bidi-command-permissions-setPermission">
descriptor: permissions.PermissionDescriptor,
state: permissions.PermissionState,
origin: text,
+ ? embeddingOrigin: text,
Should we stick with the terminology used by `environment` and call this `topLevelOrigin`? IMO that would be more consistent.
https://html.spec.whatwg.org/#concept-environment-top-level-origin
> @@ -1476,10 +1497,16 @@ <h6 id="webdriver-bidi-command-permissions-setPermission">
</li>
<li>Let |origin| be the value of the `origin` field of |command parameters|.
</li>
+ <li>Let |embedding origin| be the value of the `embeddingOrigin` field of
+ |command parameters|, if present, and |origin| otherwise.
+ </li>
+ <li>Let |key| be the result of [=powerful feature/permission key generation
+ algorithm|generating a permission key=] with |origin| and |embedding origin|.
```suggestion
algorithm|generating a permission key=] for |descriptor| with |origin| and |embedding origin|.
```
> @@ -639,10 +639,10 @@ <h2>
<div class="algorithm">
<p>
The <dfn class="export">default permission key generation algorithm</dfn>, given an
- [=environment settings object=] |settings|, runs the following steps:
+ |origin| and a |top level origin|, runs the following steps:
Please specify the types of the arguments, as well as their names.
```suggestion
[=origin=] |origin| and an [=origin=] |top level origin|, runs the following steps:
```
> @@ -805,7 +805,8 @@ <h3 id="reading-current-states">
</ol>
</li>
<li>Let |key| be the result of [=powerful feature/permission key generation
- algorithm|generating a permission key=] for |descriptor| with |settings|.
+ algorithm|generating a permission key=] with |settings|'s [=origin=] and |settings|'s
`[=origin=]` is the data type, but we need to use a reference to the `environment/origin` field here in order to know which `origin` instance to pass as an argument:
```suggestion
algorithm|generating a permission key=] with |settings|'s [=environment settings object/origin=] and |settings|'s
```
> <li>Let |key| be the result of [=powerful feature/permission key generation
- algorithm|generating a permission key=] with the [=current settings object=].
+ algorithm|generating a permission key=] with |settings|'s [=origin=] and
Same here, this should be a reference to the `environment settings object/origin` field.
```suggestion
algorithm|generating a permission key=] with |settings|'s [=environment settings object/origin=] and
```
> </p>
<ol>
- <li>Let |target origin| be [=current settings object=]'s [=environment settings
- object/origin=] if |origin| is null, or |origin| otherwise.
+ <li>Let |target key| be the result of [=powerful feature/permission key generation
+ algorithm|generating a permission key=] with [=current settings object=]'s [=environment
Same here, we need to disambiguate which `permission key generation algorithm` we want to invoke.
```suggestion
algorithm|generating a permission key=] for |descriptor| with [=current settings object=]'s [=environment
```
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/permissions/pull/468#pullrequestreview-3121426419
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/permissions/pull/468/review/3121426419@github.com>
Received on Thursday, 14 August 2025 17:46:40 UTC