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.

Sign up for GitHub

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

Jump to bottom

Loop through sources then techs #2844

Closed
wants to merge 2 commits into from
Closed

Loop through sources then techs #2844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7010016
Loop through sources then techs
Nov 23, 2015
c74e116
Add test, add options for backward compatibility
Nov 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 20 additions & 30 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1700,36 +1700,26 @@ class Player extends Component {
* @method selectSource
*/
selectSource(sources) {
// Loop through each playback technology in the options order
for (var i=0,j=this.options_.techOrder;i<j.length;i++) {
let techName = toTitleCase(j[i]);
let tech = Tech.getTech(techName);
// Support old behavior of techs being registered as components.
// Remove once that deprecated behavior is removed.
if (!tech) {
tech = Component.getComponent(techName);
}
// Check if the current tech is defined before continuing
if (!tech) {
log.error(`The "${techName}" tech is undefined. Skipped browser support check for that tech.`);
continue;
}

// Check if the browser supports this technology
if (tech.isSupported()) {
// Loop through each source object
for (var a=0,b=sources;a<b.length;a++) {
var source = b[a];

// Check if source can be played with this technology
if (tech.canPlaySource(source)) {
return { source: source, tech: techName };
}
}
}
}

return false;
// Get only the techs that exist and are supported by the current platform
let techs =
this.options_.techOrder
.map(toTitleCase)
.map(techName => Tech.getTech(techName) || Component.getComponent(techName) ||
Copy link
Member

Choose a reason for hiding this comment

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

While this was clever, I decided to make it more explicit since I wanted to add the comments from the old function back.

log.error(`The "${techName}" tech is undefined. Skipped browser support check for that tech.`))
.filter(tech => tech && tech.isSupported());

// Depending on the truthiness of options.sourceOrder, we swap the order of techs and sources
// to select from them based on their priority.
let combinedSources = this.options_.sourceOrder ?
Copy link
Member

Choose a reason for hiding this comment

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

This is very cool. I wonder if we can somehow combine the two very similar reduces here..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can take my changes/some ideas from this PR since you've done most of the work.

sources.reduce((acc, source) => acc.concat(techs.map(tech => ({source, tech}))), []) :
techs.reduce((acc, tech) => acc.concat(sources.map(source => ({source, tech}))), []);

let matchedSource = combinedSources
.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.

false;
}

/**
Expand Down
39 changes: 39 additions & 0 deletions test/unit/player.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,45 @@ test('make sure that controls listeners do not get added too many times', functi
// player.dispose();
// });

test('should select the proper tech based on the the sourceOrder option',
function() {
var fixture = document.getElementById('qunit-fixture');
var html =
'<video id="example_1">' +
'<source src="fake.foo1" type="video/unsupported-format">' +
'<source src="fake.foo2" type="video/foo-format">' +
'</video>';

// Extend TechFaker to create a tech that plays the only mime-type that TechFaker
// will not play
import Tech from '../../src/js/tech/tech.js';
import TechFaker from './tech/tech-faker.js';
class PlaysUnsupported extends TechFaker {
constructor(options, handleReady){
super(options, handleReady);
}
// Support ONLY "video/unsupported-format"
static isSupported() { return true; }
static canPlayType(type) { return (type === 'video/unsupported-format' ? 'maybe' : ''); }
static canPlaySource(srcObj) { return srcObj.type === 'video/unsupported-format'; }
}
Tech.registerTech('PlaysUnsupported', PlaysUnsupported);

fixture.innerHTML += html;
var tag = document.getElementById('example_1');

var player = new Player(tag, { techOrder: ['techFaker', 'playsUnsupported'], sourceOrder: true });
equal(player.techName_, 'PlaysUnsupported', 'selected the PlaysUnsupported tech when sourceOrder is truthy');
player.dispose();

fixture.innerHTML += html;
tag = document.getElementById('example_1');

player = new Player(tag, { techOrder: ['techFaker', 'playsUnsupported']});
equal(player.techName_, 'TechFaker', 'selected the TechFaker tech when sourceOrder is falsey');
player.dispose();
});

test('should register players with generated ids', function(){
var fixture, video, player, id;
fixture = document.getElementById('qunit-fixture');
Expand Down