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.

Sign up for GitHub

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

Jump to bottom

Restrict Notifications to secure contexts. #176

Merged
merged 10 commits into from
Nov 23, 2021
Merged

Restrict Notifications to secure contexts. #176

merged 10 commits into from
Nov 23, 2021

Conversation

miketaylr
Copy link
Member

@miketaylr miketaylr commented Nov 19, 2021 •
edited by pr-preview bot

Continuing the mess I made in #175.

Fixes #93

Preview | Diff

@miketaylr
Copy link
Member Author

(this will be even simpler with w3c/permissions@a2464fb)

@miketaylr miketaylr changed the title [WIP] Remove Permission model section. Restrict Notifications to secure contexts. Nov 19, 2021
@miketaylr miketaylr marked this pull request as ready for review November 19, 2021 15:16
@miketaylr
Copy link
Member Author

PTAL @annevk @marcoscaceres

annevk
annevk approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks great. Just a couple of nits.

notifications.bs Outdated Show resolved Hide resolved
notifications.bs Outdated Show resolved Hide resolved
notifications.bs Outdated Show resolved Hide resolved
@miketaylr @annevk
Apply @annevk's suggestions.
bbedb83 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@annevk
Copy link
Member

annevk commented Nov 22, 2021

I tried to think through this again. How come Notification.permission does not end up prompting, but Notification.requestPermission() does? Wouldn't that require passing some kind of flag when obtaining the permission? The other places shouldn't end up prompting either, by the way.

annevk
annevk reviewed Nov 22, 2021
View reviewed changes
notifications.bs Outdated Show resolved Hide resolved
@miketaylr
Copy link
Member Author

I tried to think through this again. How come Notification.permission does not end up prompting, but Notification.requestPermission() does?

Hmm, you're right - this isn't quite what we want. I think we should call https://w3c.github.io/permissions/#dfn-request-permission-to-use directly inside requestPermission instead of get the notifications permission state. The other places should be fine then.

annevk
annevk reviewed Nov 22, 2021
View reviewed changes
notifications.bs Outdated Show resolved Hide resolved
notifications.bs Outdated Show resolved Hide resolved
notifications.bs Outdated Show resolved Hide resolved
@miketaylr @annevk
Another round o' Anne's suggested tweaks.
4eff822 
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@annevk annevk merged commit 01e8216 into whatwg:main Nov 23, 2021
@annevk
Copy link
Member

annevk commented Nov 23, 2021

Thanks a lot @miketaylr for getting this cleaned up!

@miketaylr miketaylr deleted the issues/93/4 branch November 23, 2021 16:55
@miketaylr
Copy link
Member Author

Thanks a lot @miketaylr for getting this cleaned up!

Thanks for patiently guiding me through it. :)

jeremyroman added a commit to jeremyroman/nav-speculation that referenced this pull request Nov 25, 2021
@jeremyroman
Update notifications integration in prerendering.
607353c 
This resolves a build issue after whatwg/notifications#176.
@jeremyroman jeremyroman mentioned this pull request Nov 25, 2021
Merged
jeremyroman added a commit to WICG/nav-speculation that referenced this pull request Nov 29, 2021
@jeremyroman
Update notifications integration in prerendering. (#94)
0209a01 
This resolves a build issue after whatwg/notifications#176.
@annevk annevk mentioned this pull request Oct 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restrict the Notifications API to secure contexts
2 participants
@miketaylr @annevk