[webkit-reviews] review granted: [Bug 65884] New Layout tests needed for WebVTT cue text parsing rules : [Attachment 105233] addressing Eric's suggestions

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 65884: New Layout tests needed for WebVTT cue text parsing rules
https://bugs.webkit.org/show_bug.cgi?id=65884

Attachment 105233: addressing Eric's suggestions
https://bugs.webkit.org/attachment.cgi?id=105233&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105233&action=review


This is terrific, thanks!

Not setting cq+, but only because of your comment that it will need to be
landed by hand.


> LayoutTests/media/track/track-webvtt-tc014-alignment.html:10
> +	       var numberTests = 0;
> +	       var numberTracksLoaded = 0;

Suggestion: For a future change, you might want to consider initializing
"numberTests" (perhaps renamed to "numberOfTests") to the number of tests to
perform...

> LayoutTests/media/track/track-webvtt-tc014-alignment.html:15
> +		   numberTracksLoaded++;
> +		   if (numberTracksLoaded == 3) {

... and compare it to "numberTracksLoaded" (perhaps renamed to
"numberOfTracksLoaded) ...

> LayoutTests/media/track/track-webvtt-tc014-alignment.html:64
> +		   numberTests++;
> +		   if (numberTests >= 3)
> +		       endTest();

... and then decrement it here and call endTest() once it gets to zero. This
would keep you from having to hard code the number of tests in multiple places.



> LayoutTests/media/video-test.js:245
> +    testExpected(cues.length, expected.length);

Suggestion: for a future check-in, you might change this to 

    testExpected('cues.length', expected.length);

so the results will have something like :

    EXPECTED (cues.length == '0') OK

instead of 

    EXPECTED (2 == '2') OK