Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove permission check #326
Remove permission check #326
Changes from 6 commits
3b695c2
f70fd9e
0972342
04d0d22
b455083
0b3b13c
3152f77
a94e74d
f521b67
bcb1470
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we still need to queue a global task here? We're already in the main thread at this point.
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.
We don't technically need to but this provides implementations with the freedom to not return a settled Promise synchronously from this method, as otherwise this method doesn't need to be Promise-returning at all.
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.
Wouldn't this be a case of "we thought there should be async stuff happening, now there's not but we can't break backwards compatibility"? It's how we ended up with WakeLockSentinel.release() returning a resolved promise after #299, for example.
I'm not saying I'm in favor of doing that, at this point I'm more curious about whether this is standard practice, or even if I'm looking at it from the wrong perspective.
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.
Without these steps, I think you can drop the "run the following steps in parallel above", as all the remaining sub-steps are run from a queued global task.
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.
Ah, good suggestion!
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.
Acquiring the lock needs to happen in parallel.
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.
Something went wrong with the merge in these steps too: all the main substeps within the "Queue a global task" step have been duplicated inside the If document.[[ActiveLocks]]["screen"] is empty substep (see the Preview link).