-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add new web lock api interface #3604
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure exactly what happened here, did you check the CI failure?
Could you take a look at the documentation and see if that generates the code correctly?
- Looking at the reference there are a couple of APIs still missing.
- A changelog entry is missing.
It would also be nice if you link your sources. E.g. double-checking the Mozilla implementation to see if any functions are throwing.
Otherwise LGTM!
Hi @daxpedda, thanks for your review, It is my fault that I wrote the |
I only meant posting it here in the issue! But that's for the next time, I already got the links and posted them above 😉. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Changelog entry is missing.
Hi @daxpedda thanks for the reminder, I added the entry to the changelog, if the entry description is not correct, please tell me, thanks ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
fixes #3456.
this is the first time I have contributed to this repo, If there is some definition that is wrong, please tell me! thanks ~