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.

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

Merged

Conversation

martinstark
Copy link
Contributor

@martinstark martinstark commented Sep 6, 2022

Close #4469

The idea is to only enable uninstalling when it is desired, in order to not hold things in memory unnecessarily.

@martinstark martinstark changed the title Enable uninstalling PatchedMediaKeysApple feat: enable uninstalling PatchedMediaKeysApple Sep 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Incremental code coverage: 97.83%

@avelad avelad added type: enhancement New feature or request component: FairPlay The issue involves the FairPlay DRM labels Sep 6, 2022
@avelad avelad added this to the v4.3 milestone Sep 6, 2022
@martinstark martinstark marked this pull request as draft September 6, 2022 20:45
@martinstark
Copy link
Contributor Author

Will re-open once I run some more tests.

@martinstark martinstark closed this Sep 6, 2022
@avelad avelad removed this from the v4.3 milestone Sep 7, 2022
@martinstark
Copy link
Contributor Author

martinstark commented Sep 7, 2022

Tested and working.

@martinstark martinstark reopened this Sep 7, 2022
@martinstark martinstark marked this pull request as ready for review September 8, 2022 07:42
@avelad avelad added this to the v4.3 milestone Sep 12, 2022
avelad
avelad previously approved these changes Sep 12, 2022
@joeyparrish
Copy link
Member

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.

lib/polyfill/patchedmediakeys_apple.js Outdated Show resolved Hide resolved
lib/polyfill/patchedmediakeys_apple.js Show resolved Hide resolved
@martinstark
Copy link
Contributor Author

martinstark commented Sep 15, 2022

Thanks @joeyparrish, I'll look at adjusting according to your feedback and add a test as well as docs

@martinstark
Copy link
Contributor Author

martinstark commented Sep 15, 2022

@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,
tested and working

@martinstark martinstark requested review from joeyparrish and avelad and removed request for littlespex, theodab and joeyparrish September 15, 2022 14:25
@avelad
Copy link
Member

avelad commented Sep 15, 2022

@martinstark It seems that the tests have failed, can you check it?

@martinstark
Copy link
Contributor Author

martinstark commented Sep 16, 2022

@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?

@avelad
Copy link
Member

avelad commented Sep 16, 2022

I'll re-run the failed jobs

@martinstark
Copy link
Contributor Author

@avelad the safari test still fails, I've rebased on the lastest main to see if that makes a difference

@martinstark
Copy link
Contributor Author

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.

@avelad
Copy link
Member

avelad commented Sep 16, 2022

I'll ignore the fail test to approve the PR. Please send the final code please (no comments in the tests)!

@martinstark
Copy link
Contributor Author

@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!

Copy link
Member

@avelad avelad left a 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

docs/tutorials/fairplay.md Outdated Show resolved Hide resolved
avelad
avelad previously approved these changes Sep 16, 2022
@martinstark
Copy link
Contributor Author

@avelad not sure what your process is, I just squashed and force-pushed after you approval, but that dismisses your review. Any input?

@avelad avelad self-requested a review September 16, 2022 11:01
@avelad
Copy link
Member

avelad commented Sep 16, 2022

The squash it's done by github, so really it's not a problem :) I'll approve again

@avelad avelad self-requested a review September 19, 2022 14:49
@avelad
Copy link
Member

avelad commented Sep 19, 2022

@joeyparrish can you review it? Thanks!

@avelad avelad dismissed joeyparrish’s stale review September 23, 2022 07:52

Changes addressed, reviewed by Álvaro

@avelad avelad merged commit 7166f0c into shaka-project:main Sep 23, 2022
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: FairPlay The issue involves the FairPlay DRM status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to uninstall PatchedMediaKeysApple polyfill
3 participants