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

Don't test against http://example.com/english.vtt #2720

Closed
wants to merge 3 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 20, 2015

This is causing failures in IE8.
Also, don't include 'src' in json output if it doesn't exist.

This is causing failures in IE8.
Also, don't include 'src' in json output if it doesn't exist.
@pam
Copy link

pam commented Oct 20, 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: 471702e
Build details: https://travis-ci.org/pam/video.js/builds/86441440

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

@pam
Copy link

pam commented Oct 20, 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: 1250e75
Build details: https://travis-ci.org/pam/video.js/builds/86443389

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

};
if (track.src) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of specifically handling src here, would it make sense to generally exclude any falsy property?

Object.keys(ret).forEach(k => {
  if (!ret[k]) {
    delete ret[k];
  }
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was debating that. We do know exactly what keys we want, so, we could just loop through those keys and copy them over only if they have values. We might need to do a null check on the other side.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, add-if-present might be more efficient (in terms of reading the code if not performance).

let ret = {};
['kind', 'label', 'language', 'id', 'inBand...', 'mode', 'src'].forEach(k => {
  if (track[k]) {
    ret[k] = track[k];
  }
});
if (track.cues) {
  // special map handling for cues
}
return ret;

@gkatsev gkatsev mentioned this pull request Oct 26, 2015
18 tasks
@gkatsev
Copy link
Member Author

gkatsev commented Oct 26, 2015

Closing in favor of this rebased version: #2744

@gkatsev gkatsev closed this Oct 26, 2015
@gkatsev gkatsev deleted the fix-track-tests branch October 26, 2015 23:09
@pam
Copy link

pam commented Oct 26, 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: 183d56c
Build details: https://travis-ci.org/pam/video.js/builds/87564132

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

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