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

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 4 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
11 changes: 10 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 @@ -549,9 +549,19 @@ class Html5 extends Tech {
* @method setSrc
*/
setSrc(src) {
let loadstartlistener = Html5.prototype.loadStartListener_;
Copy link
Member

Choose a reason for hiding this comment

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

Are we explicitly using the Html5#loadStartListener_ function instead of this.loadStartListener_ because we want to be sure that property hasn't been added to an instance of the tech as something else? Maybe this is a candidate for being module-level private instead of class-level pseudo-private?

Copy link
Author

Choose a reason for hiding this comment

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

No, it's just arbitrary.


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

Choose a reason for hiding this comment

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

Does this assume setSrc() is the only valid way to modify the video element's source? What about setSource()?

Is there a way this can be written that doesn't require adding and removing listeners after the tech's creation? You might be able to use the readyState of the HTML element, for instance.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this function literal wouldn't get cleared properly on subsequent calls to setSrc() which would cause the loadstartlistener_ to get invoked on the wrong loadstart

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this doesn't account for setSource which is what ends up being used if a source handler is in effect.

video.js/src/js/player.js

Lines 1916 to 1919 in 2e2dbde

if (currentTech.prototype.hasOwnProperty('setSource')) {
this.techCall_('setSource', source);
} else {
this.techCall_('src', source.src);

Copy link
Member

Choose a reason for hiding this comment

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

why would loadstartlistener get called on the wrong loadstart?

Copy link
Member

Choose a reason for hiding this comment

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

Here's the scenario:

  1. Call setSrc()
  2. Video element fires loadstart
  3. Call setSrc()

loadStartListener_() would be removed on the second call to setSrc() but the inline listener that triggers the loadstartlistener on the next loadstart would not. So, the very next loadstart would run loadstartlistener.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, the first loadstart listener would not be removed and would trigger again.


this.el_.src = src;
}

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

/**
* Load media into player
*
Expand Down Expand Up @@ -1148,7 +1158,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
29 changes: 29 additions & 0 deletions test/unit/tech/html5.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ 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 @@ -442,3 +444,30 @@ 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));
},
Copy link
Member

Choose a reason for hiding this comment

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

Mocking one() and off() may make this test a little too artificial.

Copy link
Author

Choose a reason for hiding this comment

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

It does successfully test that setSrc clears contentSource_ after loadstart. Do you have any suggestions on how to make this more real?

Copy link
Member

Choose a reason for hiding this comment

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

I'd create an object that inherited from EventTarget right in the test here and use that.

Copy link
Member

Choose a reason for hiding this comment

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

We tried that. thing is a a tech mock who's one method signature is different. We could make it be a standalone Component instance, possibly.

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);

});