Prevent initial autoplay when disabled#3849
Conversation
30058de to
8226d7a
Compare
|
hi and thank you @KaannKara compare: #3936 pageWorldFiles: Our timing is worth tuning if you will. Some files only need to load if that feature is enabled |
wahajahmed010
left a comment
There was a problem hiding this comment.
Review: Prevent initial autoplay when disabled (#3849)
Great work, @KaannKara — this is a solid approach to solving the autoplay flash issue. The early guard in core.js intercepting HTMLMediaElement.prototype.play is well-placed and the localStorage mirroring trick is clever. The tests are a nice bonus. A few things worth addressing:
1. Race condition: localStorage mirror vs. message timing
syncAutoplayDisableLocalStorage() is called both during init.js boot (before page-world injections) and inside the storage-loaded message handler in core.js. But syncAutoplayDisableLocalStorage in core.js reads from ImprovedTube.storage which gets hydrated via message. If the message arrives after YouTube calls play() but before the guard is installed (page-world scripts inject async), the first play could slip through. Consider writing the setting to localStorage before page-world injection in init.js to guarantee the guard is ready on arrival.
2. Edge case: /watch? param detection
shouldPreventInitialAutoplay checks location.href.includes('/watch?'), but YouTube can use /watch?has_verified=1&v=abc123 or other param orderings. That's fine for this check, but consider whether list= exclusion is always correct — if a user manually navigates to a watch page with a playlist but has autoplay disabled, they probably still don't want autoplay.
3. Ad-showing edge case is worth a test
You return false when an ad is showing (!player.classList.contains('ad-showing')). This is correct (ads should play), but the test suite doesn't cover this path. Adding a test with an ad-playing video element would validate the ad-showing guard.
4. play() wrapper — fragile for subclass/reset scenarios
The wrapper stores improvedTubeInitialAutoplayGuard on the replacement function, but if another script resets HTMLMediaElement.prototype.play after yours (unlikely but possible), the guard is lost silently. Consider adding a weak reference or check at the call site. Not a blocker, just a defense-in-depth suggestion.
5. HTMLMediaElement.prototype.play returns Promise correctly
The wrapper returns Promise.resolve() when blocking. This is correct since HTMLMediaElement.play() is spec'd to return a Promise.
Overall: LGTM with the above considerations. The change is clean, well-structured, and really solves a UX annoyance. Thanks for including tests!
Summary
Fixes #1461
Testing