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-add*, track-change* 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/69ff05f52cb48277bd8c88fac344aa05675cc183
Cr-Commit-Position: refs/heads/master@{#386411}

[Srirama, 2016-04-11 11:01:44]

Description was changed from

==========
fix


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

Cleaning up track-add*, track-change* 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

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

to

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

Cleaning up track-add*, track-change* 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-11 11:06:39]

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

[Srirama, 2016-04-11 11:06:40]

PTAL

[Srirama, 2016-04-11 11:20:05]

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

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-track.html:16:
video.textTracks.onaddtrack = t.step_func(function() {
Adding argument 't' here is causing the problem (t.done() is not defined). I
just checked the step_func in testharness.js and whenever the argument count is
1, this object is modified, i guess this may be causing the issue.
If i just drop the argument it works fine without any error.

[philipj_slow, 2016-04-11 12:34:01]

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html
(left):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html:19:
consoleWrite("<br>*** Test cues loaded from the file.");
Can you add back these consoleWrite things as comments? They're helpful when
reading the test even if they don't appear in the output.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html
(right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html:43: var
newCue = newTrack.cues[0];
This could be simpler I think by just assigning newCue to the new VTTCue, and
then assert_equals(newCue, newTrack.cues[0]). As it is now this might not even
fail if the added cue doesn't end up as the first cue.

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

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-track.html:16:
video.textTracks.onaddtrack = t.step_func(function() {
On 2016/04/11 11:20:05, Srirama wrote:
> Adding argument 't' here is causing the problem (t.done() is not defined). I
> just checked the step_func in testharness.js and whenever the argument count
is
> 1, this object is modified, i guess this may be causing the issue.
> If i just drop the argument it works fine without any error.

I guess you mean this bit?

        var test_this = this;

        if (arguments.length === 1) {
            this_obj = test_this;
        }

That changes what the this object is, but not the arguments, so if you had
"t.step_func(function(t) {})" it sounds expected that t would be undefined.

Just using this everywhere or a single t from the outermost async_test are the
simplest ways of going about this, I think.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-track.html:18:
assert_true(event instanceof window.TrackEvent, 'instanceof');
You can drop the window. here, the test will fail if that throws an exception
anyway.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-addtrack-kind.html
(right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-addtrack-kind.html:6: var
video;
Move this and the below into the test.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-change-event.html (right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-change-event.html:17:
assert_true(event instanceof window.Event, 'instanceof');
Drop window. prefix.

[mlamouri (slow - plz ping), 2016-04-11 13:18:08]

lgtm

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-change-event.html (right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-change-event.html:11: * a
"change" event. */
style: one line comment? At least use "//"?

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

Patchset #2 (id:20001) has been deleted

[Srirama, 2016-04-11 13:34:24]

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html
(left):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html:19:
consoleWrite("<br>*** Test cues loaded from the file.");
On 2016/04/11 12:34:00, philipj wrote:
> Can you add back these consoleWrite things as comments? They're helpful when
> reading the test even if they don't appear in the output.

Done.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html
(right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-remove-cue.html:43: var
newCue = newTrack.cues[0];
On 2016/04/11 12:34:00, philipj wrote:
> This could be simpler I think by just assigning newCue to the new VTTCue, and
> then assert_equals(newCue, newTrack.cues[0]). As it is now this might not even
> fail if the added cue doesn't end up as the first cue.

Done.

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

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-track.html:16:
video.textTracks.onaddtrack = t.step_func(function() {
On 2016/04/11 12:34:00, philipj wrote:
> On 2016/04/11 11:20:05, Srirama wrote:
> > Adding argument 't' here is causing the problem (t.done() is not defined). I
> > just checked the step_func in testharness.js and whenever the argument count
> is
> > 1, this object is modified, i guess this may be causing the issue.
> > If i just drop the argument it works fine without any error.
> 
> I guess you mean this bit?
> 
>         var test_this = this;
> 
>         if (arguments.length === 1) {
>             this_obj = test_this;
>         }
> 
> That changes what the this object is, but not the arguments, so if you had
> "t.step_func(function(t) {})" it sounds expected that t would be undefined.
> 
> Just using this everywhere or a single t from the outermost async_test are the
> simplest ways of going about this, I think.

Done.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-add-track.html:18:
assert_true(event instanceof window.TrackEvent, 'instanceof');
On 2016/04/11 12:34:00, philipj wrote:
> You can drop the window. here, the test will fail if that throws an exception
> anyway.

Done.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-addtrack-kind.html
(right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-addtrack-kind.html:6: var
video;
On 2016/04/11 12:34:00, philipj wrote:
> Move this and the below into the test.

Done.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/track/track-change-event.html (right):

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-change-event.html:11: * a
"change" event. */
On 2016/04/11 13:18:08, Mounir Lamouri wrote:
> style: one line comment? At least use "//"?

Done.

https://codereview.chromium.org/1873093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/track-change-event.html:17:
assert_true(event instanceof window.Event, 'instanceof');
On 2016/04/11 12:34:01, philipj wrote:
> Drop window. prefix.

Done.

[philipj_slow, 2016-04-11 14:41:51]

lgtm

[Srirama, 2016-04-11 15:53:50]

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

[Srirama, 2016-04-11 15:53:50]

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/1873093003/#ps40001
(title: "address comments")

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

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

[commit-bot: I haz the power, 2016-04-11 18:18:04]

Description was changed from

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

Cleaning up track-add*, track-change* 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-add*, track-change* 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-11 18:18:04]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-11 18:20:21]

Description was changed from

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

Cleaning up track-add*, track-change* 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-add*, track-change* 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/69ff05f52cb48277bd8c88fac344aa05675cc183
Cr-Commit-Position: refs/heads/master@{#386411}
==========

[commit-bot: I haz the power, 2016-04-11 18:20:22]

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