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

Drop duplicate PermissionState definition #88

Merged
merged 1 commit into from Jan 30, 2023
Merged

Conversation

foolip
Copy link
Member

@foolip foolip commented Jul 21, 2020

Fixes #82.


Preview | Diff

foolip added a commit to foolip/webref that referenced this pull request Jul 22, 2020
@rakuco
Copy link
Member

rakuco commented Aug 20, 2020

Is there anything preventing this from being merged? I recall some long discussions about permissions in #57 and #68. WebKit does not define PermissionState in other IDLs, but I guess they can just continue shipping their enum in DeviceOrientationOrMotionPermissionState.idl.

@foolip
Copy link
Member Author

foolip commented Aug 20, 2020

@reillyeon your check mark is green, so I guess you have the power to merge?

Copy link
Member

@reillyeon reillyeon left a comment

Choose a reason for hiding this comment

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

Sorry, looking at this again I realized there was more to fix to make this change.

index.bs Show resolved Hide resolved
queengooborg pushed a commit to foolip/webref that referenced this pull request Nov 18, 2020
queengooborg pushed a commit to foolip/webref that referenced this pull request Dec 14, 2020
foolip added a commit to foolip/webref that referenced this pull request Jan 21, 2021
foolip added a commit to foolip/webref that referenced this pull request Jan 27, 2021
Base automatically changed from master to main February 23, 2021 02:53
@FluorescentHallucinogen

Is there something that prevents from merging this?

@reillyeon
Copy link
Member

Is there something that prevents from merging this?

There's an unresolved comment regarding some additional specification text that should be updated to keep things consistent.

@rakuco
Copy link
Member

rakuco commented Jan 20, 2023

@reillyeon following https://www.w3.org/2022/09/15-dap-minutes.html I wonder if this can be merged as-is?

This would allow us to get rid of the duplicate PermissionState enum (WebKit even has a PermissionState.idl now), which is the biggest issue.

We can replace "default" with "prompt" in another PR (the term is not exposed to script anyway), and file an issue to discuss further integration with the Permissions spec, as I imagine that will be a lot more work from a spec perspective.

@reillyeon
Copy link
Member

@rakuco I took another careful look at this and I think that even though the "default" value isn't in the PermissionState enum from the Permissions API the way that it is used in this spec doesn't technically expose that to script and so it's fine to land this change as is. We can fix it up in a follow-up which adds proper Permissions API integration and so removes all the existing spec text that uses the "default" value.

@rakuco
Copy link
Member

rakuco commented Jan 30, 2023

Thanks for reviewing it again! Can either you or @anssiko merge the PR? I don't have commit rights to this repository.

@anssiko anssiko merged commit 7e1e1d9 into w3c:main Jan 30, 2023
github-actions bot added a commit that referenced this pull request Jan 30, 2023
SHA: 7e1e1d9
Reason: push, by anssiko

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.

PermissionState is already defined in Permission API
6 participants