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

Loop through sources then techs #2844

Closed
wants to merge 2 commits into from

Conversation

forbesjo
Copy link
Contributor

This PR is to make it so that selectSources iterates through the sources then techOrder. This allows browsers without MSE to play HLS videos through Flash instead of falling back to a mp4 source.

@pam
Copy link

pam commented Nov 23, 2015

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

Commit: 7010016
Build details: https://travis-ci.org/pam/video.js/builds/92761449

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

@dmlap
Copy link
Member

dmlap commented Nov 23, 2015

We'd need a flag to enable this for backwards-compatibility.

@forbesjo
Copy link
Contributor Author

Took a test from @imbcmdth and part of his change.

@pam
Copy link

pam commented Nov 23, 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: c74e116
Build details: https://travis-ci.org/pam/video.js/builds/92797706

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

.find(({source, tech}) => tech.canPlaySource(source));

return matchedSource ?
{ source: matchedSource.source, tech: matchedSource.tech.name } :
Copy link
Member

Choose a reason for hiding this comment

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

I decided against using the .name property on the constructor because there is no guarantee that the constructor will be named or have the same name as the "tech" mapping.

For instance:

Tech.registerTech('TotallyNotFoo', Foo);

Would have a name of Foo but a "techName" of totallyNotFoo.

@forbesjo
Copy link
Contributor Author

Babel doesn't include .find 😢

@gkatsev
Copy link
Member

gkatsev commented Nov 23, 2015

Babel doesn't shim anything. We're still dealing with an es5 baseline and not an es3 baseline anymore, at least.
Also, we have both this and #2847. We should consolidate on one of them.

@forbesjo
Copy link
Contributor Author

I'll close this one

@forbesjo forbesjo closed this Nov 23, 2015
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.

5 participants