Convert track tests from video-test.js to testharness.js based

Convert track tests from video-test.js to testharness.js based

Cleaning up track-cues* 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/9d74837337cec545a019ddaa4418e873728146fa
Cr-Commit-Position: refs/heads/master@{#389787}

[Srirama, 2016-04-22 04:52:52]

Description was changed from

==========
fix

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

to

==========
Convert track tests from video-test.js to testharness.js based

Cleaning up track-cues* 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-04-22 05:02:13]

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

[Srirama, 2016-04-22 05:02:14]

PTAL

[mlamouri (slow - plz ping), 2016-04-22 10:27:57]

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html
(right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html:37: // Cue
entered.
Could the check be done outside this method?

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:43: cueCount =
3;
Can you explain this?

[Srirama, 2016-04-22 12:13:31]

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html
(right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html:37: // Cue
entered.
On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> Could the check be done outside this method?

For each cue, this function is called on "enter" and "exit" events. So
currentCue will not be active cue incase of "exit" event, so moving the same
check outside will fail.

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:43: cueCount =
3;
On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> Can you explain this?

As per the title, the events should be tested for all the cues starting from
time 0.0, but i think it is trying to test cues starting from time 5.0 as we are
setting currentTime to 5.0 before starting playback. Not sure why it was like
this, shall i try changing it to 0 and try to modify the test as per title?

[Srirama, 2016-04-22 12:28:41]

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:43: cueCount =
3;
On 2016/04/22 12:13:31, Srirama wrote:
> On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> > Can you explain this?
> 
> As per the title, the events should be tested for all the cues starting from
> time 0.0, but i think it is trying to test cues starting from time 5.0 as we
are
> setting currentTime to 5.0 before starting playback. Not sure why it was like
> this, shall i try changing it to 0 and try to modify the test as per title?

Sorry, i thought missed cues are 1 and 2 here as cue 3 starts from 5.0, but as
per missed-cues.vtt file, 3 to 7 are missed cues. So setting it to 3 is correct.

[mlamouri (slow - plz ping), 2016-04-25 15:37:47]

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html
(right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html:37: // Cue
entered.
On 2016/04/22 at 12:13:31, Srirama wrote:
> On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> > Could the check be done outside this method?
> 
> For each cue, this function is called on "enter" and "exit" events. So
currentCue will not be active cue incase of "exit" event, so moving the same
check outside will fail.

I meant adding the check in `cueChangedFromTrackElement`:
```
if (cueChangeCount % 2 == 0) { // Cue entered.
  checkCue();
}
```

You might as well inline the assert_equals() and remove `checkCue()`.

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:43: cueCount =
3;
On 2016/04/22 at 12:28:40, Srirama wrote:
> On 2016/04/22 12:13:31, Srirama wrote:
> > On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> > > Can you explain this?
> > 
> > As per the title, the events should be tested for all the cues starting from
> > time 0.0, but i think it is trying to test cues starting from time 5.0 as we
are
> > setting currentTime to 5.0 before starting playback. Not sure why it was
like
> > this, shall i try changing it to 0 and try to modify the test as per title?
> 
> Sorry, i thought missed cues are 1 and 2 here as cue 3 starts from 5.0, but as
per missed-cues.vtt file, 3 to 7 are missed cues. So setting it to 3 is correct.

I guess I don't understand the general idea here: we add events to cues 2 to 6
(0-indexed), ignoring 0 and 1. Then, we check that we get events from 3 to 6. I
wonder why we don't add events for the first two as you pointed.

Could you add comments in the test explaining why it is doing this. For example,
if we don't expect events to be sent for cues 0 and 1, maybe it should be said
in a comment when cueCount is set to 3?

[Srirama, 2016-04-26 06:49:27]

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html
(right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-cuechange.html:37: // Cue
entered.
On 2016/04/25 15:37:47, Mounir Lamouri wrote:
> On 2016/04/22 at 12:13:31, Srirama wrote:
> > On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> > > Could the check be done outside this method?
> > 
> > For each cue, this function is called on "enter" and "exit" events. So
> currentCue will not be active cue incase of "exit" event, so moving the same
> check outside will fail.
> 
> I meant adding the check in `cueChangedFromTrackElement`:
> ```
> if (cueChangeCount % 2 == 0) { // Cue entered.
>   checkCue();
> }
> ```
> 
> You might as well inline the assert_equals() and remove `checkCue()`.

Sorry, didn't read the comment properly.
Inlined it now.

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:43: cueCount =
3;
On 2016/04/25 15:37:47, Mounir Lamouri wrote:
> On 2016/04/22 at 12:28:40, Srirama wrote:
> > On 2016/04/22 12:13:31, Srirama wrote:
> > > On 2016/04/22 10:27:56, Mounir Lamouri wrote:
> > > > Can you explain this?
> > > 
> > > As per the title, the events should be tested for all the cues starting
from
> > > time 0.0, but i think it is trying to test cues starting from time 5.0 as
we
> are
> > > setting currentTime to 5.0 before starting playback. Not sure why it was
> like
> > > this, shall i try changing it to 0 and try to modify the test as per
title?
> > 
> > Sorry, i thought missed cues are 1 and 2 here as cue 3 starts from 5.0, but
as
> per missed-cues.vtt file, 3 to 7 are missed cues. So setting it to 3 is
correct.
> 
> I guess I don't understand the general idea here: we add events to cues 2 to 6
> (0-indexed), ignoring 0 and 1. Then, we check that we get events from 3 to 6.
I
> wonder why we don't add events for the first two as you pointed.
> 
> Could you add comments in the test explaining why it is doing this. For
example,
> if we don't expect events to be sent for cues 0 and 1, maybe it should be said
> in a comment when cueCount is set to 3?

Even i don't know why 1 and 2 are not missed cues. For now i am interpreting as
per missed-cues.vtt file and added a comment based on that.

https://codereview.chromium.org/1905923004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:52:
assert_equals(currentCue.id, cueCount.toString());
toString() for 2nd argument is fine? or is it better to use parseInt() for the
first argument?

[mlamouri (slow - plz ping), 2016-04-26 14:19:32]

lgtm

https://codereview.chromium.org/1905923004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cues-missed.html (right):

https://codereview.chromium.org/1905923004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cues-missed.html:52:
assert_equals(currentCue.id, cueCount.toString());
On 2016/04/26 at 06:49:27, Srirama wrote:
> toString() for 2nd argument is fine? or is it better to use parseInt() for the
first argument?

Sounds fine. It's pretty much the same.

Alternatively, you can do `assert_true(foo == bar);`

[Srirama, 2016-04-26 14:25:02]

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

[commit-bot: I haz the power, 2016-04-26 14:25:18]

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

[commit-bot: I haz the power, 2016-04-26 15:26:58]

Description was changed from

==========
Convert track tests from video-test.js to testharness.js based

Cleaning up track-cues* 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 tests from video-test.js to testharness.js based

Cleaning up track-cues* 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-04-26 15:27:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 15:29:23]

Description was changed from

==========
Convert track tests from video-test.js to testharness.js based

Cleaning up track-cues* 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 tests from video-test.js to testharness.js based

Cleaning up track-cues* 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/9d74837337cec545a019ddaa4418e873728146fa
Cr-Commit-Position: refs/heads/master@{#389787}
==========

[commit-bot: I haz the power, 2016-04-26 15:29:25]

Patchset 2 (id:??) landed as
https://crrev.com/9d74837337cec545a019ddaa4418e873728146fa
Cr-Commit-Position: refs/heads/master@{#389787}