Convert track-webvtt-tc[016-018] tests to testharness.js

Convert track-webvtt-tc[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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/d79f6e18aed7f29b8cadc61042a60208f9e61521
Cr-Commit-Position: refs/heads/master@{#396211}

[Srirama, 2016-05-25 17:59:08]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-05-25 18:03:12]

Description was changed from

==========
fix

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

to

==========
Convert track-webvtt-tc[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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-25 18:04:03]

srirama.m@samsung.com changed reviewers:
+ foolip@chromium.org, fs@opera.com

[Srirama, 2016-05-25 18:04:05]

PTAL

[fs, 2016-05-25 19:49:56]

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

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:19: function
assert_cues_equal_by_style(cues, expected) {
s/style/settings/ maybe

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html:15:
for (var i = 0; i < video.textTracks.length; i++)
It feels a bit odd to use another length for iteration. Maybe use
trackElements.length and assert that the two lists are of equal length?

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html:34:
position : 10,
I think it would be ok to write these lists as one entry per line (more
compact), i.e:

{ position: 10, align: "start",  vertical: "" },
{ position: 20, align: "middle", vertical: "" },
...

(Align probably not needed, but might help a bit)

Aesthetic preference, so feel free to ignore.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html:15:
for (var i = 0; i < video.textTracks.length; i++)
Same comment as in the previous test.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html:63:
testCues(video.textTracks[index].cues, expected);
With all these "compare various properties on cues" cases, maybe a slightly more
generic version like:

assert_cues_match(cues, expected, [ "line", "snapToLines" ]);

which check the properties in the array, so approx:

for (var property of propertyArray)
  assert_equals(cue[property], expected[property], property);

instead of the hard-coded assert_equals.

Could probably iterate the existing own properties on expected[i] too, but
that's probably more generic than we need.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:15:
for (var i = 0; i < video.textTracks.length; i++)
Same.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:21:
function trackLoaded() {
Getting a feeling that turning this pattern into a helper will save you some
typing in the future =) (There are a few of these tests IIRC.)

[Srirama, 2016-05-26 10:47:19]

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

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:19: function
assert_cues_equal_by_style(cues, expected) {
On 2016/05/25 19:49:56, fs wrote:
> s/style/settings/ maybe

Renamed as assert_cues_match

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html:15:
for (var i = 0; i < video.textTracks.length; i++)
On 2016/05/25 19:49:56, fs wrote:
> It feels a bit odd to use another length for iteration. Maybe use
> trackElements.length and assert that the two lists are of equal length?

Done.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc016-align-positioning.html:34:
position : 10,
On 2016/05/25 19:49:56, fs wrote:
> I think it would be ok to write these lists as one entry per line (more
> compact), i.e:
> 
> { position: 10, align: "start",  vertical: "" },
> { position: 20, align: "middle", vertical: "" },
> ...
> 
> (Align probably not needed, but might help a bit)
> 
> Aesthetic preference, so feel free to ignore.

Done.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html:15:
for (var i = 0; i < video.textTracks.length; i++)
On 2016/05/25 19:49:56, fs wrote:
> Same comment as in the previous test.

Done.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc017-line-position.html:63:
testCues(video.textTracks[index].cues, expected);
On 2016/05/25 19:49:56, fs wrote:
> With all these "compare various properties on cues" cases, maybe a slightly
more
> generic version like:
> 
> assert_cues_match(cues, expected, [ "line", "snapToLines" ]);
> 
> which check the properties in the array, so approx:
> 
> for (var property of propertyArray)
>   assert_equals(cue[property], expected[property], property);
> 
> instead of the hard-coded assert_equals.
> 
> Could probably iterate the existing own properties on expected[i] too, but
> that's probably more generic than we need.

Done.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html
(right):

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:15:
for (var i = 0; i < video.textTracks.length; i++)
On 2016/05/25 19:49:56, fs wrote:
> Same.

Done.

https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:21:
function trackLoaded() {
On 2016/05/25 19:49:56, fs wrote:
> Getting a feeling that turning this pattern into a helper will save you some
> typing in the future =) (There are a few of these tests IIRC.)

ok, but how can we call these testTrack and testTrackError functions from that
helper ?

[fs, 2016-05-26 12:18:22]

>
https://codereview.chromium.org/2012833002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:21:
function trackLoaded() {
> On 2016/05/25 19:49:56, fs wrote:
> > Getting a feeling that turning this pattern into a helper will save you some
> > typing in the future =) (There are a few of these tests IIRC.)
> 
> ok, but how can we call these testTrack and testTrackError functions from that
helper ?

I think this can be done a number of ways - a bit depending on how data-driven
one wants to make it.

The most straight-forward way would likely be to use multiple async_test()s,
with each being:

function check_cues_from_track(trackElement, func) {
  async_test(function() {
    trackElement.onload = t.step_func_done(function(e) {
      func(trackElement.track);
    });
    trackElement.onerror = t.step_unreachable();
  }, "Check cues from " + trackElement.src);
}

function func1(track) {
  var expected = [ ... ];
  assert_cues_match(track.cues, expected, ...);
}
...

check_cues_from_track(trackElements[0], func1);
check_cues_from_track(trackElements[1], func2);


Could probably also have a form that takes a URL and creates the
HTMLTrackElement itself et.c.

You can look at LayoutTests/media/track/opera/track/webvtt/parsing/001.html for
some inspiration.

No need to include that in this CL though, do that separately.

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

https://codereview.chromium.org/2012833002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:23:
assert_equals(cues[property], expected[property]);
This needs to index in cues too. I'd suggest loading the current cue into a
local before the inner loop and then use that (inside the inner loop.)

[Srirama, 2016-05-26 13:34:50]

Thanks for the detailed information, i will try in my next patch.

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

https://codereview.chromium.org/2012833002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:23:
assert_equals(cues[property], expected[property]);
On 2016/05/26 12:18:22, fs wrote:
> This needs to index in cues too. I'd suggest loading the current cue into a
> local before the inner loop and then use that (inside the inner loop.)

Done.

[fs, 2016-05-26 13:39:04]

lgtm

[Srirama, 2016-05-26 14:08:16]

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

[commit-bot: I haz the power, 2016-05-26 14:08:29]

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

[commit-bot: I haz the power, 2016-05-26 17:18:11]

Description was changed from

==========
Convert track-webvtt-tc[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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-26 17:18:12]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-26 17:20:20]

Description was changed from

==========
Convert track-webvtt-tc[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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[016-018] tests to testharness.js

Cleaning up track-webvtt-tc[016-018] 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/d79f6e18aed7f29b8cadc61042a60208f9e61521
Cr-Commit-Position: refs/heads/master@{#396211}
==========

[commit-bot: I haz the power, 2016-05-26 17:20:21]

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