-
Notifications
You must be signed in to change notification settings - Fork 425
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
refactor: rename 'blacklist' to 'exclude' #1274
Conversation
Codecov Report
@@ Coverage Diff @@
## next #1274 +/- ##
==========================================
- Coverage 86.24% 86.22% -0.02%
==========================================
Files 39 39
Lines 9769 9757 -12
Branches 2281 2276 -5
==========================================
- Hits 8425 8413 -12
Misses 1344 1344
Continue to review full report at Codecov.
|
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.
While we're modifying the public pieces, it may be worth renaming all instance of blacklist
throughout the repo as well as the properties (e.g., ABORT_EARLY_BLACKLIST_SECONDS
and blacklistReasons
in MasterPlaylistController
, various comments talking about blacklisting, and test messages).
We also should start a migration guide for updating to the next major version.
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.
I think there are only a few more references.
http-streaming/docs/a-walk-through-vhs.md
Line 250 in 8ca62cc
There are other modules, and other functions of the code (e.g., blacklisting logic, ABR, etc.), but this is the most critical path of VHS, the one that allows video to play in the browser. |
http-streaming/src/media-segment-request.js
Line 428 in 8ca62cc
// because we can only blacklist a playlist and abort requests |
and then we trigger blacklistplaylist
in src/master-playlist-controller.js:
http-streaming/src/master-playlist-controller.js
Line 1233 in 8ca62cc
this.tech_.trigger('blacklistplaylist'); |
(and test for it in test/videojs-http-streaming.text.js):
this.player.tech_.on('blacklistplaylist', () => excludedplaylist++); |
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.
This is great! Thank you for doing the extra rename!
src/master-playlist-controller.js
Outdated
@@ -1143,20 +1143,20 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
* @param {number=} playlistExclusionDuration an optional number of seconds to exclude the | |||
* playlist | |||
*/ | |||
excludeCurrentPlaylist(error = {}, playlistExclusionDuration) { |
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.
It may be a bit more work, but it might be worth us having to functions: excludeCurrentPlaylist
and excludePlaylist
excludeCurrentPlaylist
can call excludePlaylist
with an explicit playlist (this.masterPlaylistLoader_.media()
), and excludePlaylist
accepts a playlistToExclude
param.
This would let the code read clearer for callers of the function.
Co-authored-by: Garrett Singer <[email protected]>
No description provided.