#589 ✓ staged
David Seifried

Processing plugin should have a teardown function

Reported by David Seifried | June 22nd, 2011 @ 04:10 PM | in 0.7

Just realized this now after writing a teardown function for another ticket. Should also have unit tests that reflect this as well.

Comments and changes to this ticket

  • cadecairos

    cadecairos June 23rd, 2011 @ 11:48 AM

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

    can only target divs now. perf fixes. _teardown method + tests

    Edit:

    t589

  • David Seifried

    David Seifried June 23rd, 2011 @ 12:19 PM

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

    In processing.popcorn.unit.js:

    From lines 101 to 105 you declar a fer variables that are only used once, which really doesn't have to be done.

        var idx = 5, div, id;
        while ( --idx ) {
          div = document.getElementById( "processing-div-" + ( idx ) );
          id = trackids.pop();
          popped.removeTrackEvent( id );
          ok ( !div.children[0], "track event was removed" );
          plus();
        }
    

    can be done like this instead:

        var idx = 5;
        while ( --idx ) {
          popped.removeTrackEvent( trackids.pop() );
          ok ( !document.getElementById( "processing-div-" + ( idx ) ).children[ 0 ], 
             "track event was removed" );
          plus();
        }
    

    Also on line 105 of that

     ok ( !div.children[0], "track event was removed" );
    

    Needs some padding around the index of the array.

    The main issue is that scrolling back after a track has been removed causes a slew of errors to be thrown like the following:

    tracksByStart[tracks.endIndex] is undefined
    that.data.di...endIndex ]._natives.type ) === -1 ) {

    Scrolling back after a trackEvent has been removed should not cause any errors to be thrown.

  • cadecairos

    cadecairos June 23rd, 2011 @ 02:04 PM

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

    alright, made requested changes. t589

    this ticket also found ticket #593

  • Rick

    Rick June 23rd, 2011 @ 02:27 PM

    Nice work tracking down that error in #593

  • David Seifried

    David Seifried June 23rd, 2011 @ 02:52 PM

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

    Alright with the changes in #593)
    this will be good to go. Setting to SR, but should wait until #593) lands to really be sure.
    PR+

    Tested in FF4 and Chrome 12, passes lint, passes processing test suite

  • Rick

    Rick June 23rd, 2011 @ 02:59 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Rick” to “Jon Buckley”

    SR+

    Tested: processing plugin test suite;

    Passing in:

    • FF 3.6 (stable), 5.x (stable), 6.x (Aurora), 7.x (Nightly)

    • Chrome 12 (stable), 13 (beta), 14 (Canary)

    Lint passing

  • Jon Buckley

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