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

setSrc clears currentSource_ after loadstart #3285

Closed
wants to merge 8 commits into from
Closed

setSrc clears currentSource_ after loadstart #3285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
06dcc61
setSrc clears currentSource_ after loadstart
Apr 26, 2016
8d89829
Move disposeSourceHandler from setSrc to loadStartListener_
Apr 26, 2016
132e107
Improvements based on code review
Apr 27, 2016
47873df
Remove unused import
Apr 27, 2016
f10fc92
Move currentSource_ clearing behavior from html5 tech to tech.js
Apr 28, 2016
84810c2
Restore coveralls
Apr 28, 2016
64be2ab
Improve method names and comments in setSource
Apr 29, 2016
43aac04
Clearing currentSource_ happens on successive loadstarts
Apr 29, 2016
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
7 changes: 6 additions & 1 deletion src/js/event-target.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ EventTarget.prototype.on = function(type, fn) {
// Remove the addEventListener alias before calling Events.on
// so we don't get into an infinite type loop
let ael = this.addEventListener;
this.addEventListener = Function.prototype;
this.addEventListener = () => {};
Copy link
Member

Choose a reason for hiding this comment

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

Point for discussion: should we be careful about using fat-arrows all over the place? I know calling addEventListener() with a non-this receiver is a weird thing to do but we could use a plain function here and avoid possibly subverting people's expectations.

Copy link
Member

Choose a reason for hiding this comment

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

We're gonna change this to function() {}

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it matters here at all since we just want an empty function while Events.on runs so we don't get into an infinite loop, so, there's no expectations being subverted here.
As for elsewhere, it really depends on what you want the context of your function to be since in arrow functions this and arguments are never bound.

Copy link
Member

Choose a reason for hiding this comment

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

That's the crux of the discussion I think we should have. Using fat-arrows removes the ability of the caller to rebind this. I've felt it necessary to rebind this more than once in my life. Are we intentionally removing that ability or just doing it to save some keystrokes?

Events.on(this, type, fn);
this.addEventListener = ael;
};
Expand All @@ -23,7 +23,12 @@ EventTarget.prototype.off = function(type, fn) {
EventTarget.prototype.removeEventListener = EventTarget.prototype.off;

EventTarget.prototype.one = function(type, fn) {
// Remove the addEventListener alias before calling Events.on
// so we don't get into an infinite type loop
let ael = this.addEventListener;
this.addEventListener = () => {};
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if one of the listeners registers an event listener? Something like this:

function maybeReRegister() {
  if (someCondition()) {
    player.one('play', maybeReRegister);
    return;
  }
  alert('done!');
};
player.one('play', maybeReRegister);

Copy link
Member

Choose a reason for hiding this comment

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

After reviewing, I don't think this is a problem right now but the event stuff sure is confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I think it works fine because the event system creates a clone of the handlers before it starts triggering events:

video.js/src/js/utils/events.js

Lines 52 to 63 in 2e2dbde

if (handlers) {
// Copy handlers so if handlers are added/removed during the process it doesn't throw everything off.
var handlersCopy = handlers.slice(0);
for (var m = 0, n = handlersCopy.length; m < n; m++) {
if (event.isImmediatePropagationStopped()) {
break;
} else {
handlersCopy[m].call(elem, event, hash);
}
}
}

Events.one(this, type, fn);
this.addEventListener = ael;
};

EventTarget.prototype.trigger = function(event) {
Expand Down
1 change: 0 additions & 1 deletion src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,6 @@ Html5.prototype['featuresNativeVideoTracks'] = Html5.supportsNativeVideoTracks()
*/
Html5.prototype['featuresNativeAudioTracks'] = Html5.supportsNativeAudioTracks();


// HTML5 Feature detection and Device Fixes --------------------------------- //
let canPlayType;
const mpegurlRE = /^application\/(?:x-|vnd\.apple\.)mpegurl/i;
Expand Down
36 changes: 31 additions & 5 deletions src/js/tech/tech.js
View file
Open in desktop
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
45 changes: 44 additions & 1 deletion test/unit/tech/tech.test.js
View file
Open in desktop
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');

});