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

fix(html5): exit early on emulated tracks in html5 #3772

Merged
merged 2 commits into from
Nov 11, 2016

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 10, 2016

addRemoteTextTrack and removeRemoteTextTrack in Html5 tech assumed that
they are always called for native text tracks. However, Html5 tech can
have emulated text tracks. For those, we just exit early after the super
methods are called.

addRemoteTextTrack and removeRemoteTextTrack in Html5 tech assumed that
they are always called for native text tracks. However, Html5 tech can
have emulated text tracks. For those, we just exit early after the super
methods are called.
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified as working in the real world.

@boushley
Copy link
Contributor

This works and so is great!

I think I would actually prefer using the non-negated if here for both cases. It cleans up the add track case significantly, and the remove track case isn't so big that the extra indentation causes lots of problems. The reason for this is that it makes the return path consistent. For remove this isn't a big deal, but for add it's nice to have a single return since it makes it easier to verify that both cases return the same data.

That said, this is a pretty minor thing for such small methods, so I'm fine as is.

@boushley
Copy link
Contributor

I like it!

And if you turn on ignore whitespace it looks nice and clean: https://github.com/videojs/video.js/pull/3772/files?w=1

@gkatsev gkatsev merged commit 252bcee into videojs:master Nov 11, 2016
@gkatsev gkatsev deleted the text-tracks branch November 11, 2016 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants