#484 ✓ staged
Scott Downe

all plugins in core should be reviewed for teardown opportunities

Reported by Scott Downe | April 18th, 2011 @ 06:06 PM | in 0.6

Once #480 lands, we should review all core plugins for anything that should be cleaned when a track is destroyed.

Comments and changes to this ticket

  • annasob

    annasob May 10th, 2011 @ 05:01 PM

    • Assigned user set to “Scott Downe”
  • Scott Downe

    Scott Downe May 12th, 2011 @ 09:35 AM

    • State changed from “new” to “assigned”
    • Milestone order changed from “36” to “0”
  • Scott Downe

    Scott Downe May 13th, 2011 @ 04:05 PM

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

    Scott Downe May 13th, 2011 @ 04:11 PM

    Most plugins followed about 2-3 methods of creation, so teardowns started to follow the same thing.

    Some plugins didn't need a teardown, usually any plugins that simply changed the innerHTML of a shared element to "" would not need a teardown (subtitles, lowerthird, as an example).

    Any that have element appended, and used style.display = "none" needed the element to be removed.

    Any that have objects not stored in _setup, needed to be destroyed manually as well. Example: lastfm.

    similar teardown methods have similar testing methods.

    Once you notice the patterns used across plguin setup/teardown, the commit won't seem so large.

  • Rick

    Rick May 13th, 2011 @ 04:39 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user set to “annasob”

    Tested all plugin test suites, full pass in:

    • FF 3.6, 4
    • Chrome 11, 12
  • annasob

    annasob May 16th, 2011 @ 01:46 PM

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

    Ok I see a couple of issues :

    • you are not checking to see if options.target exists before removing its child

    • google feed makes var tmp = new GFdynamicFeedControl which i am not sure if it is ever destroyes

    • lint errors in mustache:

    Linting ./plugins/mustache/popcorn.mustache.js
    
            getData, data, getTemplate, template = null;
    
        Problem at line 144 character 16: Missing semicolon.
    
            getData, data, getTemplate, template = null;
    
        Problem at line 144 character 16: Expected an identifier and instead saw ','.
    
    undefined
    
  • Scott Downe

    Scott Downe May 16th, 2011 @ 07:24 PM

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

    annasob May 17th, 2011 @ 11:39 AM

    • State changed from “super-review-requested” to “review-looks-good”

    This looks good now ty

  • annasob

    annasob May 17th, 2011 @ 11:39 AM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

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

Pages