Created attachment 104956 [details] Patch Thanks to Silvia Pfeiffer for created the .vtt test cases.

Comment on attachment 104956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104956&action=review Marking r+, but please consider the cleanup suggestions. > LayoutTests/ChangeLog:8 > + * media/track/captions/tc014_alignment.vtt: Added. Nit: It might be useful to put "vtt" in the folder name in case we support other caption formats in the future, eg. "media/track/captions-webvtt". > LayoutTests/ChangeLog:10 > + * media/track/captions/tc014_alignment_ltr.vtt: Added. > + * media/track/captions/tc014_alignmentbad.vtt: Added. Nit: Is there a reason that the vtt files use an underscore between words and the tests use a dash (as most WebKit tests do)? > LayoutTests/ChangeLog:13 > + * media/track/captions/tc015_positioningbad.vtt: Added. Nit: is there a reason that the "bad" tests don't have a separator before "bad", eg. "tc015-positioning-bad.vtt"? > LayoutTests/ChangeLog:16 > + * media/track/captions/tc017_linepos.vtt: Added. Nit: There isn't any reason to abbreviate "positioning", eg. "tc017-line-positioning.vtt", etc (we don't live in an 8.3 world any more ;-O ) > LayoutTests/ChangeLog:23 > + * media/track/captions/tc019_cuesize.vtt: Added. > + * media/track/captions/tc019_cuesizebad.vtt: Added. > + * media/track/captions/tc020_cuesize_align.vtt: Added. > + * media/track/captions/tc020_cuesize_alignbad.vtt: Added. Nit: These test names would be slightly easier to read with a separator between words, eg. "tc019-cue-size.vtt", etc. > LayoutTests/ChangeLog:28 > + * media/track/captions/tc022_entitieswrong.vtt: Added. Ditto. > LayoutTests/ChangeLog:37 > + * media/track/captions/tc027_emtycue.vtt: Added. Typo: "emty" -> "empty" > LayoutTests/ChangeLog:44 > + * media/track/track-webvtt-tc016-align_positioning-expected.txt: Added. > + * media/track/track-webvtt-tc016-align_positioning.html: Added. Nit: Dashes and underscores? > LayoutTests/ChangeLog:48 > + * media/track/track-webvtt-tc018-align_text_lineposition-expected.txt: Added. > + * media/track/track-webvtt-tc018-align_text_lineposition.html: Added. Ditto > LayoutTests/ChangeLog:52 > + * media/track/track-webvtt-tc020-cuesizealign-expected.txt: Added. > + * media/track/track-webvtt-tc020-cuesizealign.html: Added. Nit: Separate the words please. > LayoutTests/ChangeLog:68 > + * media/track/track-webvtt-tc027-emptycue-expected.txt: Added. > + * media/track/track-webvtt-tc027-emptycue.html: Added. > + * media/track/track-webvtt-tc028-unsupportedmarkup-expected.txt: Added. > + * media/track/track-webvtt-tc028-unsupportedmarkup.html: Added. Ditto. > LayoutTests/media/track/captions/tc014_alignment.vtt:3 > +WEBVTT > + > +1 It would be really helpful to have a comment in each file describing the purpose of the test, expected behavior, etc. > LayoutTests/media/track/track-webvtt-tc014-alignment.html:57 > + <video> > + <track src="captions/tc014_alignment.vtt" onload="trackLoaded(0)"> > + <track src="captions/tc014_alignment_ltr.vtt" onload="trackLoaded(1)"> > + <track src="captions/tc014_alignmentbad.vtt" onload="trackWithErrorLoaded(2)"> > + </video> This assumes that the track "load" events will always fire in the same order. Is this guaranteed? (Here and elsewhere). > LayoutTests/media/track/track-webvtt-tc015-positioning.html:34 > + for (cue = 0; cue < video.textTracks[i].cues.length; cue++) { > + testExpected("video.textTracks[" + i + "].cues[" + cue + "].textPosition","50"); > + } "{ }" not needed for the one-line loop. > LayoutTests/media/track/track-webvtt-tc022-entities.html:18 > + testExpected("video.textTracks[" + i + "].cues[0].getCueAsHTML().textContent","This cue has an amp & character."); > + testExpected("video.textTracks[" + i + "].cues[1].getCueAsHTML().textContent","This cue has a less than < character."); > + testExpected("video.textTracks[" + i + "].cues[2].getCueAsHTML().textContent","This cue has a greater than > character."); Nit: missing a space after the commas. > LayoutTests/media/track/track-webvtt-tc022-entities.html:30 > + testExpected("video.textTracks[" + i + "].cues[0].getCueAsHTML().textContent","This cue has an amp character.\nAmpersand is ignored."); > + testExpected("video.textTracks[" + i + "].cues[1].getCueAsHTML().textContent","This cue has a less than "); > + testExpected("video.textTracks[" + i + "].cues[2].getCueAsHTML().textContent","This cue has a greater than > character.\nSince it's not related to a < character,\nit's just interpreted as text."); Ditto. > LayoutTests/media/track/track-webvtt-tc027-emptycue.html:9 > + var numberTests = 0; This variable is unused.

Also, many of these tests are very similar in structure, so you might want to consider making them data driven by adding a function to one of the shared js files that takes an object that defines expected test results. One way to do it would be a function like this (warning, untested code ahead): function testCues(index, expected) { consoleWrite("<br>*** Testing text track " + index); var cues = video.textTracks[index].cues; testExpected("cues.length", expected.length); for (i = 0; i < cues.length; i++) { var cue = cues[i]; for (j = 0; j < cues.length; j++) { var test = expected.tests[j]; testExpected("cue." + test.property, test.values[j]); } } Which would allow you to replace this: testExpected("video.textTracks[" + i + "].cues.length", "7"); testExpected("video.textTracks[" + i + "].cues[0].linePosition","0"); testExpected("video.textTracks[" + i + "].cues[0].snapToLines",false); testExpected("video.textTracks[" + i + "].cues[1].linePosition","0"); testExpected("video.textTracks[" + i + "].cues[1].snapToLines",true); testExpected("video.textTracks[" + i + "].cues[2].linePosition","50"); testExpected("video.textTracks[" + i + "].cues[2].snapToLines",false); testExpected("video.textTracks[" + i + "].cues[3].linePosition","5"); testExpected("video.textTracks[" + i + "].cues[3].snapToLines",true); testExpected("video.textTracks[" + i + "].cues[4].linePosition","100"); testExpected("video.textTracks[" + i + "].cues[4].snapToLines",false); testExpected("video.textTracks[" + i + "].cues[5].linePosition","-1"); testExpected("video.textTracks[" + i + "].cues[5].snapToLines",true); testExpected("video.textTracks[" + i + "].cues[6].linePosition","500"); testExpected("video.textTracks[" + i + "].cues[6].snapToLines",true); With this: var expected = { length : 7, tests: [ { property : "linePosition", values : [0, 0, 50, 5, 100, -1, 500], }, { property : "snapToLines", values : [false, true, false, true, false, true, true], }, ], }; testCues(i, expected); IMO, this would make the tests easier to read, understand, and modify.

Comment on attachment 104956 [details] Patch Thanks Eric. New patch on the way. View in context: https://bugs.webkit.org/attachment.cgi?id=104956&action=review >> LayoutTests/ChangeLog:8 >> + * media/track/captions/tc014_alignment.vtt: Added. > > Nit: It might be useful to put "vtt" in the folder name in case we support other caption formats in the future, eg. "media/track/captions-webvtt". Done. >> LayoutTests/ChangeLog:10 >> + * media/track/captions/tc014_alignmentbad.vtt: Added. > > Nit: Is there a reason that the vtt files use an underscore between words and the tests use a dash (as most WebKit tests do)? No. Changed. >> LayoutTests/ChangeLog:13 >> + * media/track/captions/tc015_positioningbad.vtt: Added. > > Nit: is there a reason that the "bad" tests don't have a separator before "bad", eg. "tc015-positioning-bad.vtt"? Nope, done. >> LayoutTests/ChangeLog:16 >> + * media/track/captions/tc017_linepos.vtt: Added. > > Nit: There isn't any reason to abbreviate "positioning", eg. "tc017-line-positioning.vtt", etc (we don't live in an 8.3 world any more ;-O ) Done. >> LayoutTests/ChangeLog:23 >> + * media/track/captions/tc020_cuesize_alignbad.vtt: Added. > > Nit: These test names would be slightly easier to read with a separator between words, eg. "tc019-cue-size.vtt", etc. Done. >> LayoutTests/ChangeLog:28 >> + * media/track/captions/tc022_entitieswrong.vtt: Added. > > Ditto. Done. >> LayoutTests/ChangeLog:37 >> + * media/track/captions/tc027_emtycue.vtt: Added. > > Typo: "emty" -> "empty" Done. >> LayoutTests/ChangeLog:44 >> + * media/track/track-webvtt-tc016-align_positioning.html: Added. > > Nit: Dashes and underscores? Gone. >> LayoutTests/ChangeLog:48 >> + * media/track/track-webvtt-tc018-align_text_lineposition.html: Added. > > Ditto Done. >> LayoutTests/ChangeLog:52 >> + * media/track/track-webvtt-tc020-cuesizealign.html: Added. > > Nit: Separate the words please. Done. >> LayoutTests/ChangeLog:68 >> + * media/track/track-webvtt-tc028-unsupportedmarkup.html: Added. > > Ditto. Done. >> LayoutTests/media/track/captions/tc014_alignment.vtt:3 >> +1 > > It would be really helpful to have a comment in each file describing the purpose of the test, expected behavior, etc. Done. >> LayoutTests/media/track/track-webvtt-tc014-alignment.html:57 >> + </video> > > This assumes that the track "load" events will always fire in the same order. Is this guaranteed? (Here and elsewhere). Ugh. You are quite right. They've been running fine for me, but of course there is no guarantee on the timing of the event fire. I've changed all the tests that use multiple track loads to wait until all tracks have loaded before performing the tests. >> LayoutTests/media/track/track-webvtt-tc015-positioning.html:34 >> + } > > "{ }" not needed for the one-line loop. Gone. >> LayoutTests/media/track/track-webvtt-tc022-entities.html:18 >> + testExpected("video.textTracks[" + i + "].cues[2].getCueAsHTML().textContent","This cue has a greater than > character."); > > Nit: missing a space after the commas. Gone. >> LayoutTests/media/track/track-webvtt-tc022-entities.html:30 >> + testExpected("video.textTracks[" + i + "].cues[2].getCueAsHTML().textContent","This cue has a greater than > character.\nSince it's not related to a < character,\nit's just interpreted as text."); > > Ditto. Gone. >> LayoutTests/media/track/track-webvtt-tc027-emptycue.html:9 >> + var numberTests = 0; > > This variable is unused. Gone.

Created attachment 105233 [details] addressing Eric's suggestions

Comment on attachment 105233 [details] addressing Eric's suggestions Thanks for the suggestions, Eric! They were all great ideas. But 2 downsides to the changes: 1) I went ahead and made changes to all the other LayoutTests for consistency. So, the patch is much bigger, sorry about that. 2) I believe this patch will need to be landed by hand (for the same reasons as the first round of media/track/ tests required manual landing. I do think this is ready to go though, if you agree? Thanks!

Comment on attachment 105233 [details] addressing Eric's suggestions 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

Created attachment 105365 [details] Eric's suggestions Eric, your suggestions were easy enough, I decided to just add them now rather than later.

Comment on attachment 105365 [details] Eric's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=105365&action=review > LayoutTests/media/video-test.js:252 > + testExpected("cues[i]." + test.property, test.values[i]); Suggestion: for a future modification you might consider changing this so the track number is printed as it is in the tests that don't use this function. IOW, something like this: testExpected("cues[" + i + "]." + test.property, test.values[i]); so the results look like this: EXPECTED (cues[0].direction == 'vertical') OK instead of this: EXPECTED (cues[i].direction == 'vertical') OK

Committed r93885: <http://trac.webkit.org/changeset/93885>