#1365 ✓ staged
Scott Downe

Test breakage

Reported by Scott Downe | November 1st, 2012 @ 06:42 PM | in 1.4

Looks to be #1337.

Comments and changes to this ticket

  • Jon Buckley

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

    CCing Charlie, looks like we missed an edge case in the patch.

  • Rick

    Rick November 2nd, 2012 @ 11:29 AM

    Hm. I ran and re-ran these many times, but that was months ago at this point :(

    Edit: I'll take responsibility for these

  • Rick

    Rick November 2nd, 2012 @ 11:55 AM

    • State changed from “assigned” to “peer-review-requested”
  • Jon Buckley

    Jon Buckley November 2nd, 2012 @ 06:08 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Jon Buckley” to “Rick”
    • Milestone order changed from “21” to “0”

    I made a note in the pull request; up to you whether it makes sense or not. After this, off to SR by... Scott!

  • Rick

    Rick November 5th, 2012 @ 10:30 AM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “Scott Downe”
    • Milestone order changed from “125” to “0”

    I'd rather black list, as it's only two possibilities and everything else is allowed.

  • Scott Downe

    Scott Downe November 6th, 2012 @ 12:00 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “Rick”
    • Milestone order changed from “175” to “0”

    I wanted to draw attention to something, and if it is pedantic we can just stage this as is as it works great.

    I think we can remove one if block.

    The idea is this:

              if ( reserved.concat( [ "type", "manifest" ] ).indexOf( type ) === -1 ) {
                this.on( type, callback );
              }
    

    I decided manifest and type were not really reserved, but invalid because they did not have callbacks, so adding them right to reserved didn't make sense.

    My goal here was not to restructure the array, what it means, or make a new one. My goal was to reduce an extra if block.

    SR+ with fixes or comments on the above.

  • Rick

    Rick November 6th, 2012 @ 12:29 PM

    That works for me. I'll make the changes and land the patch

  • Rick

    Rick November 6th, 2012 @ 12:46 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “Scott Downe”
    • Milestone order changed from “125” to “0”

    While working the changes in, it occurred to me that "reserved" was semantically inaccurate, since the meaning was actually just: "properties we won't auto-bind events for". I've updated the list to include "type" and "manifest" and changed the identifier to "blacklist".

    Scott, SR?

  • Scott Downe

    Scott Downe November 6th, 2012 @ 02:50 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Scott Downe” to “Rick”
    • Milestone order changed from “175” to “0”

    SR+

  • Scott Downe

    Scott Downe December 13th, 2012 @ 01:50 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

Pages