[Screen Wake Lock] Remove web driver set_permission usage #36122
Conversation
|
|
||
| promise_test(t => { | ||
| return promise_rejects_dom(t, "NotAllowedError", navigator.wakeLock.request('screen')); | ||
| }, "Screen wake lock should not be allowed in dedicated worker"); |
There was a problem hiding this comment.
The test was simply wrong (or shouldn't exist). There is nothing in the spec that exposes the API on the WorkerNavigator interface in workers, so running the above would just fail on navigator.wakeLock because .wakeLock is would be undefined.
| @@ -1,21 +0,0 @@ | |||
| // META: script=/resources/testdriver.js | |||
There was a problem hiding this comment.
Why were these tests rewritten? I generally prefer using window.js tests when testing Javascipt APIs at least in part because tools like clang-format know how to format .js files and not .html files.
There was a problem hiding this comment.
See #36122 (comment)
|
@reillyeon, I find the .js tests really hard to debug and reason about. There is too much magic going on with them building a page around them. It also means I can't just double-click on them and run them locally without also spinning up About formatting, I'd love to get agreement on that... personally, I use prettier as it handles both JS and HTML (as well as great error reporting). I'd love for us to reformat all the tests as a followup. |
marcoscaceres
force-pushed
the
remove_perm_requets
branch
from
82e90d3 to
26755a0
Compare
|
@reillyeon What do you think? Is it ready to merge? |
There was a problem hiding this comment.
- The tests were using "set_permission", not "request_permission" like the title of this PR says.
- Without getting into the merit of the changes themselves: I thought this would only be mergeable once the spec was updated accordingly per https://github.com/w3c/screen-wake-lock/blob/gh-pages/.github/PULL_REQUEST_TEMPLATE.md and our usual practice? Otherwise we'll end up with tests that do not match what the spec says.
marcoscaceres
changed the title
Yes. Unfortunately, I can't merge or now modify the corresponding pull request. @rakuco, as spec Editor, can you take that over (i.e., just delete all the permissions stuff)? We have consensus on removing the permission already, so this shouldn't need to drag longer than a few hours. |
Tests for w3c/screen-wake-lock#351
Plus some refactors and removals...