#1337 ✓ staged
Charlie Stigler

Refactor Popcorn instance _events stack to use an array instead of plain object

Reported by Charlie Stigler | September 18th, 2012 @ 06:47 PM | in 1.4

This is a follow-up to #1325, and probably should have been included in that same patch.

Popcorn.js will attempt to index functions internally by function name, before falling back to stringifying the function and adding a GUID. When indexing by function name, unexpected behavior can occur:

  • If I add two listeners, both named "onPlay," from two separate sections of code, one will overwrite the other and only one will fire
  • It also causes hard-to-track bugs in compilers like Google's Closure Compiler, which changes function names itself and will often reuse function names.

We should just always use the fallback method of stringify + GUID, which is a fine way to track them. Patch forthcoming to do this.

Comments and changes to this ticket

  • Charlie Stigler

    Charlie Stigler September 18th, 2012 @ 06:52 PM

    Or actually, given that function.toString() is a very slow method and it will have to be called very often, perhaps just eventType + GUID is better. A collision would still be so rare as to be impossible.

  • Charlie Stigler

    Charlie Stigler September 18th, 2012 @ 06:58 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “Jon Buckley”
    • Milestone order changed from “4” to “0”

    Pull Request: https://github.com/mozilla/popcorn-js/pull/211

    Patch is less than one line changed, so I don't forsee this being a hard PR =).

  • Rick

    Rick September 18th, 2012 @ 07:13 PM

    • State changed from “peer-review-requested” to “duplicate”
    • Assigned user cleared.

    Can we close one of these? The correct fix is to switch the event storage away from a plain object and use an array instead. That way it can be removed by reference

  • Rick

    Rick September 18th, 2012 @ 07:28 PM

    • Title changed from “Popcorn still uses function.name to manage event listeners etc.” to “Refactor Popcorn instance _events stack to use an array instead of plain object”
  • Rick

    Rick September 18th, 2012 @ 07:28 PM

    • State changed from “duplicate” to “assigned”
    • Assigned user set to “Charlie Stigler”
    • Milestone order changed from “16” to “0”
  • Rick

    Rick September 18th, 2012 @ 07:28 PM

    Charlie, Do you think you can handle this change?

  • Charlie Stigler

    Charlie Stigler September 18th, 2012 @ 07:30 PM

    Yeah, got it, that's definitely a better solution. Fixed patch coming up.

  • Rick

    Rick September 18th, 2012 @ 07:48 PM

    Excellent! I'm on IRC, ping me when you want a review :)

  • Charlie Stigler

    Charlie Stigler September 18th, 2012 @ 09:06 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “Rick”
    • Milestone order changed from “4” to “0”

    OK, got this working and pushed to the same pull request. I only ran into one issue, when iterating through an array with a simple for-loop or even with forEach, if the array changes (i.e. a callback calls off() within it) it will screw with the iteration. I kind of expected forEach to take care of things like that. So instead I made it just clone the array beforehand and iterate with that. I may be missing some simpler way to accomplish this, though.

  • Rick

    Rick September 18th, 2012 @ 11:28 PM

    Interesting solution... It's late now, so I will dig into this tomorrow. Glad you could get the refactor in tonight :)

  • Rick

    Rick September 18th, 2012 @ 11:42 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “Charlie Stigler”
    • Milestone order changed from “121” to “0”

    I actually couldn't help myself :)

    The copy of the array is a solid strategy, great work. Some inline comments for you in the PR.

  • Charlie Stigler

    Charlie Stigler September 19th, 2012 @ 01:05 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “Rick”
    • Milestone order changed from “4” to “0”

    Great, thanks for the review. I fixed those issues you pointed out.

    Switching the clonedEvents iteration to your while-loop is definitely more concise/nicer, but it does remove the typeof check that ensured we would only call functions. I have no idea why a non-function would end up in the listeners array anyway. I am guessing that it is probably unnecessary and we can drop it, but it's worth noting that we are losing a check that used to be there.

  • Rick

    Rick September 19th, 2012 @ 08:57 AM

    I'm trying to think of when/how that would be anything but a function.

    When I first wrote that I was probably being unnecessarily paranoid :) likely the same reason that I insisted on using an object (with string keys??) to store the events.

    Maybe a better way is to do the check when the event is first bound? Inside listen/on, you could do a check that throws if the user code passes anything but a function. Try that out and tell me what you think :)

  • Rick

    Rick September 19th, 2012 @ 12:04 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “Charlie Stigler”
    • Milestone order changed from “121” to “0”

    Charlie, Looking awesome so far.

    I added a small note about code org, but that's probably the last thing I'll nit about.

    Really great work on this :)

  • Charlie Stigler

    Charlie Stigler September 19th, 2012 @ 05:22 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Charlie Stigler” to “Rick”
    • Milestone order changed from “4” to “0”

    OK, fixed that up and added a type check in listen(). I left the other large var declaration (in listen) alone because it was already there, and because I think leaving self = this at the top is elegant.

  • Rick

    Rick September 21st, 2012 @ 08:23 PM

    This looks really awesome! I will dig in after dinner :)

  • Rick

    Rick September 22nd, 2012 @ 10:51 AM

    • State changed from “peer-review-requested” to “under-review”
  • Charlie Stigler

    Charlie Stigler October 21st, 2012 @ 10:13 PM

    Just wanted to bump this, I think it's pretty important.

  • Rick

    Rick October 26th, 2012 @ 09:02 AM

    • State changed from “under-review” to “super-review-requested”
    • Assigned user changed from “Rick” to “Jon Buckley”
    • Milestone order changed from “121” to “0”

    Oof. I'm sorry, I thought I had marked this as reviewed... I actually thought it was already landed!

  • Jon Buckley

    Jon Buckley November 1st, 2012 @ 12:25 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Jon Buckley” to “Charlie Stigler”
    • Milestone order changed from “21” to “0”

    SR+ with nits, I think there's some dead code you can remove that I've noted in the pull request. Also, please rebase on master and we can land this sucker!

  • Rick

    Rick November 1st, 2012 @ 01:20 PM

    Jon, I just responded to your nit about the deadcode, that's a special case path for event hooks.

  • Jon Buckley

    Jon Buckley November 1st, 2012 @ 01:21 PM

    Oops, yep, don't take that code out. Rebase and this is ready to go!

  • Charlie Stigler
  • Jon Buckley

    Jon Buckley November 1st, 2012 @ 02:46 PM

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

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