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

Update MW to require a factory, add *-mw #3969

Merged
merged 7 commits into from
Jan 27, 2017
Merged

Update MW to require a factory, add *-mw #3969

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e7f687f
update use to accept a middleware factory that accepts a player. Also…
gkatsev Jan 23, 2017
f4f07f2
update tests to use the new mw factories
gkatsev Jan 23, 2017
b1b132d
fix linting
gkatsev Jan 23, 2017
4bbcbe5
allow buffered, seekable, and played to be intercepted
gkatsev Jan 25, 2017
2a5cdec
no need to pass setTimeout directly if we're already passing in player
gkatsev Jan 27, 2017
fe34b23
fix setSource test with player/setTimeout change
gkatsev Jan 27, 2017
8cec6ae
trigger travis maybe?
gkatsev Jan 27, 2017
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
2 changes: 1 addition & 1 deletion src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2208,7 +2208,7 @@ class Player extends Component {

this.currentType_ = src.type;

middleware.setSource(Fn.bind(this, this.setTimeout), src, (src_, mws) => {
middleware.setSource(this, Fn.bind(this, this.setTimeout), src, (src_, mws) => {
this.middleware_ = mws;

const err = this.src_(src_);
Expand Down
38 changes: 24 additions & 14 deletions src/js/tech/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export function getMiddleware(type) {
return middlewares;
}

export function setSource(setTimeout, src, next) {
setTimeout(() => setSourceHelper(src, middlewares[src.type], next), 1);
export function setSource(player, setTimeout, src, next) {
setTimeout(() => setSourceHelper(src, middlewares[src.type], next, player), 1);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be using window.setTimeout?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, I just realized I don't need to pass in setTimeout here. But it's the setTimeout from player, so, that if player is disposed`, so will this timeout.

}

export function setTech(middleware, tech) {
Expand All @@ -32,8 +32,11 @@ export function set(middleware, tech, method, arg) {
}

export const allowedGetters = {
buffered: 1,
currentTime: 1,
duration: 1
duration: 1,
seekable: 1,
played: 1
};

export const allowedSetters = {
Expand All @@ -50,21 +53,24 @@ function middlewareIterator(method) {
};
}

function setSourceHelper(src = {}, middleware = [], next, acc = []) {
const [mw, ...mwrest] = middleware;
function setSourceHelper(src = {}, middleware = [], next, player, acc = [], lastRun = false) {
const [mwFactory, ...mwrest] = middleware;

// if mw is a string, then we're at a fork in the road
if (typeof mw === 'string') {
setSourceHelper(src, middlewares[mw], next, acc);
// if mwFactory is a string, then we're at a fork in the road
if (typeof mwFactory === 'string') {
setSourceHelper(src, middlewares[mwFactory], next, player, acc, lastRun);

// if we have an mwFactory, call it with the player to get the mw,
// then call the mw's setSource method
} else if (mwFactory) {
const mw = mwFactory(player);

// if we have an mw, call its setSource method
} else if (mw) {
mw.setSource(assign({}, src), function(err, _src) {

// something happened, try the next middleware on the current level
// make sure to use the old src
if (err) {
return setSourceHelper(src, mwrest, next, acc);
return setSourceHelper(src, mwrest, next, player, acc, lastRun);
}

// we've succeeded, now we need to go deeper
Expand All @@ -75,11 +81,15 @@ function setSourceHelper(src = {}, middleware = [], next, acc = []) {
setSourceHelper(_src,
src.type === _src.type ? mwrest : middlewares[_src.type],
next,
acc);
player,
acc,
lastRun);
});
} else if (mwrest.length) {
setSourceHelper(src, mwrest, next, acc);
} else {
setSourceHelper(src, mwrest, next, player, acc, lastRun);
} else if (lastRun) {
next(src, acc);
} else {
setSourceHelper(src, middlewares['*'], next, player, acc, true);
}
}
3 changes: 0 additions & 3 deletions src/js/tech/tech.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { bufferedPercent } from '../utils/buffer.js';
import MediaError from '../media-error.js';
import window from 'global/window';
import document from 'global/document';
import * as middleware from './middleware.js';
import {isPlain} from '../utils/obj';
import * as TRACK_TYPES from '../tracks/track-types';

Expand Down Expand Up @@ -799,8 +798,6 @@ class Tech extends Component {
throw new Error('Techs must have a static canPlaySource method on them');
}

middleware.use('*', {name, tech});

Tech.techs_[name] = tech;
return tech;
}
Expand Down
20 changes: 10 additions & 10 deletions test/unit/player.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,7 @@ QUnit.test('techGet runs through middleware if allowedGetter', function(assert)
let durs = 0;
let ps = 0;

videojs.use('video/foo', {
videojs.use('video/foo', () => ({
currentTime() {
cts++;
},
Expand All @@ -1421,14 +1421,14 @@ QUnit.test('techGet runs through middleware if allowedGetter', function(assert)
paused() {
ps++;
}
});
}));

const tag = TestHelpers.makeTag();
const player = videojs(tag, {
techOrder: ['techFaker']
});

player.middleware_ = middleware.getMiddleware('video/foo');
player.middleware_ = [middleware.getMiddleware('video/foo')[0](player)];

player.techGet_('currentTime');
player.techGet_('duration');
Expand All @@ -1446,22 +1446,22 @@ QUnit.test('techCall runs through middleware if allowedSetter', function(assert)
let cts = 0;
let vols = 0;

videojs.use('video/foo', {
videojs.use('video/foo', () => ({
setCurrentTime(ct) {
cts++;
return ct;
},
setVolume() {
vols++;
}
});
}));

const tag = TestHelpers.makeTag();
const player = videojs(tag, {
techOrder: ['techFaker']
});

player.middleware_ = middleware.getMiddleware('video/foo');
player.middleware_ = [middleware.getMiddleware('video/foo')[0](player)];

this.clock.tick(1);

Expand Down Expand Up @@ -1492,23 +1492,23 @@ QUnit.test('src selects tech based on middleware', function(assert) {
videojs.registerTech('FooTech', FooTech);
videojs.registerTech('BarTech', BarTech);

videojs.use('video/foo', {
videojs.use('video/foo', () => ({
setSource(src, next) {
next(null, {
src: 'http://example.com/video.mp4',
type: 'video/mp4'
});
}
});
}));

videojs.use('video/bar', {
videojs.use('video/bar', () => ({
setSource(src, next) {
next(null, {
src: 'http://example.com/video.flv',
type: 'video/flv'
});
}
});
}));

const tag = TestHelpers.makeTag();
const player = videojs(tag, {
Expand Down
31 changes: 19 additions & 12 deletions test/unit/tech/middleware.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ QUnit.module('Middleware', {

QUnit.test('middleware can be added with the use method', function(assert) {
const myMw = {};
const mwFactory = () => myMw;

middleware.use('foo', myMw);
middleware.use('foo', mwFactory);

assert.equal(middleware.getMiddleware('foo').pop(), myMw, 'we are able to add middleware');
assert.equal(middleware.getMiddleware('foo').pop(), mwFactory, 'we are able to add middleware');
});

QUnit.test('middleware get iterates through the middleware array the right order', function(assert) {
Expand Down Expand Up @@ -147,7 +148,7 @@ QUnit.test('setSource is run asynchronously', function(assert) {
let src;
let acc;

middleware.setSource(window.setTimeout, { src: 'foo', type: 'video/foo' }, function(_src, _acc) {
middleware.setSource({}, window.setTimeout, { src: 'foo', type: 'video/foo' }, function(_src, _acc) {
src = _src;
acc = _acc;
});
Expand All @@ -172,10 +173,11 @@ QUnit.test('setSource selects a source based on the middleware given', function(
});
}
};
const fooFactory = () => mw;

middleware.use('video/foo', mw);
middleware.use('video/foo', fooFactory);

middleware.setSource(window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
middleware.setSource({}, window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
src = _src;
acc = _acc;
});
Expand Down Expand Up @@ -209,11 +211,13 @@ QUnit.test('setSource can select multiple middleware from multiple types', funct
});
}
};
const fooFactory = () => foomw;
const barFactory = () => barmw;

middleware.use('video/foo', foomw);
middleware.use('video/bar', barmw);
middleware.use('video/foo', fooFactory);
middleware.use('video/bar', barFactory);

middleware.setSource(window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
middleware.setSource({}, window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
src = _src;
acc = _acc;
});
Expand Down Expand Up @@ -257,12 +261,15 @@ QUnit.test('setSource will select all middleware of a given type, until src chan
});
}
};
const fooFactory1 = () => foomw1;
const fooFactory2 = () => foomw2;
const fooFactory3 = () => foomw3;

middleware.use('video/foo', foomw1);
middleware.use('video/foo', foomw2);
middleware.use('video/foo', foomw3);
middleware.use('video/foo', fooFactory1);
middleware.use('video/foo', fooFactory2);
middleware.use('video/foo', fooFactory3);

middleware.setSource(window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
middleware.setSource({}, window.setTimeout, {src: 'foo', type: 'video/foo'}, function(_src, _acc) {
src = _src;
acc = _acc;
});
Expand Down