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 2 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
5 changes: 5 additions & 0 deletions src/js/event-target.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = Function.prototype;
Copy link
Member

Choose a reason for hiding this comment

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

We have encountered issues with using Function.prototype as a no-op due to the way video.js tracks functions by a GUID property. I don't think it would be an issue here, but I think it's safer to never use it. Unfortunately. 😞

Copy link
Author

Choose a reason for hiding this comment

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

What do you propose? () => {}?

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

EventTarget.prototype.trigger = function(event) {
Expand Down
13 changes: 11 additions & 2 deletions src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class Html5 extends Tech {

if (this.featuresNativeTextTracks) {
if (crossoriginTracks) {
log.warn(tsml`Text Tracks are being loaded from another origin but the crossorigin attribute isn't used.
log.warn(tsml`Text Tracks are being loaded from another origin but the crossorigin attribute isn't used.
Copy link
Member

Choose a reason for hiding this comment

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

tsml will strip spaces between lines so this will end up with "isn't used.This may", which isn't the end of the world, but we could use my alternative package tsmlj which will join each line with a space.

Copy link
Author

Choose a reason for hiding this comment

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

Will tell me editor to cut it out with the whitespace trimming.

This may prevent text tracks from loading.`);
}

Expand Down 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
28 changes: 28 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,9 @@ 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 TestHelpers from '../test-helpers.js';
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 +445,28 @@ test('Html5#reset calls Html5.resetMediaElement when called', function() {

Html5.resetMediaElement = oldResetMedia;
});

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

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: () => {},
Copy link
Member

Choose a reason for hiding this comment

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

we should verify that this has been called.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

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

});