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

setSrc clears currentSource_ after loadstart #3285

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
10 changes: 0 additions & 10 deletions src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -549,19 +549,9 @@ class Html5 extends Tech {
* @method setSrc
*/
setSrc(src) {
let loadstartlistener = Html5.prototype.loadStartListener_;

this.off(this.el_, 'loadstart', loadstartlistener);
this.one(this.el_, 'loadstart', () => this.one(this.el_, 'loadstart', loadstartlistener));

this.el_.src = src;
}

loadStartListener_() {
this.currentSource_ = null;
this.disposeSourceHandler();
}

/**
* Load media into player
*
Expand Down
36 changes: 31 additions & 5 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -814,18 +814,44 @@ Tech.withSourceHandlers = function(_Tech){
if (this.currentSource_) {
this.clearTracks(['audio', 'video']);
}
this.currentSource_ = source;

if (sh !== _Tech.nativeSourceHandler) {

this.currentSource_ = source;

// Catch if someone replaced the src without calling setSource.
// If they do, set currentSource_ to null and dispose our source handler.
this.off(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
this.off(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
this.one(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of using event handler registration as instance-level state. The current set of registered event handlers are difficult to introspect and control flow is harder to trace. I don't see any logical issues with this code so we can move ahead with this but in general, I think adding an instance variable is preferable to this kind of solution.


}

this.sourceHandler_ = sh.handleSource(source, this, this.options_);
this.on('dispose', this.disposeSourceHandler);

return this;
};

/*
* Clean up any existing source handler
*/
_Tech.prototype.disposeSourceHandler = function(){
// On the first loadstart after setSource
_Tech.prototype.firstLoadStartListener_ = function() {
this.one(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
Copy link
Member

Choose a reason for hiding this comment

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

Why use one() if you want the successiveLoadStartListener_ to run on every subsequent loadstart?

Copy link
Author

Choose a reason for hiding this comment

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

It's because disposeSourceHandler removes it. We work around this by re-adding it in successiveLoadStartListener_ after disposeSourceHandler has been called.

};

// On successive loadstarts when setSource has not been called again
_Tech.prototype.successiveLoadStartListener_ = function() {
this.currentSource_ = null;
this.disposeSourceHandler();
this.one(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
};

/*
* Clean up any existing source handler
*/
_Tech.prototype.disposeSourceHandler = function() {
if (this.sourceHandler_ && this.sourceHandler_.dispose) {
this.off(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
this.off(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
this.sourceHandler_.dispose();
}
};
Expand Down
29 changes: 0 additions & 29 deletions test/unit/tech/html5.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ var player, tech, el;
import Html5 from '../../../src/js/tech/html5.js';
import * as browser from '../../../src/js/utils/browser.js';
import document from 'global/document';
import EventTarget from '../../../src/js/event-target.js';
import * as Fn from '../../../src/js/utils/fn.js';

q.module('HTML5', {
'setup': function() {
Expand Down Expand Up @@ -444,30 +442,3 @@ test('Html5#reset calls Html5.resetMediaElement when called', function() {

Html5.resetMediaElement = oldResetMedia;
});

test('Html5#setSrc clears currentSource_ after loadstart', function() {

let disposed = false;
let thing = {
off: () => {},
one: (el, type, fun) => {
el.one(type, Fn.bind(thing, fun));
},
disposeSourceHandler: () => disposed = true,
el_: new EventTarget()
};

Html5.prototype.setSrc.call(thing, 'test');

thing.currentSource_ = 'test';

thing.el_.trigger('loadstart');

equal(thing.currentSource_, 'test');

thing.el_.trigger('loadstart');

equal(thing.currentSource_, null);
equal(disposed, true);

});
45 changes: 44 additions & 1 deletion test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import AudioTrackList from '../../../src/js/tracks/audio-track-list';
import VideoTrackList from '../../../src/js/tracks/video-track-list';
import TextTrackList from '../../../src/js/tracks/text-track-list';


q.module('Media Tech', {
'setup': function() {
this.noop = function() {};
Expand Down Expand Up @@ -370,3 +369,47 @@ test('Tech.isTech returns correct answers for techs and components', function()
ok(!isTech(new Button({}, {})), 'A Button instance is not a Tech');
ok(!isTech(isTech), 'A function is not a Tech');
});

test('Tech#setSource clears currentSource_ after repeated loadstart', function() {
let disposed = false;
let MyTech = extendFn(Tech);

Tech.withSourceHandlers(MyTech);
let tech = new MyTech();

var sourceHandler = {
canPlayType: function(type) {
return true;
},
canHandleSource: function(source) {
return true;
},
handleSource: function(source, tech, options) {
return {
dispose: function() {
disposed = true;
}
};
}
};

// Test registering source handlers
MyTech.registerSourceHandler(sourceHandler);

// First loadstart
tech.setSource('test');
tech.currentSource_ = 'test';
tech.trigger('loadstart');
equal(tech.currentSource_, 'test', 'Current source is test');

// Second loadstart
tech.trigger('loadstart');
equal(tech.currentSource_, null, 'Current source is null');
equal(disposed, true, 'disposed is true');

// Third loadstart
tech.currentSource_ = 'test';
tech.trigger('loadstart');
equal(tech.currentSource_, null, 'Current source is still null');

});