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

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

Cleaning up text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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/d57bcc2418a2151fb7638db43291bfe8d9bc81d2
Cr-Commit-Position: refs/heads/master@{#388765}

[Srirama, 2016-04-12 13:59:27]

Description was changed from

==========
fix

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

to

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

Cleaning up text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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-12 14:09:21]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-04-12 14:09:45]

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

[Srirama, 2016-04-12 14:09:45]

PTAL

[mlamouri (slow - plz ping), 2016-04-13 12:58:30]

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html:48:
video.currentTime = 0.1;
Why isn't that no longer inside 'canplaytrough' callback?

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:25:
video.src = findMediaFile("video", "../content/test");
The test used to wait for the load event and set currentTime to 0.1. I'm not
sure why it used to do that. Do you know why?

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-internal-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-internal-settings.html:13:
video.src = findMediaFile("video", "../content/test");
ditto

[Srirama, 2016-04-13 13:19:46]

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html:48:
video.currentTime = 0.1;
On 2016/04/13 12:58:30, Mounir Lamouri wrote:
> Why isn't that no longer inside 'canplaytrough' callback?

Previously (may be 2years old code) to seek video(set current time) we need to
wait for canplaythrough event. But lot of code has changed since then and we no
longer need to wait for canplaythrough event to set current time.

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:25:
video.src = findMediaFile("video", "../content/test");
On 2016/04/13 12:58:30, Mounir Lamouri wrote:
> The test used to wait for the load event and set currentTime to 0.1. I'm not
> sure why it used to do that. Do you know why?

Previously we are calling findMediaElement() to get the "video" element and then
set the "src". This requires window onload event. But now that i have moved
"video" tag before this script, window would have already loaded before this
script gets executed and we need not wait for onload event specifically.

[philipj_slow, 2016-04-15 14:04:50]

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html:48:
video.currentTime = 0.1;
On 2016/04/13 13:19:46, Srirama wrote:
> On 2016/04/13 12:58:30, Mounir Lamouri wrote:
> > Why isn't that no longer inside 'canplaytrough' callback?
> 
> Previously (may be 2years old code) to seek video(set current time) we need to
> wait for canplaythrough event. But lot of code has changed since then and we
no
> longer need to wait for canplaythrough event to set current time.

Right, this is now handled by m_defaultPlaybackStartPosition (which is per spec)

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:25:
video.src = findMediaFile("video", "../content/test");
On 2016/04/13 13:19:46, Srirama wrote:
> On 2016/04/13 12:58:30, Mounir Lamouri wrote:
> > The test used to wait for the load event and set currentTime to 0.1. I'm not
> > sure why it used to do that. Do you know why?
> 
> Previously we are calling findMediaElement() to get the "video" element and
then
> set the "src". This requires window onload event. But now that i have moved
> "video" tag before this script, window would have already loaded before this
> script gets executed and we need not wait for onload event specifically.

AFAICT, it was just because the video element was after the script tag, now it's
before and since there's no resource block the load event here, the timing will
be almost exactly the same.

(In some tests with multiple media elements, the window load event can be used
to make sure all have reached HAVE_CURRENT_DATA, because "Once the readyState
attribute reaches HAVE_CURRENT_DATA, after the loadeddata event has been fired,
set the element's delaying-the-load-event flag to false. This stops delaying the
load event." Not here.)

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:39:
if (internals) {
Need to say window.internals, otherwise it'll throw. I think just removing this
check is fine, because the asserts that follow will fail if the code isn't run
anyway.

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:49:
video.currentTime = 0.3;
Does the test still pass without this? There's a new cue at 0.3 (compared to 0),
but AFAICT the style above doesn't actually target any of the classes in that
cue...

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

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:11:
assert_not_equals(fragment, null);
Maybe assert fragment instanceof DocumentFragment instead?

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:16:
assert_equals(fragment.childNodes[0].length, 0);
maybe assert_equals(fragment.childNodes[0].data, "")? If it did fail, the
message would tell you what the string actually is then.

[Srirama, 2016-04-16 06:44:32]

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:39:
if (internals) {
On 2016/04/15 14:04:50, philipj_slow wrote:
> Need to say window.internals, otherwise it'll throw. I think just removing
this
> check is fine, because the asserts that follow will fail if the code isn't run
> anyway.

It is working without "window." prefix as well, I guess it is because window is
a global object and it is same like how we use "setInterval" "alert"
functions...
Anyway i have removed the condition.

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:49:
video.currentTime = 0.3;
On 2016/04/15 14:04:50, philipj_slow wrote:
> Does the test still pass without this? There's a new cue at 0.3 (compared to
0),
> but AFAICT the style above doesn't actually target any of the classes in that
> cue...

It doesn't pass without this, so i kept it. It doesn't apply the user settings
without this line. I did a bit of code walkthrough and found that the these
settings are applied only when cue display is updated (VTTCue::getDisplayTree)
which in turn is triggered from HTMLMediaElement and TextTrackContainer.
So this line is updating the cue display which inturn updates user css
properties.
Is it a bug that it doesn't apply the properties without some display trigger
point?

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

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:11:
assert_not_equals(fragment, null);
On 2016/04/15 14:04:50, philipj_slow wrote:
> Maybe assert fragment instanceof DocumentFragment instead?

Done.

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:16:
assert_equals(fragment.childNodes[0].length, 0);
On 2016/04/15 14:04:50, philipj_slow wrote:
> maybe assert_equals(fragment.childNodes[0].data, "")? If it did fail, the
> message would tell you what the string actually is then.

Done. So you want it as additional condition? Or you want to replace any of the
above two conditions?

[Srirama, 2016-04-21 10:22:51]

On 2016/04/16 06:44:32, Srirama wrote:
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
> (right):
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:39:
> if (internals) {
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > Need to say window.internals, otherwise it'll throw. I think just removing
> this
> > check is fine, because the asserts that follow will fail if the code isn't
run
> > anyway.
> 
> It is working without "window." prefix as well, I guess it is because window
is
> a global object and it is same like how we use "setInterval" "alert"
> functions...
> Anyway i have removed the condition.
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:49:
> video.currentTime = 0.3;
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > Does the test still pass without this? There's a new cue at 0.3 (compared to
> 0),
> > but AFAICT the style above doesn't actually target any of the classes in
that
> > cue...
> 
> It doesn't pass without this, so i kept it. It doesn't apply the user settings
> without this line. I did a bit of code walkthrough and found that the these
> settings are applied only when cue display is updated (VTTCue::getDisplayTree)
> which in turn is triggered from HTMLMediaElement and TextTrackContainer.
> So this line is updating the cue display which inturn updates user css
> properties.
> Is it a bug that it doesn't apply the properties without some display trigger
> point?
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html
> (right):
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:11:
> assert_not_equals(fragment, null);
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > Maybe assert fragment instanceof DocumentFragment instead?
> 
> Done.
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/media/track/track-cue-empty-crash.html:16:
> assert_equals(fragment.childNodes[0].length, 0);
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > maybe assert_equals(fragment.childNodes[0].data, "")? If it did fail, the
> > message would tell you what the string actually is then.
> 
> Done. So you want it as additional condition? Or you want to replace any of
the
> above two conditions?

friendly ping.

[philipj_slow, 2016-04-21 11:38:10]

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:39:
if (internals) {
On 2016/04/16 06:44:32, Srirama wrote:
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > Need to say window.internals, otherwise it'll throw. I think just removing
> this
> > check is fine, because the asserts that follow will fail if the code isn't
run
> > anyway.
> 
> It is working without "window." prefix as well, I guess it is because window
is
> a global object and it is same like how we use "setInterval" "alert"
> functions...
> Anyway i have removed the condition.

It works as long as window.internals exists, but the point of the check is to
avoid throwing an exception if it doesn't exist. Then this would throw
"ReferenceError: internals is not defined" instead. So, I suggest not doing the
check and letting an exception be thrown.

https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:49:
video.currentTime = 0.3;
On 2016/04/16 06:44:32, Srirama wrote:
> On 2016/04/15 14:04:50, philipj_slow wrote:
> > Does the test still pass without this? There's a new cue at 0.3 (compared to
> 0),
> > but AFAICT the style above doesn't actually target any of the classes in
that
> > cue...
> 
> It doesn't pass without this, so i kept it. It doesn't apply the user settings
> without this line. I did a bit of code walkthrough and found that the these
> settings are applied only when cue display is updated (VTTCue::getDisplayTree)
> which in turn is triggered from HTMLMediaElement and TextTrackContainer.
> So this line is updating the cue display which inturn updates user css
> properties.
> Is it a bug that it doesn't apply the properties without some display trigger
> point?

Well it does seek to another cue, so this line does something. It was already
like this, so OK to leave like this without further investigation.

[Srirama, 2016-04-21 11:48:27]

On 2016/04/21 11:38:10, philipj_slow wrote:
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html
> (right):
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:39:
> if (internals) {
> On 2016/04/16 06:44:32, Srirama wrote:
> > On 2016/04/15 14:04:50, philipj_slow wrote:
> > > Need to say window.internals, otherwise it'll throw. I think just removing
> > this
> > > check is fine, because the asserts that follow will fail if the code isn't
> run
> > > anyway.
> > 
> > It is working without "window." prefix as well, I guess it is because window
> is
> > a global object and it is same like how we use "setInterval" "alert"
> > functions...
> > Anyway i have removed the condition.
> 
> It works as long as window.internals exists, but the point of the check is to
> avoid throwing an exception if it doesn't exist. Then this would throw
> "ReferenceError: internals is not defined" instead. So, I suggest not doing
the
> check and letting an exception be thrown.
> 
>
https://codereview.chromium.org/1882583002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html:49:
> video.currentTime = 0.3;
> On 2016/04/16 06:44:32, Srirama wrote:
> > On 2016/04/15 14:04:50, philipj_slow wrote:
> > > Does the test still pass without this? There's a new cue at 0.3 (compared
to
> > 0),
> > > but AFAICT the style above doesn't actually target any of the classes in
> that
> > > cue...
> > 
> > It doesn't pass without this, so i kept it. It doesn't apply the user
settings
> > without this line. I did a bit of code walkthrough and found that the these
> > settings are applied only when cue display is updated
(VTTCue::getDisplayTree)
> > which in turn is triggered from HTMLMediaElement and TextTrackContainer.
> > So this line is updating the cue display which inturn updates user css
> > properties.
> > Is it a bug that it doesn't apply the properties without some display
trigger
> > point?
> 
> Well it does seek to another cue, so this line does something. It was already
> like this, so OK to leave like this without further investigation.

Thanks @philip for your detailed explanation. Is there anything pending in this
before it can goto CQ?

[philipj_slow, 2016-04-21 12:25:31]

On 2016/04/21 11:48:27, Srirama wrote:

> Thanks @philip for your detailed explanation. Is there anything pending in
this
> before it can goto CQ?

LGTM if you drop "if (internals)"

[Srirama, 2016-04-21 12:30:24]

On 2016/04/21 12:25:31, philipj_slow wrote:
> On 2016/04/21 11:48:27, Srirama wrote:
> 
> > Thanks @philip for your detailed explanation. Is there anything pending in
> this
> > before it can goto CQ?
> 
> LGTM if you drop "if (internals)"

It was already done in patchset2, so sending it to CQ.

[mlamouri (slow - plz ping), 2016-04-21 12:30:30]

lgtm assuming philipj@ is fine with it too.

[Srirama, 2016-04-21 12:30:34]

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

[commit-bot: I haz the power, 2016-04-21 12:30:43]

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

[philipj_slow, 2016-04-21 12:45:40]

On 2016/04/21 12:30:24, Srirama wrote:
> On 2016/04/21 12:25:31, philipj_slow wrote:
> > On 2016/04/21 11:48:27, Srirama wrote:
> > 
> > > Thanks @philip for your detailed explanation. Is there anything pending in
> > this
> > > before it can goto CQ?
> > 
> > LGTM if you drop "if (internals)"
> 
> It was already done in patchset2, so sending it to CQ.

Ah, sorry :)

[commit-bot: I haz the power, 2016-04-21 14:35:05]

Description was changed from

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

Cleaning up text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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 text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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-21 14:35:06]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:34:26]

Description was changed from

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

Cleaning up text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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 text-css-property*, track-css-user*, track-cue-container*
and track-cue-empty* 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/d57bcc2418a2151fb7638db43291bfe8d9bc81d2
Cr-Commit-Position: refs/heads/master@{#388765}
==========

[commit-bot: I haz the power, 2016-04-22 19:34:27]

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