833386 - [webvtt] Tests for Track element and TextTrack* DOM classes

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Marcus Saad (:msaad)]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17

Comment 1

[David Humphrey (:humph)]

This is a spin-off from bug 629350 to do the tests necessary for the spec.

Comment 2

[David Humphrey (:humph)]

Jordan and Marcus are going to work on this together.

Comment 3

[Ralph Giles (:rillian)]

How's this going? I think the best approach here is to write mochitests. That's a javascript unit test library which runs in the context of a webpage.

See https://developer.mozilla.org/en-US/docs/Mochitest for how to get the current tests running and some introductory material on writing new ones.

Note that we have some extensions for dealing with timed media in content/media/tests/manifest.js (see MediaTestManager) The TextTrack and track element tests should definitely go there. I don't know about the dom API tests. It seems like those could be either in content/media/tests/ or dom/tests/. Pointer lock is under dom, but all the media element tests are under media, except for bug 766694.

I found copying some of the simpler media tests a helpful way to start. e.g.

https://mxr.mozilla.org/mozilla-central/source/content/media/test/can_play_type_ogg.js (pref flipping and API queries)
https://mxr.mozilla.org/mozilla-central/source/content/media/test/test_playback.html?force=1 (basic playback verification)

Comment 4

[Marcus Saad (:msaad)]

(In reply to Ralph Giles (:rillian) from comment #3)
> How's this going? I think the best approach here is to write mochitests.
> That's a javascript unit test library which runs in the context of a webpage.
> 
> See https://developer.mozilla.org/en-US/docs/Mochitest for how to get the
> current tests running and some introductory material on writing new ones.
> 
> Note that we have some extensions for dealing with timed media in
> content/media/tests/manifest.js (see MediaTestManager) The TextTrack and
> track element tests should definitely go there. I don't know about the dom
> API tests. It seems like those could be either in content/media/tests/ or
> dom/tests/. Pointer lock is under dom, but all the media element tests are
> under media, except for bug 766694.
> 
> I found copying some of the simpler media tests a helpful way to start. e.g.
> 
> https://mxr.mozilla.org/mozilla-central/source/content/media/test/
> can_play_type_ogg.js (pref flipping and API queries)
> https://mxr.mozilla.org/mozilla-central/source/content/media/test/
> test_playback.html?force=1 (basic playback verification)

Thank you for this guidance. We are definitely using mochitest, Jordan and I are already familiarized with how it works. Related to where we should land the tests, we were having this discussion today and we thought that laying the API tests at /dom/tests/mochitest/webvtt would be a good idea.

Soon, the tests will be available and you can take a look on what we've done so far. On

Comment 5

[Ralph Giles (:rillian)]

Ok, thanks for the update. I look forward to seeing the first code.

Comment 6

[raffoul.jordan]

Attachment: WIP webvtt TextTrack mochitests

Comment 7

[Marcus Saad (:msaad)]

Attachment: WIP HTMLTextTrack mochitests

This is a WIP of my current work on the HTMLTextTrack class and a few others related to TextTrack class. I've created a few mochitests based on the specification found here http://www.whatwg.org/specs/web-apps/current-work/#htmltrackelement. My goal is to be able to create at least 4 tests a day, and release a substantial WIP for 0.6 release. I've used the functions we talked about on IRC, such as addLoadEvent(), SimpleTest.finish() and SimpleTest.waitForExplicitFinish().

Also, I'm including comments more often to help documenting the tests.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 709970 [details] [diff] [review]
WIP HTMLTextTrack mochitests

Review of attachment 709970 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good start, thanks! Some comments below:

These tests are all about the HTML <track> element and it's associated apis. I don't think you need the 'webvtt' subdirectory in the path, the file names are descriptive enough for dom/media/tests/mochitest/.

Be careful with trailing whitespace in your files. A number of the tests have them.

I think I would combine a number of the test files. There's certainly value to starting with a fresh document, but I think the risk of using multiple track (and video) elements is low. So, for example, all of the attribute tests in one file, all of the cue loading tests in another. Each one shouldn't be unreasonably long, but it's ok to test more than one related aspect of the API from the same test file.

The conventions when posting patches in in bugzilla is to diff against mozilla-central (or another appropriate tree) as it stands at the time, so it's clear what you're proposing to add to the central repository. That is, you should include the tests that are already in David's patch, if they're not already on another bug.

::: dom/media/tests/mochitest/webvtt/HTMLTextTrack_defaultKind.html
@@ +11,5 @@
> +</head>
> +<body>
> +	<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=833386">Mozilla Bug { 833386 } - HTMLTrackElement, Defaul value for attribute DOMString kind - Expecting "subtitles" </a><br>
> +	<video width="320" height="240" controls>
> +		<track id="subtitlesEN" label="Testing Default Kind Value" srclang="en"/>

Isn't a 'src' attribute required?

@@ +17,5 @@
> +	<pre id="test">
> +	<script class="testbody" type="text/javascript">
> +	/** Test for Bug {833386} - Defaul value for attribute DOMString kind - Expecting "subtitles" **/
> +	SimpleTest.waitForExplicitFinish();
> +	

trailing whitespace.

@@ +24,5 @@
> +		/** Does it returns subtitles as the default value? **/
> +		is(track[0].kind,"subtitles");
> +		SimpleTest.finish();
> +	}
> +	addLoadEvent(check);

By calling check on document load we're hoping to check that the kind has its default value regardless of track.readyState?

::: dom/media/tests/mochitest/webvtt/HTMLTextTrack_kindAndSrclang_srclangOff.html
@@ +17,5 @@
> +	<pre id="test">
> +	<script class="testbody" type="text/javascript">
> +	/** Test for Bug {833386} -
> +	 * 
> +	 * The srclang attribute gives the language of the text track data. This attribute must be present if the element's kind attribute is in the subtitles state. [BCP47]

This is a conformance issue for producers, not consumers like a web browser. There's nothing we need to verify here.

@@ +29,5 @@
> +	function check(){
> +		var trackelement = document.getElementsByTagName('track');
> +		var track = trackelement[0];
> +			
> +		is(track.srclang,"");

So this is checking that srclang is defined and the empty string? Has the empty string as a default value in other words?

::: dom/media/tests/mochitest/webvtt/HTMLTextTrack_multipleTracks_false.html
@@ +33,5 @@
> +					}
> +				}
> +			}
> +		}
> +		is(equalTracks, false);

This is checking that all corresponding properties have distinct values?

Might be more clear to just check them against a second static array of expected values.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Ralph Giles (:rillian) from comment #8)
> Comment on attachment 709970 [details] [diff] [review]
> WIP HTMLTextTrack mochitests
> 
> Review of attachment 709970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like a good start, thanks! Some comments below:
> 
> These tests are all about the HTML <track> element and it's associated apis.
> I don't think you need the 'webvtt' subdirectory in the path, the file names
> are descriptive enough for dom/media/tests/mochitest/.

I think it's OK to have the test files in a subdir, as then you can just run mochitests on the subdir and avoid running all the other tests in the parent directory tree.

Comment 10

[Marcus Saad (:msaad)]

(In reply to Ralph Giles (:rillian) from comment #8)
> Comment on attachment 709970 [details] [diff] [review]
> WIP HTMLTextTrack mochitests
> 
> Review of attachment 709970 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/media/tests/mochitest/webvtt/HTMLTextTrack_defaultKind.html

> Isn't a 'src' attribute required?
> trailing whitespace.
> By calling check on document load we're hoping to check that the kind has
> its default value regardless of track.readyState?

 Yes, src is a required attribute, my mistake. I'll be more careful with trailing whitespaces. My though on this is that the kind doesn't depend on the state of the track.

> dom/media/tests/mochitest/webvtt/HTMLTextTrack_kindAndSrclang_srclangOff.html

> This is a conformance issue for producers, not consumers like a web browser.
> There's nothing we need to verify here.
> So this is checking that srclang is defined and the empty string? Has the
> empty string as a default value in other words?

Good to know, I was about to implement a BCP verification test. Regarding the assertion, it's checking if the srclang attribute is empty. Should I be doing it in a different way?

> ::: dom/media/tests/mochitest/webvtt/HTMLTextTrack_multipleTracks_false.html
> This is checking that all corresponding properties have distinct values?
> Might be more clear to just check them against a second static array of
> expected values.

Yes, it's verifying if the properties have a distinct value. This test is contradictory, I found here (https://developer.mozilla.org/en-US/docs/HTML/Element/track), saying that :
"A media element cannot have more than one track with the same kind, srclang, and label.", but the HTML Standard (http://www.whatwg.org/specs/web-apps/current-work/#htmltrackelement), doesn't say anything about it. Any thoughts on this rillian/cpearce?


I'll be uploading a v2 either tomorrow or wednesday. Thank you for all the help so far, it's good to work with you.

Comment 11

[Marcus Saad (:msaad)]

Attachment: Finished HTMLTrackElement tests

In this patch, I focussed on creating a single file for every property in the HTMLTrackElement class. Every html file tests each functionality and behaviour of a determined property. I did some research and looked to what google's WebKit did, so I took some inspiration from there. Next patch, will contain TextTrackCueList tests.
Changes added :
-Included track on html/content/test/test_bug389797.html
-Changed Mochitest/Makefile.in to include webvtt folder if WEB_VTT is defined
-Changed webvtt/Makefile.in to include the new tests
-Included the new tests for HTMLTrackElement.

Comment 12

[Marcus Saad (:msaad)]

I realized that the latest patch doesn't include tests for kind. I'll will be added.
Sorry for that.

Comment 13

[raffoul.jordan]

Attachment: WIP TextTrack Tests

Comment 14

[raffoul.jordan]

Attachment: TextTrack tests

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 732606 [details] [diff] [review]
TextTrack tests

Review of attachment 732606 [details] [diff] [review]:
-----------------------------------------------------------------

Reasonable start, but see the not about how to update the tests to function regardless of the initial pref setting.

I'd also like you to combine the tests more. E.g. all all different active_cue tests use the same vtt file and just test different seek points. It's fine to just have a sequence of seeks in the same test.

::: dom/media/tests/mochitest/TextTrack_active_cues_0.html
@@ +18,5 @@
> +  </video>
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +  SpecialPowers.setBoolPref("media.webvtt.enabled", true);

This isn't going to do anything. If the pref is true it has no effect, and if the pref if false, the HTML parser has already instantiated an HTMLUnknownElement and textTrack.activeCues.length will be undefined. if the is(undefined, 0) passes, I suppose that's ok, otherwise you need to protect that test with a check that media.webvtt.enabled was true when the page loaded.

We still want a test like this, to check static parsing, and it's ok if it doesn't do anything with the pref disabled. But we need a second variation of the test which does a document.createElement('track') to intantiate a new track element after flipping the pref.

::: dom/media/tests/mochitest/TextTrack_active_cues_1.html
@@ +29,5 @@
> +    textTrack = video.textTracks[0];
> +
> +    video.addEventListener('seeked', seeked);
> +    video.currentTime = 0.2;
> +    SimpleTest.finish();

I think you need to finish the test from the seeked callback, not here. Otherwise the runner will race against completion of the seek.

::: dom/media/tests/mochitest/TextTrack_active_cues_null.html
@@ +20,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +  SpecialPowers.setBoolPref("media.webvtt.enabled", true);
> +  SimpleTest.waitForExplicitFinish();
> +  

Please remove the trailing whitespace.

::: dom/media/tests/mochitest/TextTrack_addCue.html
@@ +18,5 @@
> +  </video>
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +  SpecialPowers.setBoolPref("media.webvtt.enabled", true);

Same here. Do the test only if media.webvtt.enabled is already true. Then add two more clauses: flip the pref to true, create and append an new element and check the tests; flip the pref to false, create and append a new track element and confirm it doesn't do anything.

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 713711 [details] [diff] [review]
Finished HTMLTrackElement tests

Removing obsolete review request; we've covered this on IRC and github, I think.

Comment 17

[Marcus Saad (:msaad)]

Attachment: TextTrackList tests

This new patch includes tests for the TextTrackList class. When media.webvtt.enabled is not defined, I'm asserting in the tests that the values should be undefined. Is there anything wrong with that?

Comment 18

[Ralph Giles (:rillian)]

Comment on attachment 737666 [details] [diff] [review]
TextTrackList tests

Review of attachment 737666 [details] [diff] [review]:
-----------------------------------------------------------------

Getting closer. See the comments inline. Also, can you combine these three test files? just add the length checks before and after calling addTextTrack in the first one.

::: dom/media/tests/mochitest/test_TextTrackList_getter.html
@@ +44,5 @@
> +			videoElement.src = "/tests/content/media/test/seek.webm";
> +			videoDiv.appendChild(videoElement);
> +			var TextTrackList = videoElement.textTracks;
> +			videoElement.addTextTrack("subtitles", "label", "en-CA");
> +		   	is(TextTrackList[0].kind, "subtitles", "TextTrackList[0].kind |Pref => False");

Does this pass with the current code? I would expect addTextTrack to fail at TextTrackList to be undefined if the pref is false.

[16:48:31.083] var v = document.createElement('video');
[16:48:31.086] undefined
--
[16:48:46.479] document.body.appendChild(v);
[16:48:46.481] [object HTMLVideoElement]
--
[16:48:54.534] v.controls = true;
[16:48:54.537] true
--
[16:49:47.062] var t = v.addTextTrack("subtitles", "label", "en-CA");
[16:49:47.064] TypeError: v.addTextTrack is not a function
--
[16:49:54.363] t
[16:49:54.365] undefined
--
[16:50:31.892] var TextTrackList = v.textTracks;
[16:50:31.893] undefined
--
[16:50:40.580] TextTrackList[0].kind
[16:50:40.582] TypeError: TextTrackList is undefined
--
[16:50:50.485] TextTrackList[0].kind == undefined
[16:50:50.487] TypeError: TextTrackList is undefined

@@ +49,5 @@
> +			is(TextTrackList[0].label, "label", "TextTrackList[0].label |Pref => False");
> +			is(TextTrackList[0].language, "en-CA", "TextTrackList[0].language |Pref => False");
> +			is(TextTrackList[0].mode, "hidden", "TextTrackList[0].mode |Pref => False");
> +			SimpleTest.finish();
> +		 }else{

This is far enough from the original clause it could use a comment. something like:

}else{
  // Testing with the pref unavailable.

@@ +62,5 @@
> +			is(TextTrackList[0].mode, undefined, "TextTrackList[0].mode |Pref => Undefined");
> +			SimpleTest.finish();
> +		}
> +	</script>
> +	</pre>	

Please remove trailing whitespace.

Comment 19

[raffoul.jordan]

Attachment: TextTrack tests

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 739323 [details] [diff] [review]
TextTrack tests

Review of attachment 739323 [details] [diff] [review]:
-----------------------------------------------------------------

Some nice test ideas. See comments inline.

I think you can combine the kind, label, and language test files.

Please put a charset declaration in the header of each html file to quiet the console warning about interpreting as ASCII.
<meta charset='utf-8'>

These should all move to content/media/test/ to be with the other media test files.

More generally, it's helpful when updating a patch to give a summary of what's changed and refer to any earlier review comments. Harder this time, I know, since I didn't record them in the bug, but it helps get the reviewer sync'd with what you've done.

Please resubmit after addressing these comments.

::: dom/media/tests/mochitest/TextTrack_active_cues.html
@@ +25,5 @@
> +  var video = document.getElementById('video_1');
> +  var trackElement = document.createElement('track');
> +  trackElement.id = 'track_1';
> +  trackElement.kind = 'subtitles';
> +  trackElement.src = 'overlap.vtt';

This file is a nice idea!

@@ +35,5 @@
> +
> +    // active cues 0
> +    video.addEventListener('seeked', seeked(0));
> +    video.currentTime = 2.0;
> +    video.removeEventListener('seeked', seeked);

You can't and and remove an event listener like this. The browser js runtime is single-threaded, so seeked will never be called until after all of start() has finished running. Instead you need to chain the tests, invoking the next one from inside the seeked() call from the previous one.

::: dom/media/tests/mochitest/TextTrack_addCue.html
@@ +20,5 @@
> +<script class="testbody" type="text/javascript">
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.setBoolPref("media.webvtt.enabled", true);
> +
> +  var video = document.getElementById('video_1');

You could make this shorter by dropping the id on the <video> element and just using document.querySelector('video').

If there's more than one you do:
var videos = document.querySelectorAll('video');
video[0].appendChild(track);
video[1].appendChild(track);
etc.

@@ +31,5 @@
> +
> +  function start() {
> +    var textTrack = trackElement.track;
> +
> +    is(textTrack.cues.length, 3, 'initial cue count');

The initial vtt file is loaded asynchronously. You want to do this and the remaining checks in a callback after loadedmedatadata is dispatched by the video element.

@@ +36,5 @@
> +
> +    cue = new TextTrackCue(10, 15, "foo");
> +
> +    textTrack.addCue(cue);
> +    is(textTrack.cues.length, 4, 'add cue - increase cue count by 1');

It would be clever here to check that length is really the previous length+1. As it is, if the first fails (because fast.vtt has changed, or something) then every test necessarily fails, which doesn't give any extra information.

::: dom/media/tests/mochitest/TextTrack_cues.html
@@ +30,5 @@
> +  video.appendChild(trackElement);
> +
> +  function start() {
> +    var textTrack = trackElement.track;
> +    is(textTrack.cues.length, 3, 'total cues');

This test is redundant with the addCue one.

@@ +33,5 @@
> +    var textTrack = trackElement.track;
> +    is(textTrack.cues.length, 3, 'total cues');
> +    SimpleTest.finish();
> +  }
> +  addLoadEvent(start);

As before, you need to call your test from a loadedmetadata event callback on the video element.

::: dom/media/tests/mochitest/TextTrack_language.html
@@ +27,5 @@
> +  trackElement.id = 'track_1';
> +  trackElement.src = 'fast.vtt';
> +
> +  video.appendChild(trackElement);
> +  

Please remove trailing whitespace.

Comment 21

[Marcus Saad (:msaad)]

Attachment: TextTrackList tests v2

On this patch, I've combined all the tests into a single file.

-When pref is set to false, HTMLMediaElement shouldn't have a TextTrackList reference. That made me change the assertions to undefined.

-Cleaned trailing white space

-Fixed previous comments

They should be good to be pushed upstream.

Comment 22

[Marcus Saad (:msaad)]

Attachment: TextTrackCue Tests

-Added ID to one of the cues inside basic.vtt in order to test TextTrackCue.id
-Removed new line at the end of the file (Bug related to not returning the last cue if there isn't a new line at the end).
-New tests for TextTrackCue

Comment 23

[Rick Eyre (:reyre)]

Are you still working on this Marcus or Jordan? If not I'll take and start work on it now.

Comment 24

[Caitlin Potter (:caitp)]

Attachment: TextTrackList tests v3

I've done a bit of work to bring these more in line with the suggestions from :ted and others (Asynchronous pref-setting, basically). I've also removed tests on TextTrack itself, as given the name of the test, this should be restricted to the TextTrackList DOM interface. However work on the TextTrack interface is coming shortly

Comment 25

[Rick Eyre (:reyre)]

Attachment: Current WIP - still needs more work

Comment 26

[Rick Eyre (:reyre)]

Attachment: v1:Initial tests for HTMLTrackElement and TextTrack* DOM classes

- Consolidates Jordan's and Marcus's tests
- Incorporated a few of the checks in the TextTrackList tests that Caitlin was doing as well

Comment 27

[Caitlin Potter (:caitp)]

Not cool.

Comment 28

[Rick Eyre (:reyre)]

Sorry about that Caitlin, but you were fully aware that I had been working on this bug for the last few days. You've basically duplicated a small subset of the work that I've done with my patch. I did, however, incorporate a few of the text track list checks that you had in your patch that weren't in mine.

One of the main goals that Ralph and I discussed for landing this patch was consolidating as many of the tests that Marcus and Jordan made into the same test files. So we want less test files not more.

That being said, feel free to mark your patch as un-obsolete and see what we can do.

Comment 29

[Caitlin Potter (:caitp)]

Actually you never really discuss the work you're doing, all anybody ever sees from you is "Great, thanks! :):):)" and it's a complete mystery what you're actually doing! I don't think it's very good to even fully replace Marcus's stuff, he's back in Toronto and should be getting this stuff landed if he's up for it. I'm done with this bug because it's pretty impossible to work as a team that does not coordinate in any way whatsoever.

Comment 30

[Rick Eyre (:reyre)]

Marcus and I talked and agreed that I would continue this bug because he was away on vacation. That's why I assigned the bug to myself and why you have been seeing me ask a lot of JS Mochitest questions on IRC -- where you have been actively participating in those discussions.

Comment 31

[Caitlin Potter (:caitp)]

Sorry but no, there has really been essentially no coordination here, even when asked specifically which of these you're touching and avoiding the HTMLTrackElement stuff which you have actively been commenting on. There has never been any evidence that you were doing the entire collection of dependencies, and you've even told me to work on some. So you're basically telling me to waste my time that could be spent doing more valuable things like fixing problems in WebKit and Chromium. Not cool at all.

And further, `hg log -u` is a CV, it's really not cool to take credit for other peoples work. Particularly for students. That is not appropriate whatsoever, in any universe.

Comment 32

[Rick Eyre (:reyre)]

I suggested to you to work on the bugs that are blocking this one, not on this one itself. Might have been a communication error there.

Comment 33

[Caitlin Potter (:caitp)]

I have been doing that -- But it's pointless now, this whole **** thing is a mess because of the lack of coordination and communication. This is not how you collaborate, this is how you waste other peoples time.

Comment 34

[Ralph Giles (:rillian)]

Let's keep the bug comments civil, please.

Rick and I talked about him consolidating msaad and jbraffoul's work at the end of the rendering meeting in Taipei, at least until Marcus was available again. Sorry Caitlin if I didn't communicate that clearly. This is the next priority after the loadlistener patch, so I wanted to move it forward.

Rick credited the original patch authors in his commit message, which is appropriate.

Comment 35

[Rick Eyre (:reyre)]

I forgot to mention, but for some of these tests to run correctly you will need to apply the patches in bug 833382 and bug 875169. Sorry about that.

Comment 36

[Ralph Giles (:rillian)]

Comment on attachment 758103 [details] [diff] [review]
v1:Initial tests for HTMLTrackElement and TextTrack* DOM classes

Review of attachment 758103 [details] [diff] [review]:
-----------------------------------------------------------------

The general set of tests look good. Please address the comments below and ask for review again. Also passing to cpearce who wanted a chance to comment.

I get 'TEST-PASS | unknown test url' running this with 'mach mochitest-plain /content/html/content/test/...' Is it possible to set the url to something meaningful?

Please rename test_htmltrackelement_attributes_reflection.html to test_track_attributes_reflection.html and test_htmltrackelement_disabled.html to test_track_disabled.html. This is shorter and matches local style.

test_texttracklist.html is quite short. I think you could reasonable move those checks into test_texttrack.html and save the overhead of another file.

As a general style thing, you might consider defining the tests in their own function and than pass the function name to SpecialPowers.pushPrefEnv(). That way when (if?) we eventually remove the pref, we won't have to touch every line of the test itself to re-indent the code.

::: content/html/content/test/test_bug389797.html
@@ +217,4 @@
>  HTML_TAG("time", "Time");
>  HTML_TAG("title", "Title");
>  HTML_TAG("tr", "TableRow");
> +HTML_TAG("track", "Track");

This needs a different reviewer, maybe bz. Best to split this into a separate patch.

::: content/html/content/test/test_htmltrackelement_attributes_reflection.html
@@ +19,5 @@
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},
> +  function() {
> +    /* TODO:: See https://bugzilla.mozilla.org/show_bug.cgi?id=871184 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=22221

871184 is resolved, so this should work now, right?

If not, this should be updated to refer to bug 880064.

::: content/html/content/test/test_htmltrackelement_disabled.html
@@ +7,5 @@
> +  <meta charset='utf-8'>
> +  <title>Test for Bug 833386 - HTMLTrackElement</title>
> +  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/content/html/content/test/reflect.js"></script>

You don't need to include reflect here.

::: content/media/test/test_texttrack.html
@@ +7,5 @@
> +  <meta charset='utf-8'>
> +  <title>Test for Bug 833386 - TextTrackList</title>
> +  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/content/html/content/test/reflect.js"></script>

Don't need reflect.

@@ +20,5 @@
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},
> +  function() {
> +    var video = document.createElement("video");
> +    video.src = "./seek.webm";

I'd lose the './' it doesn't add anything.

@@ +41,5 @@
> +
> +    // Play/pause the video - mode should now be set to "showing"
> +    video.play();
> +    video.pause();
> +    todo_is(textTrack.mode, 'showing', "Mode should now be showing.");

Please add a comment with a bug number for each todo_.

Can you point me at where in the spec calling play() is supposed to switch a track from hidden to showing? I can't find this. The closest is 'steps to perform automatic text track selection' which will switch the first disabled track with the default attribute set to showing. http://www.whatwg.org/specs/web-apps/current-work/#sourcing-out-of-band-text-tracks

I'd also be surprised if this works synchronously. Does the spec say this is enough, or can the user-agent wait for stable state? If not, move this to the end and put the is() check and finish() in a callback on the video 'pause' or 'playing' event.

::: content/media/test/test_texttrackcue.html
@@ +7,5 @@
> +  <meta charset='utf-8'>
> +  <title>Test for Bug 833386 - HTMLTrackElement</title>
> +  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/content/html/content/test/reflect.js"></script>

Don't need reflect.

@@ +20,5 @@
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},
> +  function() {
> +    var video = document.createElement("video");
> +    video.src = "./seek.webm";

This doesn't affect the test, but seek.webm is 4 seconds long and basic.vtt's first cue starts at 11 seconds, so none of these will actually display. It would be better to shift cue timeline so there's some visual feedback when the test is running.

@@ +22,5 @@
> +  function() {
> +    var video = document.createElement("video");
> +    video.src = "./seek.webm";
> +    var trackElement = document.createElement("track");
> +    trackElement.src = "./basic.vtt";

I would drop the './' prefixes here too. Just "seek.webm" and "basic.vtt" are already relative paths.

@@ +77,5 @@
> +        try {
> +          cue = new TextTrackCue(1, 2, "foo");
> +          trackElement.removeCue(cue);
> +        } catch (e) {
> +          // How can we test this if exception wasn't thrown.. ?

Set a variable in the exception handler and check it later.

::: content/media/test/test_texttracklist.html
@@ +7,5 @@
> +  <meta charset='utf-8'>
> +  <title>Test for Bug 833386 - TextTrackList</title>
> +  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/content/html/content/test/reflect.js"></script>

Don't need reflect here.

::: content/media/test/test_webvtt_disabled.html
@@ +7,5 @@
> +  <meta charset='utf-8'>
> +  <title>Test for Bug 833386 - TextTrackList Pref Off</title>
> +  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/content/html/content/test/reflect.js"></script>

You don't need to include reflect.js here.

Comment 37

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #36)
> Comment on attachment 758103 [details] [diff] [review]

> As a general style thing, you might consider defining the tests in their own
> function and than pass the function name to SpecialPowers.pushPrefEnv().
> That way when (if?) we eventually remove the pref, we won't have to touch
> every line of the test itself to re-indent the code.

We'll still have to delete the function and outdent the code in the function -- or are you thinking that we will just replace pushPrefEnv() with a direct call to that function?

Comment 38

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #36)
> Can you point me at where in the spec calling play() is supposed to switch a
> track from hidden to showing? I can't find this. The closest is 'steps to
> perform automatic text track selection' which will switch the first disabled
> track with the default attribute set to showing.
> http://www.whatwg.org/specs/web-apps/current-work/#sourcing-out-of-band-text-
> tracks

This was taken directly from Jordan's code so it might be against an older spec. I guess we can remove from now as I can't find a place either that talks about doing this as well.

Comment 39

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #36)
> ::: content/html/content/test/test_bug389797.html
> @@ +217,4 @@
> >  HTML_TAG("time", "Time");
> >  HTML_TAG("title", "Title");
> >  HTML_TAG("tr", "TableRow");
> > +HTML_TAG("track", "Track");

Bz's recommendation is to not do this test until we remove the pref as it will be to complicated and unnecessary at this point. I'll add bugs to track the eventual pref removal (?) and addition of this test.

Comment 40

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #36)
> I get 'TEST-PASS | unknown test url' running this with 'mach mochitest-plain
> /content/html/content/test/...' Is it possible to set the url to something
> meaningful?

So apparently this just happens when you run one test. When you run multiple of them the test URL will be shown. I've asked around and looked and AFAIK you can't set it.

Comment 41

[Ralph Giles (:rillian)]

(In reply to Rick Eyre (:reyre) from comment #37)

> We'll still have to delete the function and outdent the code in the function
> -- or are you thinking that we will just replace pushPrefEnv() with a direct
> call to that function?

That's what I was suggesting, yes. If you ultimately want to remove the function too, it doesn't help. I don't mind either way, I just wanted to point out the planning opportunity.

Comment 42

[Rick Eyre (:reyre)]

Attachment: v2: Tests for Track element and TextTrack* DOM classes

Haven't updated to the style that Ralph suggested yet, with tests in function and passing that to pushPrefEnv, as am waiting for Ralph's response to my previous comment. 

I'll update that if necessary.

Comment 43

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #41)
> (In reply to Rick Eyre (:reyre) from comment #37)
> 
> > We'll still have to delete the function and outdent the code in the function
> > -- or are you thinking that we will just replace pushPrefEnv() with a direct
> > call to that function?
> 
> That's what I was suggesting, yes. If you ultimately want to remove the
> function too, it doesn't help. I don't mind either way, I just wanted to
> point out the planning opportunity.

Most simple mochitests I've viewed don't separate out the checks into a separate functions so I think it might be worth it to keep with that style.

Comment 44

[Ralph Giles (:rillian)]

Comment on attachment 759838 [details] [diff] [review]
v2: Tests for Track element and TextTrack* DOM classes

Review of attachment 759838 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good for me, except for the invalid timestamps in basic.vtt. Does that mean we have a parser bug if the test is passing with these? r+ with that issue addressed.

Marcus has a lot of different spellings for his name. Have you checked with him if that's what he'd like for credit?

::: content/media/test/Makefile.in
@@ +138,5 @@
>  		test_streams_tracks.html \
>  		$(filter disabled-for-intermittent-failures--bug-608634, test_error_in_video_document.html) \
>  		test_timeupdate_small_files.html \
> +		test_texttrackcue.html \
> +		test_texttrack.html \

texttrack sorted before texttrackcue, please.

::: content/media/test/basic.vtt
@@ +1,4 @@
> +WEBVTT
> +
> +1
> +00:1.000 --> 00:2.000

seconds field must be two digits. 00:01.000 --> 00:02.000.

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 759838 [details] [diff] [review]
v2: Tests for Track element and TextTrack* DOM classes

Review of attachment 759838 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_texttrackcue.html
@@ +33,5 @@
> +
> +        // Check if first cue was parsed correctly.
> +        var cue = cueList[0];
> +        is(cue.id, "1", "Cue's ID should be 1.");
> +        is(cue.startTime, 1, "Cue's start time should be 11.");

"Cue's start time should be 1."  ?? Your is(cue.endTime...) statement below also seem to be off by 10?

Comment 46

[Marcus Saad (:msaad)]

(In reply to Rick Eyre (:reyre) from comment #30)
> Marcus and I talked and agreed that I would continue this bug because he was
> away on vacation.

+1 on this. We've been in touch, every change that rick did to this bug has been approved by me, he took the time to get in touch, check how things were and we agreed to move this forward. 

(In reply to Ralph Giles (:rillian) from comment #44)
> Comment on attachment 759838 [details] [diff] [review]

> Marcus has a lot of different spellings for his name. Have you checked with
> him if that's what he'd like for credit?

Thanks for pointing that out rillian, rick almost did it properly hehehe. It's Marcus Saad (msaad). No worries about that at all, that won't change anything. The sole purpose of this is to contribute to a better web.



(In reply to Caitlin Potter (:caitp) from comment #29)
> Actually you never really discuss the work you're doing, all anybody ever
> sees from you is "Great, thanks! :):):)" and it's a complete mystery what
> you're actually doing! I don't think it's very good to even fully replace
> Marcus's stuff, he's back in Toronto and should be getting this stuff landed
> if he's up for it. I'm done with this bug because it's pretty impossible to
> work as a team that does not coordinate in any way whatsoever.

Caitp, I appreciate your concern about this. I really do. I'm sad to hear things happened in such a disorganized way, but  we should not let this moves us apart, we still have lots to do and I'm sure we will do.

Comment 47

[Rick Eyre (:reyre)]

(In reply to marcus saad from comment #46)
> Thanks for pointing that out rillian, rick almost did it properly hehehe.
> It's Marcus Saad (msaad).

Sorry about that Marcus :). I'll update your name in the commit message now.

Comment 48

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #44)
> Looks good for me, except for the invalid timestamps in basic.vtt. Does that
> mean we have a parser bug if the test is passing with these? r+ with that
> issue addressed.

Do you mean r+ with the bug in the parser addressed or the malformed time stamps in basic.vtt?

Comment 49

[Ralph Giles (:rillian)]

R+ from me with the invalid timestamps fixed and the question answered. If the parser isn't buggy, that's good. If it is, opening a bug against it is also an answer. :-)

Comment 50

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #49)
> R+ from me with the invalid timestamps fixed and the question answered. If
> the parser isn't buggy, that's good. If it is, opening a bug against it is
> also an answer. :-)

Seems to be incorrect and have logged issued at https://github.com/mozilla/webvtt/issues/409

Comment 51

[Rick Eyre (:reyre)]

Attachment: v3: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Comment 52

[Rick Eyre (:reyre)]

https://tbpl.mozilla.org/?tree=Try&rev=7b5f717c894f

Comment 53

[Caitlin Potter (:caitp)]

> Seems to be incorrect and have logged issued at
> https://github.com/mozilla/webvtt/issues/409

Just to be clear, this is PEBKAC and is addressed in #881475

Comment 54

[Rick Eyre (:reyre)]

Attachment: v4: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

- Added a manual test for HTMLTrackElement.readyState since we can't test that using reflect.js
- Added a manual test for HTMLTrackElement.kind until bug 880064 is sorted out.
- Renamed test_track_attributes_reflection.html to test_track.html as we are doing more then just reflect.js stuff in it now.

Comment 55

[Rick Eyre (:reyre)]

https://tbpl.mozilla.org/?tree=Try&rev=386329fcf6c3

Comment 56

[Ralph Giles (:rillian)]

Comment on attachment 761136 [details] [diff] [review]
v4: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Review of attachment 761136 [details] [diff] [review]:
-----------------------------------------------------------------

The new tests look good, thanks. Two cosmetic comments below.

What's up with the try push? I thought these tests would be passing now that 833382 has landed? r- until we figure that out.

::: content/html/content/test/test_track.html
@@ +58,5 @@
> +    // Following are manual track.kind tests until the reflect.js problems get
> +    // cleared up above.
> +    // See: https://bugzilla.mozilla.org/show_bug.cgi?id=880064 and
> +    //      https://www.w3.org/Bugs/Public/show_bug.cgi?id=22221
> +    

trailing whitespace.

@@ +70,5 @@
> +    setKind("descriptions", "Kind should be set to descriptions.");
> +    setKind("chapters", "Kind should be set to chapters.");
> +    setKind("metadata", "Kind should be set to metadata.");
> +
> +    function setKind(kind, message) {

Might be more clear to call this 'checkKind()' since it checks the result of the set.

Comment 57

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #56)
> What's up with the try push? I thought these tests would be passing now that
> 833382 has landed? r- until we figure that out.

I've been trying to figure it out. It tests fine locally on my linux and mac boxes. I'm thinking it has to do with the .vtt not being found or the pref not being set correctly. I really have no idea atm though.

Comment 58

[Rick Eyre (:reyre)]

So it works when you run the test directly, but if you run it with the suite of tests it fails.

Comment 59

[Rick Eyre (:reyre)]

Attachment: v5: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Updated to fix nits and to use HTMLTrackElement::ReadyState to detect when the cues are actually loaded.

See bug 882535 and 882131

Comment 60

[Rick Eyre (:reyre)]

Attachment: v5: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Forgot to rebase. Realized this when I tried to apply my patch and push to try server and got merge errors.

Comment 61

[Ralph Giles (:rillian)]

Comment on attachment 762047 [details] [diff] [review]
v5: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Review of attachment 762047 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look good to me. r+ if it passes on try.

::: content/media/test/test_texttrack.html
@@ +55,5 @@
> +
> +    textTrack.kind = "captions";
> +    is(textTrack.kind, "subtitles", "Kind is read-only so should still be \"subtitles\"");
> +
> +    function setMode(value, message) {

this should probably be checkMode too.

::: content/media/test/test_texttrackcue.html
@@ +27,5 @@
> +    document.getElementById("content").appendChild(video);
> +    video.appendChild(trackElement);
> +    video.addEventListener("loadedmetadata",
> +      function run_tests() {
> +        if (trackElement.readyState == 1) {

Maybe a comment here that we're re-queuing ourselves at the end of the event loop until the track reaches loaded.

Comment 62

[Rick Eyre (:reyre)]

https://tbpl.mozilla.org/?tree=Try&rev=9f09df497708

The tests don't show up in the changesets shown, but you can see the tests being run if you go a search for them in the log file.

Comment 63

[Rick Eyre (:reyre)]

Attachment: v6: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Carrying forward r=rillian

Comment 64

[Rick Eyre (:reyre)]

Attachment: v7: Tests for Track element and TextTrack* DOM classes r=rillian,cpearce

Had to rebase on top of the current version in m-c.

Comment 65

[Rick Eyre (:reyre)]

https://tbpl.mozilla.org/?tree=Try&rev=f00aca510bb3

Comment 66

[Rick Eyre (:reyre)]

Attachment: Part 1 v8: Tests for TrackElement and TextTrack(List)

This patch splits the TestTrackCue tests out so we can land this separately while we wait for the Android bug to be fixed.

Comment 67

[Rick Eyre (:reyre)]

Attachment: Part 2 v8: Tests for TextTrackCue objects

Second part to be landed after Android bug is fixed.

Please note this patch includes tests for bug 884879.

Comment 68

[Ralph Giles (:rillian)]

Comment on attachment 765349 [details] [diff] [review]
Part 1 v8: Tests for TrackElement and TextTrack(List)

Review of attachment 765349 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. If this set passes try, let's land them!

You can put [leave open] in the whiteboard along with checkin-needed in keywords to tell the committer there are more patches still to come.

Comment 69

[Ralph Giles (:rillian)]

I rebased part 1 and pushed to try. https://tbpl.mozilla.org/?tree=Try&rev=a75e90318720

Comment 70

[Rick Eyre (:reyre)]

Awesome. Thanks Ralph!

Comment 71

[Rick Eyre (:reyre)]

Looks green. Marking as checkin-needed now.

Comment 72

[Rick Eyre (:reyre)]

Please check in only part 1.

Comment 73

[Ralph Giles (:rillian)]

Attachment: Part 1 v8: Tests for TrackElement and TextTrack(List)

Rebase against m-c. Carrying forward r=me.

Comment 74

[Ralph Giles (:rillian)]

Comment on attachment 765978 [details] [diff] [review]
Part 1 v8: Tests for TrackElement and TextTrack(List)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa6deebc1be

Comment 75

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3aa6deebc1be

Comment 76

[Rick Eyre (:reyre)]

Attachment: Part 2 v9: Tests for TextTrackCue objects

For some reason "loadedmetadata" isn't being called on Android in the test set up we have. See bug 884884. 

I've just taken out the listener for "loadedmetadata" since we can now just rely on the TrackElement's ReadyState. 

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6c0dd7bed40e&pusher=[e-mail]

Comment 77

[Ralph Giles (:rillian)]

Comment on attachment 767015 [details] [diff] [review]
Part 2 v9: Tests for TextTrackCue objects

Review of attachment 767015 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for banging this into shape.

Comment 78

[Rick Eyre (:reyre)]

No problem ralph. Try push here:

https://tbpl.mozilla.org/?tree=Try&rev=124e697e804e

The changes don't show up as I pushed them earlier, but it should be in the test results.

Comment 79

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/068cba1e5d11

Comment 80

[Rick Eyre (:reyre)]

Awesome, thanks Ralph.

Comment 81

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/068cba1e5d11