#751 ✓ staged
Steven W

Refactor the SSA parser

Reported by Steven W | September 30th, 2011 @ 12:22 AM | in 1.0 Release (closed)

Just like #750. Refactor this parser, clean it up, and make it shine.

Comments and changes to this ticket

  • Steven W

    Steven W October 1st, 2011 @ 12:06 PM

    • Tag set to parsers
  • David Seifried

    David Seifried October 2nd, 2011 @ 12:36 AM

    • Milestone set to 1.0 Release
    • Milestone order changed from “76” to “0”
  • Steven W

    Steven W October 2nd, 2011 @ 06:48 PM

    • State changed from “new” to “peer-review-requested”

    Fixed in branch:
    https://github.com/stevenaw/popcorn-js/tree/t751/

    Passes all tests in:
    FF7
    Safari 5
    Chrome 15
    IE9

    According to qunit's timing, it seems to work a few milliseconds quicker now too!

  • cadecairos

    cadecairos October 17th, 2011 @ 12:03 PM

    • Assigned user set to “cadecairos”
  • cadecairos

    cadecairos October 17th, 2011 @ 12:10 PM

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

    Looks good,

    Tested: SSA Parser Unit Tests

    On: Firefox 7, Firefox 3.6, Chrome 14, Safari 5

    All passing

    Passes lint.

    PR+

  • David Seifried

    David Seifried October 17th, 2011 @ 12:55 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Steven W”

    Since this is a cleanup ticket as well, we may as well fix styling in the parser while were here as it needs to be done ( we just finished cleaning up all the plugins ).

    popcorn.parserSSA.js:

    • line 3 needs whitespace padding around "Popcorn"
    • line 100 could use a bit more whitespace padding around the multiplication thats being done ( around the * )
    • lines 5, 13, 15, 29, 44, 49, 54, 56, 62, 66, 73, 78, 83, 86, 89, 94, 99, 102, 107, 108, 112, 115, 121, 128, 139 all have end of line whitespace that needs to go

    popcorn.parserSSA.html:

    • all instances of single quotes need to be switched over to use double quotes with the exception of lines 33 and 37 ( this has to be written the way it is according to the HTML5 video spec )
    • lines 5, 10, 29, 33, 37, 39, 41 have end of line whitespace

    popcorn.parserSSA.unit.html:

    • same as above with single quotes
    • lines 7, 8, 11, 25, 26, 29, 33, 37, 38, 43 have end of line whitespace

    popcorn.parserSSA.unit.js:

    • line 24 can use some padding around the "*"
    • line 39 can use some padding around evt
    • line 41 array whitespace padding
    • lines 2, 25, 31, 33, 36, 42, 51, 56 have end of line whitespace that needs to go

    Sorry to be so harsh on the styling, but on our may to 1.0 we may as well make everything that we are going ship look nice and pretty according to the style guide :)

    Other than styling, everything looks good. Passes lint, passes in FF 7, FF 3.6, Opera 11.* , Safari, Chrome.

  • Steven W

    Steven W October 17th, 2011 @ 08:00 PM

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

    No worries, the styling of the parsers has needed a cleanup for a while anyways. Makes sense to do it in the same ticket.
    Re-pushed to the branch t751, available here:

    https://github.com/stevenaw/popcorn-js/tree/t751/

  • cadecairos

    cadecairos October 18th, 2011 @ 10:23 AM

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

    PR Still stands

  • David Seifried

    David Seifried October 19th, 2011 @ 10:51 AM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Steven W”

    Almost done just 2 style changes:

    popcorn.parserSSA.js:

    Line 100 - the first instance of "*" on this line needs some whitespace padding around it
    Line 108 - end of line whitespace

    After this is done this is good to be staged. Linting and unit test passes still stand.

    Thanks again for working on all these parser bugs, its awesome.

  • Steven W

    Steven W October 20th, 2011 @ 08:59 PM

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

    Np, can't believe I missed those first time through.
    Fixed and re-pushed. Passes unit tests.

    https://github.com/stevenaw/popcorn-js/tree/t751/

  • cadecairos

    cadecairos October 24th, 2011 @ 10:02 AM

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

    All you dave

  • David Seifried

    David Seifried October 24th, 2011 @ 04:27 PM

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

    Awesome, everything looks good. Still passing as per what I said before and styling is now 100%.

    SR+

  • cadecairos

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

Tags

Pages