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

Seekable from source handlers #2376

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions src/js/tech/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,16 @@ class Flash extends Tech {
* @method setCurrentTime
*/
setCurrentTime(time) {
this.lastSeekTarget_ = time;
this.el_.vjs_setProperty('currentTime', time);
super.setCurrentTime();
let seekable = this.seekable();
if (seekable.length) {
// clamp to the current seekable range
time = time > seekable.start(0) ? time : seekable.start(0);
time = time < seekable.end(seekable.length - 1) ? time : seekable.end(seekable.length - 1);

this.lastSeekTarget_ = time;
this.el_.vjs_setProperty('currentTime', time);
super.setCurrentTime();
}
}

/**
Expand Down
11 changes: 11 additions & 0 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,16 @@ Tech.withSourceHandlers = function(_Tech){
this.sourceHandler_ = sh.handleSource(source, this);
this.on('dispose', this.disposeSourceHandler);

this.originalSeekable_ = this.seekable;
// when a source handler is registered, prefer its implementation of
// seekable when present.
this.seekable = function() {
if (this.sourceHandler_ && this.sourceHandler_.seekable) {
return this.sourceHandler_.seekable();
}
return this.originalSeekable_.call(this);
};

return this;
};

Expand All @@ -574,6 +584,7 @@ Tech.withSourceHandlers = function(_Tech){
_Tech.prototype.disposeSourceHandler = function(){
if (this.sourceHandler_ && this.sourceHandler_.dispose) {
this.sourceHandler_.dispose();
this.seekable = this.originalSeekable_;
}
};

Expand Down
26 changes: 23 additions & 3 deletions test/unit/tech/flash.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Flash from '../../../src/js/tech/flash.js';
import { createTimeRange } from '../../../src/js/utils/time-ranges.js';
import document from 'global/document';

q.module('Flash');
Expand Down Expand Up @@ -30,14 +31,17 @@ test('currentTime', function() {
// Mock out a Flash instance to avoid creating the swf object
let mockFlash = {
el_: {
vjs_setProperty: function(prop, val){
vjs_setProperty(prop, val){
Copy link
Member

Choose a reason for hiding this comment

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

Wait what? We can use this outside of classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

:)

Copy link
Member

Choose a reason for hiding this comment

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

These are shorthand for functions. They're different from the methods in classes. Notice how there's a comma between them but not in classes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see.

setPropVal = val;
},
vjs_getProperty: function(){
vjs_getProperty(){
return getPropVal;
}
},
seeking: function(){
seekable(){
return createTimeRange(5, 1000);
},
seeking(){
return seeking;
}
};
Expand All @@ -57,6 +61,15 @@ test('currentTime', function() {
result = getCurrentTime.call(mockFlash);
equal(result, 20, 'currentTime is retrieved from the lastSeekTarget while seeking');
notEqual(result, getPropVal, 'currentTime is not retrieved from the element while seeking');

// clamp seeks to seekable
setCurrentTime.call(mockFlash, 1001);
result = getCurrentTime.call(mockFlash);
equal(result, mockFlash.seekable().end(0), 'clamped to the seekable end');

setCurrentTime.call(mockFlash, 1);
result = getCurrentTime.call(mockFlash);
equal(result, mockFlash.seekable().start(0), 'clamped to the seekable start');
});

test('dispose removes the object element even before ready fires', function() {
Expand Down Expand Up @@ -141,3 +154,10 @@ test('seekable', function() {
result = seekable.call(mockFlash);
equal(result.length, mockFlash.duration_, 'seekable is empty with a zero duration');
});

// fake out the <object> interaction but leave all the other logic intact
Copy link
Member

Choose a reason for hiding this comment

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

What's your thoughts on this type of mock vs the mockFlash object above? Is see that setCurrentTime requires a super call, but interested otherwise.

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 was an experiment. It implicitly pulls in more logic from the Flash tech than the mockFlash mechanism used elsewhere which could be a pro or a con. I can try to convert the rest of the tests over and see if it results in less setup in the individual tests if you think it's worth considering.

Copy link
Member

Choose a reason for hiding this comment

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

So I've actually been moving in the opposite direction, trying to require less code that's unrelated to the test to execute. With the Player and Tech mocks like this one, errors in those classes cause half the tests in the suite to fail. The other way requires more setup but it's also pretty explicit about what the function expects. Though if the class you're mocking changes, I guess you could miss issues around that. So not sure which is better or if there's a middle ground.

class MockFlash extends Flash {
constructor() {
super({});
}
}
40 changes: 37 additions & 3 deletions test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var noop = function() {}, clock, oldTextTracks;

import Tech from '../../../src/js/tech/tech.js';
import { createTimeRange } from '../../../src/js/utils/time-ranges.js';

q.module('Media Tech', {
'setup': function() {
Expand Down Expand Up @@ -97,7 +98,7 @@ test('dispose() should stop time tracking', function() {
ok(true, 'no exception was thrown');
});

test('should add the source hanlder interface to a tech', function(){
test('should add the source handler interface to a tech', function(){
var sourceA = { src: 'foo.mp4', type: 'video/mp4' };
var sourceB = { src: 'no-support', type: 'no-support' };

Expand Down Expand Up @@ -176,7 +177,7 @@ test('should add the source hanlder interface to a tech', function(){
ok(disposeCalled, 'the handler dispose method was called when the tech was disposed');
});

test('should handle unsupported sources with the source hanlder API', function(){
test('should handle unsupported sources with the source handler API', function(){
// Define a new tech class
var MyTech = Tech.extend();
// Extend Tech with source handlers
Expand All @@ -197,7 +198,40 @@ test('should track whether a video has played', function() {
let tech = new Tech();

equal(tech.played().length, 0, 'starts with zero length');

tech.trigger('playing');
equal(tech.played().length, 1, 'has length after playing');
});

test('delegates seekable to the source handler', function(){
let MyTech = Tech.extend({
seekable: function() {
throw new Error('You should not be calling me!');
}
});
Tech.withSourceHandlers(MyTech);

let seekableCount = 0;
let handler = {
seekable: function() {
seekableCount++;
return createTimeRange(0, 0);
}
};

MyTech.registerSourceHandler({
canHandleSource: function() {
return true;
},
handleSource: function(source, tech) {
return handler;
}
});

let tech = new MyTech();
tech.setSource({
src: 'example.mp4',
type: 'video/mp4'
});
tech.seekable();
equal(seekableCount, 1, 'called the source handler');
});