Convert csp-*, event-* and invalid-* tests to testharness.js

Convert csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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/2403d8e3ac2673941329a1942091409e4b21bb64
Cr-Commit-Position: refs/heads/master@{#396827}

[Srirama, 2016-05-29 16:16:00]

Description was changed from

==========
fix


fix

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

to

==========
Convert csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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-05-29 16:17:07]

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

[Srirama, 2016-05-29 16:17:07]

PTAL

[fs, 2016-05-30 09:11:39]

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/csp-blocks-video.html (right):

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:15:
setTimeout(t.step_func_done(), 500);
window.onload = ...;

?

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/event-attributes.html (left):

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/event-attributes.html:16: // Don't log
progress event since the number and order are platform
Perhaps keep this comment.

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

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/event-attributes.html:27:
video.addEventListener(type, t.step_func(function() {
Hmm, the .on<event> will map to the attribute, while I don't think this will. So
maybe make this:

video['on' + type] = ...

to make sure the ... = null assignments below have the intended effect?

(It's quite possible that ditching the switch and just assign each "event-step"
manual could be more readable, but I'll leave that to you to decide.)

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html (left):

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:30:
document.body.appendChild(audio);
It doesn't look like these have an useful purpose, but maybe keep them just in
case (you never quite know with crash tests...)

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

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:10: var audio
= document.createElement("audio");
Could we turn this into two tests - one per tag-type? (Could even have function
to generate them for maximum de-dupe.)

[Srirama, 2016-05-31 07:04:23]

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/csp-blocks-video.html (right):

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:15:
setTimeout(t.step_func_done(), 500);
On 2016/05/30 09:11:39, fs wrote:
> window.onload = ...;
> 
> ?

Done.

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

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/event-attributes.html:27:
video.addEventListener(type, t.step_func(function() {
On 2016/05/30 09:11:39, fs wrote:
> Hmm, the .on<event> will map to the attribute, while I don't think this will.
So
> maybe make this:
> 
> video['on' + type] = ...
> 
> to make sure the ... = null assignments below have the intended effect?
> 
> (It's quite possible that ditching the switch and just assign each
"event-step"
> manual could be more readable, but I'll leave that to you to decide.)

Done.

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

https://codereview.chromium.org/2024533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:10: var audio
= document.createElement("audio");
On 2016/05/30 09:11:39, fs wrote:
> Could we turn this into two tests - one per tag-type? (Could even have
function
> to generate them for maximum de-dupe.)

Done.

[fs, 2016-05-31 08:53:06]

lgtm

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/event-attributes.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:81: video.onabort =
t.step_func(function() {
Arguably, these that don't "do" anything, could perhaps share a function:

var defaultEventStepFunction = t.step_func(...);
video.onabort = defaultEventStepFunction;
video.onduration = defaultEventStepFunction;
...

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:16: function
errorEvent() {
Maybe just have this be anonymous in the step_func_done (only place where it's
referenced.)

[foolip, 2016-05-31 10:50:18]

lgtm % nits

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/csp-blocks-video.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:14: video.load();
This shouldn't be needed, just setting the src attribute will trigger the media
element load algorithm.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:15: window.onload =
t.step_func_done();
This looks either racy or assuming a bit much knowledge of timing from the
reader. I guess that an error event will be fired on the video element, can you
use that as the pass condition instead?

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/event-attributes.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:2: <title>Test event
attributes for media element.</title>
This is testing way too many things at once IMHO. If it were just to test the
attributes, it could simply add event listeners and fire synthetic event to
verify that the wiring works. Instead this is testing all kinds of things in a
single test.

I don't think this would be accepted into w-p-t as is. It's fine to rewrite it
like this as a first step, but if you're up for it it would be good to figure
out what things are being implicitly tested here and see if there are already
tests for it in w-p-t or not.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:23:
actual_events.push(event.type);
This and the other event handlers is relying on the global event attribute on
window, but that's actually not per spec. IIRC, that's only supposed to be in
scope for event handler attributes in HTML, like <html
onclick="alert(event.target)"> and similar. So please add event as a function
argument here and elsewhere.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:25: return;
This seems kind of pointless given that oncanplaythrough is reset right after
this.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:71: // Don't log
progress event since the number and order are platform specific.
I've seen (maybe written) other tests like this that do log the progress events
and instead has rules that allow or require a certain number of them. This test
doesn't need to do everything, though.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:13:
mediaElement.onerror = t.step_func_done(errorEvent);
Suggest just inlining the errorEvent method so that it doesn't need to have a
name.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:14:
document.body.appendChild(mediaElement);
This shouldn't be neeed, it's probably here to solve a GC problem that I hope is
no longer present.

[Srirama, 2016-05-31 13:23:28]

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/csp-blocks-video.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:14: video.load();
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This shouldn't be needed, just setting the src attribute will trigger the
media
> element load algorithm.

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/csp-blocks-video.html:15: window.onload =
t.step_func_done();
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This looks either racy or assuming a bit much knowledge of timing from the
> reader. I guess that an error event will be fired on the video element, can
you
> use that as the pass condition instead?

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/event-attributes.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:2: <title>Test event
attributes for media element.</title>
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This is testing way too many things at once IMHO. If it were just to test the
> attributes, it could simply add event listeners and fire synthetic event to
> verify that the wiring works. Instead this is testing all kinds of things in a
> single test.
> 
> I don't think this would be accepted into w-p-t as is. It's fine to rewrite it
> like this as a first step, but if you're up for it it would be good to figure
> out what things are being implicitly tested here and see if there are already
> tests for it in w-p-t or not.

Acknowledged. I will check it later.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:23:
actual_events.push(event.type);
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This and the other event handlers is relying on the global event attribute on
> window, but that's actually not per spec. IIRC, that's only supposed to be in
> scope for event handler attributes in HTML, like <html
> onclick="alert(event.target)"> and similar. So please add event as a function
> argument here and elsewhere.

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:25: return;
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This seems kind of pointless given that oncanplaythrough is reset right after
> this.

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:71: // Don't log
progress event since the number and order are platform specific.
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> I've seen (maybe written) other tests like this that do log the progress
events
> and instead has rules that allow or require a certain number of them. This
test
> doesn't need to do everything, though.

Acknowledged.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/event-attributes.html:81: video.onabort =
t.step_func(function() {
On 2016/05/31 08:53:06, fs wrote:
> Arguably, these that don't "do" anything, could perhaps share a function:
> 
> var defaultEventStepFunction = t.step_func(...);
> video.onabort = defaultEventStepFunction;
> video.onduration = defaultEventStepFunction;
> ...

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html (right):

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:13:
mediaElement.onerror = t.step_func_done(errorEvent);
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> Suggest just inlining the errorEvent method so that it doesn't need to have a
> name.

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:14:
document.body.appendChild(mediaElement);
On 2016/05/31 10:50:18, Philip Jägenstedt wrote:
> This shouldn't be neeed, it's probably here to solve a GC problem that I hope
is
> no longer present.

Done.

https://codereview.chromium.org/2024533002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/invalid-media-url-crash.html:16: function
errorEvent() {
On 2016/05/31 08:53:06, fs wrote:
> Maybe just have this be anonymous in the step_func_done (only place where it's
> referenced.)

Done.

[foolip, 2016-05-31 13:57:05]

Thanks, PS3 LGTM

[Srirama, 2016-05-31 14:00:16]

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

[Srirama, 2016-05-31 14:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2024533002/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2016-05-31 14:00:32]

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

[commit-bot: I haz the power, 2016-05-31 15:03:23]

Description was changed from

==========
Convert csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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 csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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-05-31 15:03:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-31 15:05:01]

Description was changed from

==========
Convert csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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 csp-*, event-* and invalid-* tests to testharness.js

Cleaning up csp-*, event-* and invalid-* 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/2403d8e3ac2673941329a1942091409e4b21bb64
Cr-Commit-Position: refs/heads/master@{#396827}
==========

[commit-bot: I haz the power, 2016-05-31 15:05:02]

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