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

Rewrite the permissions logic to integrate with the permissions API. #200

Merged
merged 2 commits into from
Jul 20, 2020

Conversation

mkruisselbrink
Copy link
Contributor

@mkruisselbrink mkruisselbrink commented Jul 14, 2020

This is partially editorial (just changing how algorithms check
permissions), but also has a couple of normative changes:

  • A change from boolean writable to a enum, with currently "read"
    and "readwrite" options. This is to keep open the possibility for
    write-only handles as discussed in Support write-only handles #119.
  • Changes from NotAllowedError to SecurityError in a couple of places,
    to better align with how other APIs behave.
  • And of course the integration with navigator.permissions.query,
    although that is unlikely to be implemented in chrome any time soon,
    as a lot of the infra for that is missing in chrome.

This fixes #120.


Preview | Diff

This is partially editorial (just changing how algorithms check
permissions), but also has a couple of normative changes:

- A change from `boolean writable` to a enum, with currently "read"
  and "readwrite" options. This is to keep open the possibility for
  write-only handles as discussed in #119.
- Changes from NotAllowedError to SecurityError in a couple of places,
  to better align with how other APIs behave.
- And of course the integration with navigator.permissions.query,
  although that is unlikely to be implemented in chrome any time soon,
  as a lot of the infra for that is missing in chrome.

This fixes #120.
@pwnall pwnall self-requested a review July 15, 2020 20:00
Copy link
Collaborator

@pwnall pwnall left a comment

Choose a reason for hiding this comment

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

LGTM w/ two comments.

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@mkruisselbrink mkruisselbrink merged commit a4a44a8 into master Jul 20, 2020
@mkruisselbrink mkruisselbrink deleted the permissions-integration branch July 20, 2020 18:05
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2020
This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 27, 2020
This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792004}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 27, 2020
This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792004}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 1, 2020
…ing spec change., a=testonly

Automatic update from web-platform-tests
[NativeFS] Update permissions API following spec change.

This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792004}

--

wpt-commits: e85c3405f1269a2ca77ec9e4ea63a4df73ee754e
wpt-pr: 24688
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Aug 1, 2020
…ing spec change., a=testonly

Automatic update from web-platform-tests
[NativeFS] Update permissions API following spec change.

This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792004}

--

wpt-commits: e85c3405f1269a2ca77ec9e4ea63a4df73ee754e
wpt-pr: 24688
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Aug 3, 2020
…ing spec change., a=testonly

Automatic update from web-platform-tests
[NativeFS] Update permissions API following spec change.

This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <taptedchromium.org>
Reviewed-by: Will Harris <wfhchromium.org>
Reviewed-by: Victor Costan <pwnallchromium.org>
Commit-Queue: Marijn Kruisselbrink <mekchromium.org>
Cr-Commit-Position: refs/heads/master{#792004}

--

wpt-commits: e85c3405f1269a2ca77ec9e4ea63a4df73ee754e
wpt-pr: 24688

UltraBlame original commit: 6e390caa95e19e686e5068f51f22537f3a1694b7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Aug 3, 2020
…ing spec change., a=testonly

Automatic update from web-platform-tests
[NativeFS] Update permissions API following spec change.

This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <taptedchromium.org>
Reviewed-by: Will Harris <wfhchromium.org>
Reviewed-by: Victor Costan <pwnallchromium.org>
Commit-Queue: Marijn Kruisselbrink <mekchromium.org>
Cr-Commit-Position: refs/heads/master{#792004}

--

wpt-commits: e85c3405f1269a2ca77ec9e4ea63a4df73ee754e
wpt-pr: 24688

UltraBlame original commit: 6e390caa95e19e686e5068f51f22537f3a1694b7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Aug 3, 2020
…ing spec change., a=testonly

Automatic update from web-platform-tests
[NativeFS] Update permissions API following spec change.

This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <taptedchromium.org>
Reviewed-by: Will Harris <wfhchromium.org>
Reviewed-by: Victor Costan <pwnallchromium.org>
Commit-Queue: Marijn Kruisselbrink <mekchromium.org>
Cr-Commit-Position: refs/heads/master{#792004}

--

wpt-commits: e85c3405f1269a2ca77ec9e4ea63a4df73ee754e
wpt-pr: 24688

UltraBlame original commit: 6e390caa95e19e686e5068f51f22537f3a1694b7
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This implements the changes from WICG/file-system-access#200.

Change-Id: I9702f9888c4f0fff2084143ac6077b8484dfce16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298296
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#792004}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 588cb247d7dc0b4356b8bb4b34c35632a5f123b4
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.

When is user activation / prompting required?
3 participants