Convert track-webvtt-tc[000-002] tests to testharness.js

Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

BUG=588956
Committed: https://crrev.com/483b390585dcf834ffdba1013f80cc00ccdfc12f
Cr-Commit-Position: refs/heads/master@{#394771}

[Srirama, 2016-05-08 14:20:53]

Description was changed from

==========
fix

BUG=
==========

to

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track to use testharness.js
instead of
video-test.js. This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

[Srirama, 2016-05-08 15:44:19]

Description was changed from

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track to use testharness.js
instead of
video-test.js. This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

to

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

[Srirama, 2016-05-08 15:45:27]

srirama.m@samsung.com changed reviewers:
+ mlamouri@chromium.org

[Srirama, 2016-05-08 15:45:28]

PTAL

[Srirama, 2016-05-09 07:45:21]

Patchset #1 (id:1) has been deleted

[mlamouri (slow - plz ping), 2016-05-10 09:14:39]

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
(right):

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
assert_equals(cues[i].text, textValues[i]);
I think it would make sense to have a helper file that implement something like
testCues() so you don't have to re-implement it dozens of time.

What about creating a track-helpers.js and add a `assert_cues_equal()`?

[Srirama, 2016-05-10 09:32:50]

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
(right):

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
assert_equals(cues[i].text, textValues[i]);
On 2016/05/10 09:14:39, Mounir Lamouri wrote:
> I think it would make sense to have a helper file that implement something
like
> testCues() so you don't have to re-implement it dozens of time.
> 
> What about creating a track-helpers.js and add a `assert_cues_equal()`?

Agreed, makes sense. Even enableAllTextTracks() is done in some of the test
cases. This as well we can move it to helper.

[Srirama, 2016-05-10 13:04:44]

On 2016/05/10 09:32:50, Srirama wrote:
>
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
> (right):
> 
>
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
> assert_equals(cues[i].text, textValues[i]);
> On 2016/05/10 09:14:39, Mounir Lamouri wrote:
> > I think it would make sense to have a helper file that implement something
> like
> > testCues() so you don't have to re-implement it dozens of time.
> > 
> > What about creating a track-helpers.js and add a `assert_cues_equal()`?
> 
> Agreed, makes sense. Even enableAllTextTracks() is done in some of the test
> cases. This as well we can move it to helper.

Have a doubt, right now we are trying to get rid of video-test.js to be able to
upstream these tests to web-platform-tests. So is it ok to have a new utility
file. If so should it be in LayoutTests/resources folder? Or should we just add
these utility functions to testharness.js?

[mlamouri (slow - plz ping), 2016-05-11 09:50:44]

On 2016/05/10 at 13:04:44, srirama.m wrote:
> On 2016/05/10 09:32:50, Srirama wrote:
> >
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
> > (right):
> > 
> >
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
> > assert_equals(cues[i].text, textValues[i]);
> > On 2016/05/10 09:14:39, Mounir Lamouri wrote:
> > > I think it would make sense to have a helper file that implement something
> > like
> > > testCues() so you don't have to re-implement it dozens of time.
> > > 
> > > What about creating a track-helpers.js and add a `assert_cues_equal()`?
> > 
> > Agreed, makes sense. Even enableAllTextTracks() is done in some of the test
> > cases. This as well we can move it to helper.
> 
> Have a doubt, right now we are trying to get rid of video-test.js to be able
to upstream these tests to web-platform-tests. So is it ok to have a new utility
file. If so should it be in LayoutTests/resources folder? Or should we just add
these utility functions to testharness.js?

The issue with video-test.js isn't that it is an utility file but that it is
assuming we are using Chromium's TestRunner. I think it's fine to have utility
files for some parts of HTML if it helps make testing easier. There is no reason
why we couldn't upstream to W3C the test with the utility.

[Srirama, 2016-05-11 10:18:21]

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
(right):

https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
assert_equals(cues[i].text, textValues[i]);
On 2016/05/10 09:32:50, Srirama wrote:
> On 2016/05/10 09:14:39, Mounir Lamouri wrote:
> > I think it would make sense to have a helper file that implement something
> like
> > testCues() so you don't have to re-implement it dozens of time.
> > 
> > What about creating a track-helpers.js and add a `assert_cues_equal()`?
> 
> Agreed, makes sense. Even enableAllTextTracks() is done in some of the test
> cases. This as well we can move it to helper.

Done.

[Srirama, 2016-05-18 11:16:18]

On 2016/05/11 10:18:21, Srirama wrote:
>
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html
> (right):
> 
>
https://codereview.chromium.org/1956213002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/media/track/track-webvtt-tc002-bom.html:24:
> assert_equals(cues[i].text, textValues[i]);
> On 2016/05/10 09:32:50, Srirama wrote:
> > On 2016/05/10 09:14:39, Mounir Lamouri wrote:
> > > I think it would make sense to have a helper file that implement something
> > like
> > > testCues() so you don't have to re-implement it dozens of time.
> > > 
> > > What about creating a track-helpers.js and add a `assert_cues_equal()`?
> > 
> > Agreed, makes sense. Even enableAllTextTracks() is done in some of the test
> > cases. This as well we can move it to helper.
> 
> Done.

Gentle reminder.

[mlamouri (slow - plz ping), 2016-05-19 11:40:32]

Thanks for the reminder. I completely missed your review for some reasons.

lgtm with the comment applied.

https://codereview.chromium.org/1956213002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/1956213002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:9: function
assert_cues_equal(cues, expected) {
Can you change `expected` to be an array of Object instead of an Object
containing arrays?

so you could write:
```
for (var i = 0; i < cues.length; ++i) {
  assert_equals(cues[i].id, expected[i].id);
  [...]
}
```

[Srirama, 2016-05-19 13:59:36]

No problem, it happens when you have lot of reviews :)

https://codereview.chromium.org/1956213002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/1956213002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:9: function
assert_cues_equal(cues, expected) {
On 2016/05/19 11:40:31, Mounir Lamouri wrote:
> Can you change `expected` to be an array of Object instead of an Object
> containing arrays?
> 
> so you could write:
> ```
> for (var i = 0; i < cues.length; ++i) {
>   assert_equals(cues[i].id, expected[i].id);
>   [...]
> }
> ```

Done.

[Srirama, 2016-05-19 13:59:58]

The CQ bit was checked by srirama.m@samsung.com

[Srirama, 2016-05-19 13:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1956213002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-05-19 14:00:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956213002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956213002/60001

[commit-bot: I haz the power, 2016-05-19 15:10:37]

Description was changed from

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

to

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

[commit-bot: I haz the power, 2016-05-19 15:10:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-19 15:12:29]

Description was changed from

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests. 

BUG=588956
==========

to

==========
Convert track-webvtt-tc[000-002] tests to testharness.js

Cleaning up track-webvtt-tc[000-002] tests in media/track
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

BUG=588956

Committed: https://crrev.com/483b390585dcf834ffdba1013f80cc00ccdfc12f
Cr-Commit-Position: refs/heads/master@{#394771}
==========

[commit-bot: I haz the power, 2016-05-19 15:12:30]

Patchset 3 (id:??) landed as
https://crrev.com/483b390585dcf834ffdba1013f80cc00ccdfc12f
Cr-Commit-Position: refs/heads/master@{#394771}