Skip to content
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 host_permissions and optional_host_permissions #221

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

carlosjeurissen
Copy link
Contributor

@carlosjeurissen carlosjeurissen commented May 31, 2022

As each vendor agreed to add optional_host_permissions and thus also host_permissions in manifest v3, it's time to add it to this repo.

Background issue about supporting optional_host_permissions: #119


Preview | Diff

Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested a small change, but beyond that this LGTM.

index.bs Outdated Show resolved Hide resolved
Comment on lines +84 to +99
### Key `permissions`

This key may be present.

### Key `optional_permissions`

This key may be present.

### Key `host_permissions`

This key may be present.

### Key `optional_host_permissions`

This key may be present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We may want to consider sorting the list of keys alphabetically or by another well-defined scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list was already not sorted alphabetically. Grouped the permissions-related keys together. Logical grouping seems more useful than alphabetically sorting the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the casing of the Manifest V3 mention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefaced this with "nit" because it was a small, relatively unimportant observation. I was mostly just thinking out loud. I mostly agree with the intent behind grouping related keys.

That said, but for readers that aren't clear on the association between proximity-based groups I worry that it can end up feeling like a random collection rather than an intentionally curated list. One way we could address this is by making groups part of the heading structure of the document, but I don't feel strongly enough about it to tackle that concern in this PR or likely even this year.

Co-authored-by: Simeon Vincent <[email protected]>
Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for taking this on, @carlosjeurissen.

Comment on lines +84 to +99
### Key `permissions`

This key may be present.

### Key `optional_permissions`

This key may be present.

### Key `host_permissions`

This key may be present.

### Key `optional_host_permissions`

This key may be present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefaced this with "nit" because it was a small, relatively unimportant observation. I was mostly just thinking out loud. I mostly agree with the intent behind grouping related keys.

That said, but for readers that aren't clear on the association between proximity-based groups I worry that it can end up feeling like a random collection rather than an intentionally curated list. One way we could address this is by making groups part of the heading structure of the document, but I don't feel strongly enough about it to tackle that concern in this PR or likely even this year.

Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

@dotproto dotproto merged commit abcedeb into w3c:main Jul 7, 2022
github-actions bot added a commit that referenced this pull request Jul 7, 2022
…sions

SHA: abcedeb
Reason: push, by @dotproto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants