We need a similar way to destroy youtube, as we do with plugins.
Example, the youtube tests create a bunch of instances of popcorn.youtube, which can bog down the test suite, causing tests to fail because of weird timeouts.
Destroying a player wouldn't require a reference to a player, but it would require player plugins to have a _teardown.
Maybe we don't need a removePlugin, maybe we need popcorn.destroy() to call any player _teardown functions.
Maybe we just let the players expose a function onto the popcorn instance, and core doesn't have to do nothing.
These are ideas to the problem of Youtube needs a way to clean itself up.
Comments and changes to this ticket
I think the larger issue here is the player itself, and not the track events. Don't event teardowns already get called on destroy?
An example of where this is a problem is the youtube tests.
Each test creates a fresh instance of youtube, as tests should, but the old instance still lives. After a few unit tests, the page gets bogged down by all these flash players.
I am leaning towards the solution to allow player authors to create a teardown, which is called when p.destroy is called.
I like the idea of a player exposing a _teardown function and destroy simply calling them, but I think there is also another problem here. With HTML5 video we are able to have multiple popcorn instances off of one video but with youtube and vimeo it seems to be 1 instance per video. I think we need a way to allow an instance to use an already existing youtube/vimeo player.
- Milestone order changed from 9 to 0
Dave raises a good point about HTML5 allowing multiple instances per player, where as youtube does not.
Very good point, but I don't think this is the problem of this ticket.
Also, this might just be something player authors have to decide on. They have to make the choice if their player will create an instance and destroy an instance, or have an instance passed in, and thus, not have to destroy it.
- State changed from assigned to peer-review-requested
- Assigned user changed from Scott Downe to cadecairos
Settled on a _teardown functionality.
Added tests for this base functionality in the player module.
Implemented the teardown in the youtube player.
youtube tests for the youtube player's new teardown.
Also upgraded youtube tests to call destroy, thus cleaning themselves up. This makes the tests much better.
- State changed from peer-review-requested to review-needs-work
- Assigned user changed from cadecairos to Scott Downe
Left a comment, What do you think Scott?
- State changed from super-review-requested to review-looks-good
- Assigned user changed from David Seifried to cadecairos
Yea the tests look a lot better now, not a 10 video playing at once anymore :)
The fix looks good, passes lint.
The error test is still failing, but if that fix isn't in this ticket then it should be fine once merged with develop.
- State changed from review-looks-good to staged
- Milestone order changed from 11 to 0
The error test passes with scott's fix now
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.
Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.