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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: msaad, Assigned: reyre)

Tracking

(Blocks 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 21 obsolete attachments)

10.36 KB, patch
rillian
: review+
rillian
: checkin+
Details | Diff | Splinter Review
6.91 KB, patch
rillian
: review+
rillian
: checkin+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17
This is a spin-off from bug 629350 to do the tests necessary for the spec.
Assignee: nobody → mv.nsaad
Blocks: webvtt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Jordan and Marcus are going to work on this together.
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)
(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
Ok, thanks for the update. I look forward to seeing the first code.
Posted patch WIP HTMLTextTrack mochitests (obsolete) — Splinter Review
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.
Attachment #709970 - Flags: feedback?
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.
Attachment #709970 - Flags: feedback? → feedback+
(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.
(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.
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.
Attachment #709970 - Attachment is obsolete: true
Attachment #713711 - Flags: review?
I realized that the latest patch doesn't include tests for kind. I'll will be added.
Sorry for that.
Attachment #713711 - Flags: review? → review?(giles)
Posted patch WIP TextTrack Tests (obsolete) — Splinter Review
Attachment #709516 - Attachment is obsolete: true
Attachment #732603 - Flags: review?(rillian)
Posted patch TextTrack tests (obsolete) — Splinter Review
Attachment #732603 - Attachment is obsolete: true
Attachment #732603 - Flags: review?(rillian)
Attachment #732606 - Flags: review?(giles)
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.
Attachment #732606 - Flags: review?(giles) → review-
Comment on attachment 713711 [details] [diff] [review]
Finished HTMLTrackElement tests

Removing obsolete review request; we've covered this on IRC and github, I think.
Attachment #713711 - Flags: review?(giles)
Posted patch TextTrackList tests (obsolete) — Splinter Review
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?
Attachment #737666 - Flags: review?(giles)
Attachment #737666 - Attachment is patch: true
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.
Attachment #737666 - Flags: review?(giles) → review-
Posted patch TextTrack tests (obsolete) — Splinter Review
Attachment #732606 - Attachment is obsolete: true
Attachment #739323 - Flags: review?(giles)
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.
Attachment #739323 - Flags: review?(giles) → review-
Posted patch TextTrackList tests v2 (obsolete) — Splinter Review
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.
Attachment #737666 - Attachment is obsolete: true
Attachment #740343 - Flags: review?(giles)
Posted patch TextTrackCue Tests (obsolete) — Splinter Review
-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
Attachment #743137 - Flags: review?(giles)
Attachment #743137 - Attachment is patch: true
Are you still working on this Marcus or Jordan? If not I'll take and start work on it now.
Depends on: 871178
Depends on: 871188
Depends on: 871184
Assignee: mv.nsaad → rick.eyre
Depends on: 833382
Posted patch TextTrackList tests v3 (obsolete) — Splinter Review
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
Attachment #740343 - Attachment is obsolete: true
Attachment #740343 - Flags: review?(giles)
Attachment #757782 - Flags: review?(giles)
Attachment #713711 - Attachment is obsolete: true
Attachment #739323 - Attachment is obsolete: true
Attachment #743137 - Attachment is obsolete: true
Attachment #743137 - Flags: review?(giles)
Attachment #757968 - Flags: feedback?(giles)
- Consolidates Jordan's and Marcus's tests
- Incorporated a few of the checks in the TextTrackList tests that Caitlin was doing as well
Attachment #757782 - Attachment is obsolete: true
Attachment #757968 - Attachment is obsolete: true
Attachment #757782 - Flags: review?(giles)
Attachment #757968 - Flags: feedback?(giles)
Attachment #758103 - Flags: review?(giles)
Not cool.
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.
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.
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.
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.
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.
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.
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.
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.
Depends on: 875169
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.
Attachment #758103 - Flags: review?(giles)
Attachment #758103 - Flags: review?(cpearce)
Attachment #758103 - Flags: review+
(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?
(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.
(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.
(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.
(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.
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.
Attachment #758103 - Attachment is obsolete: true
Attachment #758103 - Flags: review?(cpearce)
Attachment #759838 - Flags: review?(giles)
Attachment #759838 - Flags: review?(cpearce)
(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 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 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?
Attachment #759838 - Flags: review?(cpearce) → review+

(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.
(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.
(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?
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. :-)
(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
Attachment #759838 - Attachment is obsolete: true
Attachment #759838 - Flags: review?(giles)
> 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
- 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.
Attachment #760435 - Attachment is obsolete: true
Attachment #761136 - Flags: review?(giles)
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.
Attachment #761136 - Flags: review?(giles) → review-
(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.
So it works when you run the test directly, but if you run it with the suite of tests it fails.
Depends on: 882535
Updated to fix nits and to use HTMLTrackElement::ReadyState to detect when the cues are actually loaded.

See bug 882535 and 882131
Attachment #761136 - Attachment is obsolete: true
Attachment #762015 - Flags: review?(giles)
Forgot to rebase. Realized this when I tried to apply my patch and push to try server and got merge errors.
Attachment #762015 - Attachment is obsolete: true
Attachment #762015 - Flags: review?(giles)
Attachment #762047 - Flags: review?(giles)
Depends on: 882817
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.
Attachment #762047 - Flags: review?(giles) → review+
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.
Carrying forward r=rillian
Attachment #762047 - Attachment is obsolete: true
Had to rebase on top of the current version in m-c.
Attachment #762463 - Attachment is obsolete: true
Depends on: 883173
Blocks: 884879
Depends on: 884884
No longer blocks: 884879
No longer depends on: 883173
This patch splits the TestTrackCue tests out so we can land this separately while we wait for the Android bug to be fixed.
Attachment #763542 - Attachment is obsolete: true
Attachment #765349 - Flags: review?(giles)
Second part to be landed after Android bug is fixed.

Please note this patch includes tests for bug 884879.
Attachment #765369 - Flags: review?(giles)
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.
Attachment #765349 - Flags: review?(giles) → review+
Awesome. Thanks Ralph!
Looks green. Marking as checkin-needed now.
Keywords: checkin-needed
Whiteboard: [leave open]
Please check in only part 1.
Rebase against m-c. Carrying forward r=me.
Attachment #765349 - Attachment is obsolete: true
Attachment #765978 - Flags: review+
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=rick.eyre@hotmail.com
Attachment #765369 - Attachment is obsolete: true
Attachment #765369 - Flags: review?(giles)
Attachment #767015 - Flags: review?(giles)
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.
Attachment #767015 - Flags: review?(giles) → review+
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.
Awesome, thanks Ralph.
https://hg.mozilla.org/mozilla-central/rev/068cba1e5d11
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.