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-cue-rendering* 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/053272d957ba133ba95b295edfb95f411d51396b
Cr-Commit-Position: refs/heads/master@{#390071}

[Srirama, 2016-04-23 15:40:35]

Description was changed from

==========
fix


fix

BUG=

patch from issue 1911963004 at patchset 1 (http://crrev.com/1911963004#ps1)
==========

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.
==========

[Srirama, 2016-04-23 15:41:11]

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.
==========

to

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

Cleaning up track-cue-rendering* tests in media/track to use testharness.js
instead of video-test.js. This will enable to upstream these tests
to web-platform-tests.
==========

[Srirama, 2016-04-23 15:41:47]

Description was changed from

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

Cleaning up track-cue-rendering* tests in media/track to use testharness.js
instead of video-test.js. This will enable to upstream these tests
to web-platform-tests.
==========

to

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

Cleaning up track-cue-rendering* tests in media/track to use
testharness.js instead of video-test.js. This will enable to upstream
these tests to web-platform-tests.
==========

[Srirama, 2016-04-23 15:42:07]

Description was changed from

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

Cleaning up track-cue-rendering* tests in media/track to use
testharness.js instead of video-test.js. This will enable to upstream
these tests to web-platform-tests.
==========

to

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

Cleaning up track-cue-rendering* tests in media/track to use
testharness.js instead of video-test.js. This will enable to
upstream these tests to web-platform-tests.
==========

[Srirama, 2016-04-25 09:22:53]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-04-25 11:13:22]

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

[Srirama, 2016-04-25 11:13:23]

PTAL.

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html
(right):

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html:37:
var cueRenderingPosition = [
Right now these cue rendering position values are logged using consoleWrite into
expected file. So i have made an array of expected values here. First 7 test
cases have 1 active queue and the 8th test has 7 active cues, so i have made the
array accordingly. Please suggest if there is a better way.

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html
(right):

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html:8:
<track src="captions-webvtt/captions.vtt" kind="captions" mode="showing">
i have added "mode" attribute here and removed the two lines related to track
finding and setting its mode.

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html
(right):

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:36:
assert_equals(getComputedStyle(textTrackDisplayElement(video,
"display").firstChild).backgroundColor, "rgba(0, 0, 0, 0.8)");
mediacontrols.css specifies, background-color: rgba(0, 0, 0, 0.8); for
video::cue, so i have changed it to make the test pass.

[philipj_slow, 2016-04-25 11:55:26]

philipj@opera.com changed reviewers:
- philipj@opera.com

[philipj_slow, 2016-04-25 11:55:27]

This is my last week at Opera, so I can't take on any new reviews I'm afraid.

[mlamouri (slow - plz ping), 2016-04-25 15:53:26]

You are missing the BUG= line.

Though, I did not have enough time to look at the entire patch today. Will
finish tomorrow.

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html
(right):

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:47:
document.body.offsetTop;
nit: add a comment explaining this line is for forcing a layout.

[Srirama, 2016-04-26 06:50:19]

Description was changed from

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

Cleaning up track-cue-rendering* tests in media/track to use
testharness.js instead of video-test.js. This will enable to
upstream these tests to web-platform-tests.
==========

to

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

Cleaning up track-cue-rendering* 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-26 07:00:16]

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html
(right):

https://codereview.chromium.org/1913423002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:47:
document.body.offsetTop;
On 2016/04/25 15:53:26, Mounir Lamouri wrote:
> nit: add a comment explaining this line is for forcing a layout.

Done.

[mlamouri (slow - plz ping), 2016-04-26 15:21:36]

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-rtl.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-rtl.html:41:
assert_true(2 * testCueDisplayBox.offsetLeft == video.videoWidth -
testCueDisplayBox.offsetWidth);
assert_equals?

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html:25:
t.done();
Instead of doing that, could you check if there was an exception thrown and
check for this? Ideally, you could check that the exception matches the
type/name.

The issue with the current system if that the test will timeout if the exception
is no longer fired which could be easily taken for a flake.

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:25:
assert_true(2 * testCueDisplayBox.offsetLeft == video.videoWidth -
testCueDisplayBox.offsetWidth)
assert_equals?

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:40:
video.currentTime = video.currentTime + 1;
nit: `+= 1` or even `++`?

[Srirama, 2016-04-27 05:39:22]

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-rtl.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-rtl.html:41:
assert_true(2 * testCueDisplayBox.offsetLeft == video.videoWidth -
testCueDisplayBox.offsetWidth);
On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> assert_equals?

Done.

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html:25:
t.done();
On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> Instead of doing that, could you check if there was an exception thrown and
> check for this? Ideally, you could check that the exception matches the
> type/name.
> 
> The issue with the current system if that the test will timeout if the
exception
> is no longer fired which could be easily taken for a flake.

Problem is it currently throws a message, 
**** throw "Failed to find text track container element"; *****
So the exception name is undefined. Or should i assert_equals with the string 
"Failed to find text track container element" ?

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:25:
assert_true(2 * testCueDisplayBox.offsetLeft == video.videoWidth -
testCueDisplayBox.offsetWidth)
On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> assert_equals?

Done.

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering.html:40:
video.currentTime = video.currentTime + 1;
On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> nit: `+= 1` or even `++`?

Done.

[philipj_slow, 2016-04-27 09:02:28]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2016-04-27 09:02:29]

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html:25:
t.done();
On 2016/04/27 05:39:22, Srirama wrote:
> On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> > Instead of doing that, could you check if there was an exception thrown and
> > check for this? Ideally, you could check that the exception matches the
> > type/name.
> > 
> > The issue with the current system if that the test will timeout if the
> exception
> > is no longer fired which could be easily taken for a flake.
> 
> Problem is it currently throws a message, 
> **** throw "Failed to find text track container element"; *****
> So the exception name is undefined. Or should i assert_equals with the string 
> "Failed to find text track container element" ?

Try assert_throws(null, function() { textTrackDisplayElement(video, "display");
});

[Srirama, 2016-04-27 10:39:04]

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html
(right):

https://codereview.chromium.org/1913423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-tree-is-removed-properly.html:25:
t.done();
On 2016/04/27 09:02:29, philipj_slow wrote:
> On 2016/04/27 05:39:22, Srirama wrote:
> > On 2016/04/26 15:21:35, Mounir Lamouri wrote:
> > > Instead of doing that, could you check if there was an exception thrown
and
> > > check for this? Ideally, you could check that the exception matches the
> > > type/name.
> > > 
> > > The issue with the current system if that the test will timeout if the
> > exception
> > > is no longer fired which could be easily taken for a flake.
> > 
> > Problem is it currently throws a message, 
> > **** throw "Failed to find text track container element"; *****
> > So the exception name is undefined. Or should i assert_equals with the
string 
> > "Failed to find text track container element" ?
> 
> Try assert_throws(null, function() { textTrackDisplayElement(video,
"display");
> });

Thanks, works fine now.

[mlamouri (slow - plz ping), 2016-04-27 11:26:32]

lgtm with the comment below addressed.

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html
(right):

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html:58:
t.done();
Should you do:
```
if (seekTimeIndex == testCaseSeekTime.length) {
  t.done();
  return;
}
```

[mlamouri (slow - plz ping), 2016-04-27 11:26:33]

lgtm with the comment below addressed.

[Srirama, 2016-04-27 11:32:00]

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html
(right):

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html:58:
t.done();
On 2016/04/27 11:26:32, Mounir Lamouri wrote:
> Should you do:
> ```
> if (seekTimeIndex == testCaseSeekTime.length) {
>   t.done();
>   return;
> }
> ```

Once we call t.done(), the test will be finished and the remaining code will not
be executed. So is it required? I just verified by adding assert_false(true);
after t.done() and it is of no effect.

[philipj_slow, 2016-04-27 13:11:07]

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html
(right):

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html:58:
t.done();
On 2016/04/27 11:32:00, Srirama wrote:
> On 2016/04/27 11:26:32, Mounir Lamouri wrote:
> > Should you do:
> > ```
> > if (seekTimeIndex == testCaseSeekTime.length) {
> >   t.done();
> >   return;
> > }
> > ```
> 
> Once we call t.done(), the test will be finished and the remaining code will
not
> be executed. So is it required? I just verified by adding assert_false(true);
> after t.done() and it is of no effect.

Nothing that happens after t.done() will change the outcome of the test, but the
code will continue to execute. So for clarity I agree that an early return here
would be good, otherwise one might wonder what the following lines actually do
afterwards.

[Srirama, 2016-04-27 13:18:43]

Patchset #5 (id:100001) has been deleted

[Srirama, 2016-04-27 13:21:31]

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html
(right):

https://codereview.chromium.org/1913423002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html:58:
t.done();
On 2016/04/27 13:11:07, philipj_slow wrote:
> On 2016/04/27 11:32:00, Srirama wrote:
> > On 2016/04/27 11:26:32, Mounir Lamouri wrote:
> > > Should you do:
> > > ```
> > > if (seekTimeIndex == testCaseSeekTime.length) {
> > >   t.done();
> > >   return;
> > > }
> > > ```
> > 
> > Once we call t.done(), the test will be finished and the remaining code will
> not
> > be executed. So is it required? I just verified by adding
assert_false(true);
> > after t.done() and it is of no effect.
> 
> Nothing that happens after t.done() will change the outcome of the test, but
the
> code will continue to execute. So for clarity I agree that an early return
here
> would be good, otherwise one might wonder what the following lines actually do
> afterwards.

Done.

[Srirama, 2016-04-27 13:22:07]

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

[Srirama, 2016-04-27 13:22:08]

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/1913423002/#ps120001
(title: "address comment")

[commit-bot: I haz the power, 2016-04-27 13:22:20]

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

[commit-bot: I haz the power, 2016-04-27 14:45:58]

Description was changed from

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

Cleaning up track-cue-rendering* 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-cue-rendering* 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-27 14:45:59]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-04-27 14:48:10]

Description was changed from

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

Cleaning up track-cue-rendering* 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-cue-rendering* 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/053272d957ba133ba95b295edfb95f411d51396b
Cr-Commit-Position: refs/heads/master@{#390071}
==========

[commit-bot: I haz the power, 2016-04-27 14:48:11]

Patchset 5 (id:??) landed as
https://crrev.com/053272d957ba133ba95b295edfb95f411d51396b
Cr-Commit-Position: refs/heads/master@{#390071}