#804 ✓ staged
Scott Downe

p/Popcorn.removePlayer function

Reported by Scott Downe | November 1st, 2011 @ 12:35 PM | in 1.2 (closed)

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

  • Rick

    Rick November 1st, 2011 @ 12:41 PM

    Perhaps iterate all of the instance track events and execute the _teardown for each?

  • Scott Downe

    Scott Downe November 11th, 2011 @ 05:14 PM

    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.

  • David Seifried

    David Seifried November 14th, 2011 @ 02:27 PM

    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.

  • David Seifried

    David Seifried November 16th, 2011 @ 03:06 PM

    • Assigned user set to “Scott Downe”
    • State changed from “open” to “assigned”
  • Scott Downe

    Scott Downe December 12th, 2011 @ 12:45 PM

    • Milestone changed from 1.1 to 1.2
    • Milestone order changed from “28” to “0”
  • Scott Downe

    Scott Downe March 1st, 2012 @ 04:51 PM

    • 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.

  • Scott Downe

    Scott Downe March 2nd, 2012 @ 02:31 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “cadecairos”

    https://github.com/ScottDowne/popcorn-js/commits/t804

    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.

  • cadecairos

    cadecairos March 2nd, 2012 @ 03:36 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “Scott Downe”
  • Scott Downe

    Scott Downe March 2nd, 2012 @ 03:45 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “cadecairos”

    Talked to Chris, and we agreed the request for a combineFn utility function needs to be in a new ticket, with some discussion.

  • cadecairos

    cadecairos March 2nd, 2012 @ 03:59 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “cadecairos” to “David Seifried”

    filed #929 about combine Fn

    Other than that, this is awesome and works great, unit tests are all passing.

    lint looks good.

    PR+

  • David Seifried

    David Seifried March 2nd, 2012 @ 04:33 PM

    • 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.

    SR+

  • Jon Buckley

    Jon Buckley March 2nd, 2012 @ 09:59 PM

    • State changed from “review-looks-good” to “staged”
    • Milestone order changed from “11” to “0”

    The error test passes with scott's fix now

    Staged: https://github.com/cadecairos/popcorn-js/commit/ecd7f3c048910f14932...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

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.

Shared Ticket Bins

Referenced by

Pages