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

Fixup tracks some more #2494

Closed
wants to merge 3 commits into from
Closed

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Aug 19, 2015

No description provided.

@pam
Copy link

pam commented Aug 19, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 91d71c0
Build details: https://travis-ci.org/pam/video.js/builds/76361049

(Please note that this is a fully automated comment.)

@@ -127,7 +127,7 @@ class Html5 extends Tech {

// If the original tag is still there, clone and remove it.
if (el) {
const clone = el.cloneNode(false);
const clone = el.cloneNode(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why is a deep clone necessary here (and not before)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, we won't get tracks available on something like ios.
Previously, we'd grab all the tracks from the element and store them on the options object (which still happens) but then we need to pass it to the tech. However, if we pass through both that option and the textTracks object, if you swap between techs, you'll start getting duplicated tracks. So, instead, I can remove the need to pass through the tracks option to the tech and just clone the node with its sub nodes, since the tracks option should only have been used the first time the player was initialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully, that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that makes sense.

@mmcc
Copy link
Member

mmcc commented Aug 19, 2015

lgtm

@@ -20,6 +20,7 @@ class TextTrackButton extends MenuButton {
constructor(player, options){
super(player, options);

this.update();
Copy link
Member Author

Choose a reason for hiding this comment

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

this forces the buttons to update in case the video element has tracks from before we are able to register the handlers to update the buttons.

@heff
Copy link
Member

heff commented Aug 19, 2015

Got some travis errors

@pam
Copy link

pam commented Aug 19, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: e585c5c
Build details: https://travis-ci.org/pam/video.js/builds/76376526

(Please note that this is a fully automated comment.)

@heff
Copy link
Member

heff commented Aug 19, 2015

lgtm

@gkatsev gkatsev closed this in 904bf0c Aug 19, 2015
@gkatsev gkatsev deleted the fixup-tracks-some-more branch August 19, 2015 23:36
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.

4 participants