Convert media-cont* and media-element* tests to testharness.js

Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
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/a462ecaa9f7d0abf50519fc9508ed68a2746371b
Cr-Commit-Position: refs/heads/master@{#397398}

[Srirama, 2016-06-01 20:18:36]

Description was changed from

==========
fix

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

to

==========
Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

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

[Srirama, 2016-06-01 20:21:21]

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

[Srirama, 2016-06-01 20:21:22]

PTAL

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:15:
assert_equals(getTimeLineValue(), video.currentTime);
Should we compare with 0 instead of video.currentTime here and in error event?

[foolip, 2016-06-02 09:37:04]

lgtm

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/media-continues-playing-after-replace-source.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-continues-playing-after-replace-source.html:35:
timeupdateEventCount = 0;
This line makes the structure of the test harder to understand, but it's fine to
keep it this way, better to improve test separately from the testharness.js
rewrites I guess.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:15:
assert_equals(getTimeLineValue(), video.currentTime);
On 2016/06/01 20:21:22, Srirama wrote:
> Should we compare with 0 instead of video.currentTime here and in error event?

If there are other tests that verify how and when currentTime changes then I
guess the right behavior is still locked down. Maybe you can just add assertions
for the video.currentTime values first, to be extra clear about what is being
assumed:

assert_equals(video.currentTime, 0);
assert_equals(getTimeLineValue(), 0);

In onseeked/onerror as well, of course.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:31:
function getTimeLineValue() {
This capitalization is too weird, and it's MediaControlTimelineElement
internally, can you change it?

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:33: return
parseInt(timeStr);
parseInt('1.5') return 1, so it's not obvious if this is implicitly allowing for
some difference in the actual value. Can you return +timeStr and see if the test
still passes?

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html:2:
<title>This test ensures that media element fires the "playing" event every time
it starts playing after eos. It also ensure that "pause" and "ended" events are
fired when media playback ends.</title>
I'm not sure I understand why this needs to test that it works multiple times,
perhaps there was a bug at one point that this covers. Can you see why the test
was added?

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html:38:
audio.currentTime = audio.duration - 0.2;
This all feels a bit convoluted to me, and AFAICT it doesn't test the order of
the pause and ended events.

If you like, as a follow-up, I think this test would benefit from being more in
the style of media/autoplay-with-preload-none.html where the event order is
explicitly recorder and verified at the end of the test.

[Srirama, 2016-06-02 13:29:37]

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/media-continues-playing-after-replace-source.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-continues-playing-after-replace-source.html:35:
timeupdateEventCount = 0;
On 2016/06/02 09:37:03, Philip Jägenstedt wrote:
> This line makes the structure of the test harder to understand, but it's fine
to
> keep it this way, better to improve test separately from the testharness.js
> rewrites I guess.

Acknowledged.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:15:
assert_equals(getTimeLineValue(), video.currentTime);
On 2016/06/02 09:37:03, Philip Jägenstedt wrote:
> On 2016/06/01 20:21:22, Srirama wrote:
> > Should we compare with 0 instead of video.currentTime here and in error
event?
> 
> If there are other tests that verify how and when currentTime changes then I
> guess the right behavior is still locked down. Maybe you can just add
assertions
> for the video.currentTime values first, to be extra clear about what is being
> assumed:
> 
> assert_equals(video.currentTime, 0);
> assert_equals(getTimeLineValue(), 0);
> 
> In onseeked/onerror as well, of course.

Done.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:31:
function getTimeLineValue() {
On 2016/06/02 09:37:04, Philip Jägenstedt wrote:
> This capitalization is too weird, and it's MediaControlTimelineElement
> internally, can you change it?

Done.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-controls-invalid-url.html:33: return
parseInt(timeStr);
On 2016/06/02 09:37:03, Philip Jägenstedt wrote:
> parseInt('1.5') return 1, so it's not obvious if this is implicitly allowing
for
> some difference in the actual value. Can you return +timeStr and see if the
test
> still passes?

Done.

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html:2:
<title>This test ensures that media element fires the "playing" event every time
it starts playing after eos. It also ensure that "pause" and "ended" events are
fired when media playback ends.</title>
On 2016/06/02 09:37:04, Philip Jägenstedt wrote:
> I'm not sure I understand why this needs to test that it works multiple times,
> perhaps there was a bug at one point that this covers. Can you see why the
test
> was added?

It is from webkit times and there is an issue of not being set "paused" to true
and it is preventing "playing" event.
(https://bugs.webkit.org/show_bug.cgi?id=60972)

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html:38:
audio.currentTime = audio.duration - 0.2;
On 2016/06/02 09:37:04, Philip Jägenstedt wrote:
> This all feels a bit convoluted to me, and AFAICT it doesn't test the order of
> the pause and ended events.
> 
> If you like, as a follow-up, I think this test would benefit from being more
in
> the style of media/autoplay-with-preload-none.html where the event order is
> explicitly recorder and verified at the end of the test.

Acknowledged.

[foolip, 2016-06-02 13:36:11]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-06-02 13:36:11]

lgtm

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html
(right):

https://codereview.chromium.org/2031783002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/media-element-play-after-eos.html:2:
<title>This test ensures that media element fires the "playing" event every time
it starts playing after eos. It also ensure that "pause" and "ended" events are
fired when media playback ends.</title>
On 2016/06/02 13:29:37, Srirama wrote:
> On 2016/06/02 09:37:04, Philip Jägenstedt wrote:
> > I'm not sure I understand why this needs to test that it works multiple
times,
> > perhaps there was a bug at one point that this covers. Can you see why the
> test
> > was added?
> 
> It is from webkit times and there is an issue of not being set "paused" to
true
> and it is preventing "playing" event.
> (https://bugs.webkit.org/show_bug.cgi?id=60972)

OK, so doing it twice is very intentional then.

[commit-bot: I haz the power, 2016-06-02 13:36:29]

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

[commit-bot: I haz the power, 2016-06-02 14:50:11]

Description was changed from

==========
Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

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

to

==========
Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
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-06-02 14:50:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-02 14:51:41]

Description was changed from

==========
Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

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

to

==========
Convert media-cont* and media-element* tests to testharness.js

Cleaning up media-cont* and media-element* tests in media/
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/a462ecaa9f7d0abf50519fc9508ed68a2746371b
Cr-Commit-Position: refs/heads/master@{#397398}
==========

[commit-bot: I haz the power, 2016-06-02 14:51:42]

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