-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: enable uninstalling PatchedMediaKeysApple #4471
feat: enable uninstalling PatchedMediaKeysApple #4471
Conversation
Incremental code coverage: 97.83% |
Will re-open once I run some more tests. |
Tested and working. |
I don't understand this PR or the associated issue, and it seems really odd to me. Let's discuss on the issue, but for now, please do not merge the PR. Sorry for the inconvenience. |
Thanks @joeyparrish, I'll look at adjusting according to your feedback and add a test as well as docs |
aaa35a3
to
22b8eaa
Compare
@avelad @joeyparrish I've addressed the feedback, introduced unit tests and added a short blurb in the documentation. I opted for not restricting the unit test to only run on Safari. The functionality that's being tested is just overriding and restoring browser globals. I don't know of a way of checking strict equality on the mediaKeys getter. The test does a best effort check to see if it looks like the value is no longer overridden, but it doesn't guarantee that it was restored. I have not yet run the latest PR updates on a Mac, I'll update the PR once I have. Edit, |
@martinstark It seems that the tests have failed, can you check it? |
@avelad managed to finally get through linting 😅 1) cues ending exactly now
TextDisplayer layout using browser-native rendering
native-end-time-edge-case: Expected 0.9133498736570255 not to be less than 0.95. I don't think this is related to my changes, what do you think? |
I'll re-run the failed jobs |
be7f2b6
to
39b0c54
Compare
@avelad the safari test still fails, I've rebased on the lastest |
1) cues ending exactly now
TextDisplayer layout using browser-native rendering
native-end-time-edge-case: Expected 0.9133498736570255 not to be less than 0.95. Keeps failing with exactly the same number. I've had a brief look at the test in question, as well as the text displayer, but no ideas spring to mind. |
I'll ignore the fail test to approve the PR. Please send the final code please (no comments in the tests)! |
42aa21c
to
841b9a9
Compare
@avelad I tried to remove the unit tests to see if that's what was causing the Safari test suite failure. I've reverted this and squashed everything. Thanks for your time! |
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.
A minor nit, otherwise LGTM
605751c
to
f05c951
Compare
@avelad not sure what your process is, I just squashed and force-pushed after you approval, but that dismisses your review. Any input? |
The squash it's done by github, so really it's not a problem :) I'll approve again |
@joeyparrish can you review it? Thanks! |
Changes addressed, reviewed by Álvaro
Close #4469
The idea is to only enable uninstalling when it is desired, in order to not hold things in memory unnecessarily.