Convert audio-controls* and audio-delete* tests to testharness.js

Convert audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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/ceb895e19332cab2b012148c5319bb6eddae7a0b
Cr-Commit-Position: refs/heads/master@{#395908}

[Srirama, 2016-05-24 13:14:01]

Description was changed from

==========
fix

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

to

==========
Convert audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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-24 13:19:38]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-05-24 13:19:53]

srirama.m@samsung.com changed reviewers:
+ fs@opera.com

[Srirama, 2016-05-24 13:19:53]

PTAL

[fs, 2016-05-24 13:32:23]

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/audio-controls-captions.html (right):

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
audio.onloadedmetadata =
t.step_func_done(testClosedCaptionsButtonVisibility(false));
This doesn't look right, the argument ought to be as before, or maybe using
bind.

[Srirama, 2016-05-24 13:41:28]

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/audio-controls-captions.html (right):

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
audio.onloadedmetadata =
t.step_func_done(testClosedCaptionsButtonVisibility(false));
On 2016/05/24 13:32:23, fs wrote:
> This doesn't look right, the argument ought to be as before, or maybe using
> bind.

you mean it should be
..... = t.step_func_done(function() {
testClosedCaptionsButtonVisibility(false));
});
?

Even directly calling a function inside it, is working fine. I just changed
"false" to "true" and it fails the test. Will there be a problem with this?

[fs, 2016-05-24 14:06:20]

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/audio-controls-captions.html (right):

https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
audio.onloadedmetadata =
t.step_func_done(testClosedCaptionsButtonVisibility(false));
On 2016/05/24 at 13:41:28, Srirama wrote:
> On 2016/05/24 13:32:23, fs wrote:
> > This doesn't look right, the argument ought to be as before, or maybe using
> > bind.
> 
> you mean it should be
> ..... = t.step_func_done(function() {
> testClosedCaptionsButtonVisibility(false));
> });
> ?
> 
> Even directly calling a function inside it, is working fine. I just changed
"false" to "true" and it fails the test. Will there be a problem with this?

It's confusing as to what ordering is required. I think the intention of the
test is to check that the 'CC' button isn't visible after the media file is
sufficiently loaded. With the changes to track selection I think this passes
trivially though.. (i.e the <track> should have a 'default' attribute added to
be more reasonable.)

testClosedCaptionsButtonVisibility appears to be using testExpected though too,
how is that handled?

[Srirama, 2016-05-25 07:26:30]

On 2016/05/24 14:06:20, fs wrote:
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/media/audio-controls-captions.html
(right):
> 
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
> audio.onloadedmetadata =
> t.step_func_done(testClosedCaptionsButtonVisibility(false));
> On 2016/05/24 at 13:41:28, Srirama wrote:
> > On 2016/05/24 13:32:23, fs wrote:
> > > This doesn't look right, the argument ought to be as before, or maybe
using
> > > bind.
> > 
> > you mean it should be
> > ..... = t.step_func_done(function() {
> > testClosedCaptionsButtonVisibility(false));
> > });
> > ?
> > 
> > Even directly calling a function inside it, is working fine. I just changed
> "false" to "true" and it fails the test. Will there be a problem with this?
> 
> It's confusing as to what ordering is required. I think the intention of the
> test is to check that the 'CC' button isn't visible after the media file is
> sufficiently loaded. With the changes to track selection I think this passes
> trivially though.. (i.e the <track> should have a 'default' attribute added to
> be more reasonable.)
> 
> testClosedCaptionsButtonVisibility appears to be using testExpected though
too,
> how is that handled?

Sorry actually there are more problems here, inside
testClosedCaptionsButtonVisibility it is using mediaElement which is set using
"findMediaElement" call. But now that i removed video-test.js reference and
removed call to "findMediaElement" mediaElement is not set and the call
testClosedCaptionsButtonVisibility never reaches "testExpected".
I think i have to do some work in testClosedCaptionsButtonVisibility function
before i can covert these tests.
So should i clean up testClosedCaptionsButtonVisibility to remove testExpected
and mediaElement usage from it and update all the effected *-expected.txt files
and then come to these test cases? Is it the right approach or is there any
better way?

[fs, 2016-05-25 07:57:37]

On 2016/05/25 at 07:26:30, srirama.m wrote:
> On 2016/05/24 14:06:20, fs wrote:
> >
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/media/audio-controls-captions.html
(right):
> > 
> >
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
> > audio.onloadedmetadata =
> > t.step_func_done(testClosedCaptionsButtonVisibility(false));
> > On 2016/05/24 at 13:41:28, Srirama wrote:
> > > On 2016/05/24 13:32:23, fs wrote:
> > > > This doesn't look right, the argument ought to be as before, or maybe
using
> > > > bind.
> > > 
> > > you mean it should be
> > > ..... = t.step_func_done(function() {
> > > testClosedCaptionsButtonVisibility(false));
> > > });
> > > ?
> > > 
> > > Even directly calling a function inside it, is working fine. I just
changed
> > "false" to "true" and it fails the test. Will there be a problem with this?
> > 
> > It's confusing as to what ordering is required. I think the intention of the
> > test is to check that the 'CC' button isn't visible after the media file is
> > sufficiently loaded. With the changes to track selection I think this passes
> > trivially though.. (i.e the <track> should have a 'default' attribute added
to
> > be more reasonable.)
> > 
> > testClosedCaptionsButtonVisibility appears to be using testExpected though
too,
> > how is that handled?
> 
> Sorry actually there are more problems here, inside
testClosedCaptionsButtonVisibility it is using mediaElement which is set using
"findMediaElement" call. But now that i removed video-test.js reference and
removed call to "findMediaElement" mediaElement is not set and the call
testClosedCaptionsButtonVisibility never reaches "testExpected".
> I think i have to do some work in testClosedCaptionsButtonVisibility function
before i can covert these tests.
> So should i clean up testClosedCaptionsButtonVisibility to remove testExpected
and mediaElement usage from it and update all the effected *-expected.txt files
and then come to these test cases? Is it the right approach or is there any
better way?

One way could be to split out the "predicate" from
testClosedCaptionsButtonVisibility (maybe isClosedCaptionsButtonVisible or so)
and then use that predicate here (and phase out any other uses of the current
function similarly.) Hopefully you then wouldn't need to update any additional
*-expected.txt files.

[Srirama, 2016-05-25 09:52:45]

On 2016/05/25 07:57:37, fs wrote:
> On 2016/05/25 at 07:26:30, srirama.m wrote:
> > On 2016/05/24 14:06:20, fs wrote:
> > >
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/media/audio-controls-captions.html
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
> > > audio.onloadedmetadata =
> > > t.step_func_done(testClosedCaptionsButtonVisibility(false));
> > > On 2016/05/24 at 13:41:28, Srirama wrote:
> > > > On 2016/05/24 13:32:23, fs wrote:
> > > > > This doesn't look right, the argument ought to be as before, or maybe
> using
> > > > > bind.
> > > > 
> > > > you mean it should be
> > > > ..... = t.step_func_done(function() {
> > > > testClosedCaptionsButtonVisibility(false));
> > > > });
> > > > ?
> > > > 
> > > > Even directly calling a function inside it, is working fine. I just
> changed
> > > "false" to "true" and it fails the test. Will there be a problem with
this?
> > > 
> > > It's confusing as to what ordering is required. I think the intention of
the
> > > test is to check that the 'CC' button isn't visible after the media file
is
> > > sufficiently loaded. With the changes to track selection I think this
passes
> > > trivially though.. (i.e the <track> should have a 'default' attribute
added
> to
> > > be more reasonable.)
> > > 
> > > testClosedCaptionsButtonVisibility appears to be using testExpected though
> too,
> > > how is that handled?
> > 
> > Sorry actually there are more problems here, inside
> testClosedCaptionsButtonVisibility it is using mediaElement which is set using
> "findMediaElement" call. But now that i removed video-test.js reference and
> removed call to "findMediaElement" mediaElement is not set and the call
> testClosedCaptionsButtonVisibility never reaches "testExpected".
> > I think i have to do some work in testClosedCaptionsButtonVisibility
function
> before i can covert these tests.
> > So should i clean up testClosedCaptionsButtonVisibility to remove
testExpected
> and mediaElement usage from it and update all the effected *-expected.txt
files
> and then come to these test cases? Is it the right approach or is there any
> better way?
> 
> One way could be to split out the "predicate" from
> testClosedCaptionsButtonVisibility (maybe isClosedCaptionsButtonVisible or so)
> and then use that predicate here (and phase out any other uses of the current
> function similarly.) Hopefully you then wouldn't need to update any additional
> *-expected.txt files.

I added a new function "isClosedCaptionsButtonVisible" and modified the test
accordingly. I hope this is what you mean by "predicate".

[fs, 2016-05-25 10:26:19]

On 2016/05/25 at 09:52:45, srirama.m wrote:
> On 2016/05/25 07:57:37, fs wrote:
> > On 2016/05/25 at 07:26:30, srirama.m wrote:
> > > On 2016/05/24 14:06:20, fs wrote:
> > > >
> >
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > > File third_party/WebKit/LayoutTests/media/audio-controls-captions.html
> > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > > third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
> > > > audio.onloadedmetadata =
> > > > t.step_func_done(testClosedCaptionsButtonVisibility(false));
> > > > On 2016/05/24 at 13:41:28, Srirama wrote:
> > > > > On 2016/05/24 13:32:23, fs wrote:
> > > > > > This doesn't look right, the argument ought to be as before, or
maybe
> > using
> > > > > > bind.
> > > > > 
> > > > > you mean it should be
> > > > > ..... = t.step_func_done(function() {
> > > > > testClosedCaptionsButtonVisibility(false));
> > > > > });
> > > > > ?
> > > > > 
> > > > > Even directly calling a function inside it, is working fine. I just
> > changed
> > > > "false" to "true" and it fails the test. Will there be a problem with
this?
> > > > 
> > > > It's confusing as to what ordering is required. I think the intention of
the
> > > > test is to check that the 'CC' button isn't visible after the media file
is
> > > > sufficiently loaded. With the changes to track selection I think this
passes
> > > > trivially though.. (i.e the <track> should have a 'default' attribute
added
> > to
> > > > be more reasonable.)
> > > > 
> > > > testClosedCaptionsButtonVisibility appears to be using testExpected
though
> > too,
> > > > how is that handled?
> > > 
> > > Sorry actually there are more problems here, inside
> > testClosedCaptionsButtonVisibility it is using mediaElement which is set
using
> > "findMediaElement" call. But now that i removed video-test.js reference and
> > removed call to "findMediaElement" mediaElement is not set and the call
> > testClosedCaptionsButtonVisibility never reaches "testExpected".
> > > I think i have to do some work in testClosedCaptionsButtonVisibility
function
> > before i can covert these tests.
> > > So should i clean up testClosedCaptionsButtonVisibility to remove
testExpected
> > and mediaElement usage from it and update all the effected *-expected.txt
files
> > and then come to these test cases? Is it the right approach or is there any
> > better way?
> > 
> > One way could be to split out the "predicate" from
> > testClosedCaptionsButtonVisibility (maybe isClosedCaptionsButtonVisible or
so)
> > and then use that predicate here (and phase out any other uses of the
current
> > function similarly.) Hopefully you then wouldn't need to update any
additional
> > *-expected.txt files.
> 
> I added a new function "isClosedCaptionsButtonVisible" and modified the test
accordingly. I hope this is what you mean by "predicate".

Almost. What I'm imagining would return a boolean result, and then you'd use at
as:

assert_false/true(isClosedCaptionsButtonVisible(myMediaElement));

maybe it won't be cleanly reusable in testClosedCaptionsButtonVisibility though,
but I guess we can work on phasing that one out.

[foolip, 2016-05-25 10:34:17]

foolip@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2016-05-25 10:34:17]

Just a heads up that this will conflict with
https://codereview.chromium.org/2015503002

[fs, 2016-05-25 10:41:17]

On 2016/05/25 at 10:34:17, foolip wrote:
> Just a heads up that this will conflict with
https://codereview.chromium.org/2015503002

This, and everything else ;-)

[foolip, 2016-05-25 11:36:31]

On 2016/05/25 10:41:17, fs wrote:
> On 2016/05/25 at 10:34:17, foolip wrote:
> > Just a heads up that this will conflict with
> https://codereview.chromium.org/2015503002
> 
> This, and everything else ;-)

I looked at Srirama's reviews, are there a bunch of other in-flight CLs I'll
break? If so I can hold off and land at a better time.

[Srirama, 2016-05-25 11:39:54]

On 2016/05/25 11:36:31, Philip Jägenstedt wrote:
> On 2016/05/25 10:41:17, fs wrote:
> > On 2016/05/25 at 10:34:17, foolip wrote:
> > > Just a heads up that this will conflict with
> > https://codereview.chromium.org/2015503002
> > 
> > This, and everything else ;-)
> 
> I looked at Srirama's reviews, are there a bunch of other in-flight CLs I'll
> break? If so I can hold off and land at a better time.

No, this is the only one right now, Please go ahead and land your CL.

[Srirama, 2016-05-25 12:41:03]

On 2016/05/25 10:26:19, fs wrote:
> On 2016/05/25 at 09:52:45, srirama.m wrote:
> > On 2016/05/25 07:57:37, fs wrote:
> > > On 2016/05/25 at 07:26:30, srirama.m wrote:
> > > > On 2016/05/24 14:06:20, fs wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > > > File third_party/WebKit/LayoutTests/media/audio-controls-captions.html
> > > (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/2004963003/diff/20001/third_party/WebKit/Layo...
> > > > > third_party/WebKit/LayoutTests/media/audio-controls-captions.html:20:
> > > > > audio.onloadedmetadata =
> > > > > t.step_func_done(testClosedCaptionsButtonVisibility(false));
> > > > > On 2016/05/24 at 13:41:28, Srirama wrote:
> > > > > > On 2016/05/24 13:32:23, fs wrote:
> > > > > > > This doesn't look right, the argument ought to be as before, or
> maybe
> > > using
> > > > > > > bind.
> > > > > > 
> > > > > > you mean it should be
> > > > > > ..... = t.step_func_done(function() {
> > > > > > testClosedCaptionsButtonVisibility(false));
> > > > > > });
> > > > > > ?
> > > > > > 
> > > > > > Even directly calling a function inside it, is working fine. I just
> > > changed
> > > > > "false" to "true" and it fails the test. Will there be a problem with
> this?
> > > > > 
> > > > > It's confusing as to what ordering is required. I think the intention
of
> the
> > > > > test is to check that the 'CC' button isn't visible after the media
file
> is
> > > > > sufficiently loaded. With the changes to track selection I think this
> passes
> > > > > trivially though.. (i.e the <track> should have a 'default' attribute
> added
> > > to
> > > > > be more reasonable.)
> > > > > 
> > > > > testClosedCaptionsButtonVisibility appears to be using testExpected
> though
> > > too,
> > > > > how is that handled?
> > > > 
> > > > Sorry actually there are more problems here, inside
> > > testClosedCaptionsButtonVisibility it is using mediaElement which is set
> using
> > > "findMediaElement" call. But now that i removed video-test.js reference
and
> > > removed call to "findMediaElement" mediaElement is not set and the call
> > > testClosedCaptionsButtonVisibility never reaches "testExpected".
> > > > I think i have to do some work in testClosedCaptionsButtonVisibility
> function
> > > before i can covert these tests.
> > > > So should i clean up testClosedCaptionsButtonVisibility to remove
> testExpected
> > > and mediaElement usage from it and update all the effected *-expected.txt
> files
> > > and then come to these test cases? Is it the right approach or is there
any
> > > better way?
> > > 
> > > One way could be to split out the "predicate" from
> > > testClosedCaptionsButtonVisibility (maybe isClosedCaptionsButtonVisible or
> so)
> > > and then use that predicate here (and phase out any other uses of the
> current
> > > function similarly.) Hopefully you then wouldn't need to update any
> additional
> > > *-expected.txt files.
> > 
> > I added a new function "isClosedCaptionsButtonVisible" and modified the test
> accordingly. I hope this is what you mean by "predicate".
> 
> Almost. What I'm imagining would return a boolean result, and then you'd use
at
> as:
> 
> assert_false/true(isClosedCaptionsButtonVisible(myMediaElement));
> 
> maybe it won't be cleanly reusable in testClosedCaptionsButtonVisibility
though,
> but I guess we can work on phasing that one out.

Done, Moved assertion logic out of isClosedCaptionsButtonVisible.

[fs, 2016-05-25 13:07:30]

LGTM w/ the below fixed

https://codereview.chromium.org/2004963003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/media-controls.js (right):

https://codereview.chromium.org/2004963003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/media-controls.js:104:
captionsButtonElement = mediaControlsButton(currentMediaElement,
"toggle-closed-captions-button");
Make these local to the function too ('var captions...') to avoid the
side-effect of setting globals.

[Srirama, 2016-05-25 14:10:26]

@Philip, do you want to land your CL before this?

[foolip, 2016-05-25 14:32:45]

On 2016/05/25 14:10:26, Srirama wrote:
> @Philip, do you want to land your CL before this?

I was waiting for another LG2M, it's in CQ now.

[foolip, 2016-05-25 14:32:47]

On 2016/05/25 14:10:26, Srirama wrote:
> @Philip, do you want to land your CL before this?

I was waiting for another LG2M, it's in CQ now.

[Srirama, 2016-05-25 15:14:53]

On 2016/05/25 14:32:47, Philip Jägenstedt wrote:
> On 2016/05/25 14:10:26, Srirama wrote:
> > @Philip, do you want to land your CL before this?
> 
> I was waiting for another LG2M, it's in CQ now.

Yes, saw that you got 2nd LGTM, so i pinged :). I have rebased and sending it to
CQ now.

[Srirama, 2016-05-25 15:15:04]

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

[Srirama, 2016-05-25 15:15:04]

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/2004963003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-25 15:15:27]

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

[commit-bot: I haz the power, 2016-05-25 16:27:27]

Description was changed from

==========
Convert audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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 audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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-25 16:27:28]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-05-25 16:30:18]

Description was changed from

==========
Convert audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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 audio-controls* and audio-delete* tests to testharness.js

Cleaning up audio-controls* and audio-delete* 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/ceb895e19332cab2b012148c5319bb6eddae7a0b
Cr-Commit-Position: refs/heads/master@{#395908}
==========

[commit-bot: I haz the power, 2016-05-25 16:30:19]

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