Convert webvtt regions tests from video-test.js to testharness.js based

Convert webvtt regions tests from video-test.js to testharness.js based

Cleaning up tests in media/track/regions-webvtt 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/5e5ca9d011d519d3ee29f52a1d2ebeeeff57f685
Cr-Commit-Position: refs/heads/master@{#385722}

[Srirama, 2016-04-01 09:52:47]

Description was changed from

==========
fix


fix


test

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

to

==========
Convert media tests from video-test.js to testharness.js

Cleaning up tests in media/track/regions-webvtt 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-01 09:54:00]

Description was changed from

==========
Convert media tests from video-test.js to testharness.js

Cleaning up tests in media/track/regions-webvtt to use testharness.js instead of
video-test.js.
This will enable to upstream these tests to web-platform-tests.
BUG=588956
==========

to

==========
Convert webvtt regions tests from video-test.js to testharness.js

Cleaning up tests in media/track/regions-webvtt 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-01 09:54:30]

Description was changed from

==========
Convert webvtt regions tests from video-test.js to testharness.js

Cleaning up tests in media/track/regions-webvtt to use testharness.js
instead of video-test.js. This will enable to upstream these
tests to web-platform-tests.
BUG=588956
==========

to

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

Cleaning up tests in media/track/regions-webvtt 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-01 09:58:18]

PTAL. There are two issues in two tests please let me know how can we fix them.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:26:
assert_throws(new IndexSizeError, function() { region.viewportAnchorX =
invalidPercentageValues[index]; });
Though this throw IndexSizeError object, it gives "IndexSizeError" not defined
error. Any idea what to do for this?

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:67:
assert_throws(new NotFoundError, function() {
testTrack.track.removeRegion(region); });
Though this throw NotFoundError object, it gives "NotFoundError" not defined
error. Any idea what to do for this?

[Srirama, 2016-04-01 10:07:53]

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

[fs, 2016-04-01 10:39:44]

fs@opera.com changed reviewers:
- mlamouri@chromium.org, philipj@opera.com

[fs, 2016-04-01 10:39:46]

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:26:
assert_throws(new IndexSizeError, function() { region.viewportAnchorX =
invalidPercentageValues[index]; });
On 2016/04/01 at 09:58:18, Srirama wrote:
> Though this throw IndexSizeError object, it gives "IndexSizeError" not defined
error. Any idea what to do for this?

Try "IndexSizeError".

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:67:
assert_throws(new NotFoundError, function() {
testTrack.track.removeRegion(region); });
On 2016/04/01 at 09:58:18, Srirama wrote:
> Though this throw NotFoundError object, it gives "NotFoundError" not defined
> error. Any idea what to do for this?

Same as above.

[fs, 2016-04-01 10:40:31]

fs@opera.com changed reviewers:
+ mlamouri@chromium.org, philipj@opera.com

[philipj_slow, 2016-04-01 11:07:50]

Took a peek at the first two tests only.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:6:
async_test(function (t) {
Consistency in style for functions would be nice. Here it's "function (t) {" and
below it's "function(){". I'd suggest  "function(*) {" everywhere.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:7:
assert_not_equals(window.VTTRegion, null);
Not sure why this was here, I guess to bail out early if regions weren't
supported at all. That doesn't seem necessary, I think it'd be OK to remove it
any any similar guards you find. (An exception might be if a failure would be a
timeout, but I wouldn't spend too much time tinkering with that.)

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:25:
t.done();
You can drop this by using step_func_done above.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:27:
track.src= '../captions-webvtt/header-regions.vtt';
Missing space before =

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:7:
assert_not_equals(window.VTTRegion, null);
You can drop this, the next line will throw if VTTRegion isn't there.

[Srirama, 2016-04-01 12:14:23]

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:6:
async_test(function (t) {
On 2016/04/01 11:07:49, philipj_UTC7 wrote:
> Consistency in style for functions would be nice. Here it's "function (t) {"
and
> below it's "function(){". I'd suggest  "function(*) {" everywhere.

Done.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:7:
assert_not_equals(window.VTTRegion, null);
On 2016/04/01 11:07:49, philipj_UTC7 wrote:
> Not sure why this was here, I guess to bail out early if regions weren't
> supported at all. That doesn't seem necessary, I think it'd be OK to remove it
> any any similar guards you find. (An exception might be if a failure would be
a
> timeout, but I wouldn't spend too much time tinkering with that.)

Done.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:25:
t.done();
On 2016/04/01 11:07:49, philipj_UTC7 wrote:
> You can drop this by using step_func_done above.

Done.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:27:
track.src= '../captions-webvtt/header-regions.vtt';
On 2016/04/01 11:07:49, philipj_UTC7 wrote:
> Missing space before =

Done.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:7:
assert_not_equals(window.VTTRegion, null);
On 2016/04/01 11:07:49, philipj_UTC7 wrote:
> You can drop this, the next line will throw if VTTRegion isn't there.

Done.

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:26:
assert_throws(new IndexSizeError, function() { region.viewportAnchorX =
invalidPercentageValues[index]; });
On 2016/04/01 10:39:45, fs wrote:
> On 2016/04/01 at 09:58:18, Srirama wrote:
> > Though this throw IndexSizeError object, it gives "IndexSizeError" not
defined
> error. Any idea what to do for this?
> 
> Try "IndexSizeError".

Thank you, you always come to my rescue when i get stuck :)

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:67:
assert_throws(new NotFoundError, function() {
testTrack.track.removeRegion(region); });
On 2016/04/01 10:39:45, fs wrote:
> On 2016/04/01 at 09:58:18, Srirama wrote:
> > Though this throw NotFoundError object, it gives "NotFoundError" not defined
> > error. Any idea what to do for this?
> 
> Same as above.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:42:
testTrack.onload = this.step_func(function() {
Not able to use var 't' here so i removed it everywhere and i am using 'this'.
It says t.step_func not defined, may be 't' is overridden by some other variable
somehow.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:23:
assert_true(regions instanceof VTTRegionList, 'instanceof');
should i remove these kind of gaurds also? Or its fine to have?

[philipj_slow, 2016-04-06 12:19:40]

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:42:
testTrack.onload = this.step_func(function() {
On 2016/04/01 12:14:23, Srirama wrote:
> Not able to use var 't' here so i removed it everywhere and i am using 'this'.
> It says t.step_func not defined, may be 't' is overridden by some other
variable
> somehow.

It looks like you're just missing a "t" above, try "async_test(function(t) {"

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:23:
assert_true(regions instanceof VTTRegionList, 'instanceof');
On 2016/04/01 12:14:23, Srirama wrote:
> should i remove these kind of gaurds also? Or its fine to have?

This one seems OK, but you could remove the follow assert_not_equals which is
redundant.

[philipj_slow, 2016-04-06 12:40:04]

LGTM % nits

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:7:
var cue = new VTTCue(0.0, 1.0, "sample");
While you're here, you could just use 0 and 1.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:13:
var video = document.createElement('video');
And perhaps make use of " and ' consistent in each file. Either is fine, take a
pick.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:15:
track.onload = t.step_func_done(function(t) {
You ought not need the t argument here, even if it works it just makes me wonder
why it's there :)

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:23:
for (index in invalidPercentageValues) {
You could use `for (value of invalidPercentageValues)` here and make the
following lines a bit shorter.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:15:
var crtSeekTime = 0;
Based on how it's used I guess this means "current seek time", but can you make
it seekTimeIndex or something more explicit?

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:44:
video.removeEventListener('canplaythrough', this);
I don't think this is the callback here, is it? arguments.callee might work, but
if removing the listener isn't even needed for the test, you can just drop it.
Or use oncanplaythrough and set it to null.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html:8:
var testTrack;
Looks like you can move these into the async_test at least?

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:6:
<track id="testTrack" src="captions-webvtt/captions-fast.vtt">
Given that the test is synchronous, does this really do anything?

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:9:
var testTrack;
Move in to test?

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-parser.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-parser.html:11:
testTrack.onload = t.step_func_done(function(t) {
Drop t argument. Also, is this test really stable? Seems like it doesn't wait
for the media resource to be loaded.

[Srirama, 2016-04-07 10:40:07]

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:7:
var cue = new VTTCue(0.0, 1.0, "sample");
On 2016/04/06 12:40:04, philipj wrote:
> While you're here, you could just use 0 and 1.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:13:
var video = document.createElement('video');
On 2016/04/06 12:40:04, philipj wrote:
> And perhaps make use of " and ' consistent in each file. Either is fine, take
a
> pick.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/text-track-cue-region-attribute.html:15:
track.onload = t.step_func_done(function(t) {
On 2016/04/06 12:40:04, philipj wrote:
> You ought not need the t argument here, even if it works it just makes me
wonder
> why it's there :)

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-constructor.html:23:
for (index in invalidPercentageValues) {
On 2016/04/06 12:40:04, philipj wrote:
> You could use `for (value of invalidPercentageValues)` here and make the
> following lines a bit shorter.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:15:
var crtSeekTime = 0;
On 2016/04/06 12:40:04, philipj wrote:
> Based on how it's used I guess this means "current seek time", but can you
make
> it seekTimeIndex or something more explicit?

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:42:
testTrack.onload = this.step_func(function() {
On 2016/04/06 12:19:40, philipj wrote:
> On 2016/04/01 12:14:23, Srirama wrote:
> > Not able to use var 't' here so i removed it everywhere and i am using
'this'.
> > It says t.step_func not defined, may be 't' is overridden by some other
> variable
> > somehow.
> 
> It looks like you're just missing a "t" above, try "async_test(function(t) {"

Because of this issue i have removed 't' from every place. Now i have added 't'
whereever i can but as you can see, i am using "this.step_func" below as i
cannot use 't' there even though i declare 't' here. Same is the case with one
more test (vtt-region-dom-layout.html).

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:44:
video.removeEventListener('canplaythrough', this);
On 2016/04/06 12:40:04, philipj wrote:
> I don't think this is the callback here, is it? arguments.callee might work,
but
> if removing the listener isn't even needed for the test, you can just drop it.
> Or use oncanplaythrough and set it to null.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html:8:
var testTrack;
On 2016/04/06 12:40:04, philipj wrote:
> Looks like you can move these into the async_test at least?

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:6:
<track id="testTrack" src="captions-webvtt/captions-fast.vtt">
On 2016/04/06 12:40:04, philipj wrote:
> Given that the test is synchronous, does this really do anything?

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:9:
var testTrack;
On 2016/04/06 12:40:04, philipj wrote:
> Move in to test?

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-list.html:23:
assert_true(regions instanceof VTTRegionList, 'instanceof');
On 2016/04/06 12:19:40, philipj wrote:
> On 2016/04/01 12:14:23, Srirama wrote:
> > should i remove these kind of gaurds also? Or its fine to have?
> 
> This one seems OK, but you could remove the follow assert_not_equals which is
> redundant.

Done.

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-parser.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-parser.html:11:
testTrack.onload = t.step_func_done(function(t) {
On 2016/04/06 12:40:04, philipj wrote:
> Drop t argument. Also, is this test really stable? Seems like it doesn't wait
> for the media resource to be loaded.

Done.

[philipj_slow, 2016-04-07 11:07:39]

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:42:
testTrack.onload = this.step_func(function() {
On 2016/04/07 10:40:07, Srirama wrote:
> On 2016/04/06 12:19:40, philipj wrote:
> > On 2016/04/01 12:14:23, Srirama wrote:
> > > Not able to use var 't' here so i removed it everywhere and i am using
> 'this'.
> > > It says t.step_func not defined, may be 't' is overridden by some other
> > variable
> > > somehow.
> > 
> > It looks like you're just missing a "t" above, try "async_test(function(t)
{"
> 
> Because of this issue i have removed 't' from every place. Now i have added
't'
> whereever i can but as you can see, i am using "this.step_func" below as i
> cannot use 't' there even though i declare 't' here. Same is the case with one
> more test (vtt-region-dom-layout.html).

That sounds weird, why couldn't you use t below? For consistency, maybe just use
this everywhere in this test?

https://codereview.chromium.org/1854553003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html
(right):

https://codereview.chromium.org/1854553003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html:13:
video.oncanplaythrough = this.step_func_done(function() {
This also mixes t and this, can you use just one of them?

[Srirama, 2016-04-07 11:15:52]

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html
(right):

https://codereview.chromium.org/1854553003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-display.html:42:
testTrack.onload = this.step_func(function() {
On 2016/04/07 11:07:39, philipj wrote:
> On 2016/04/07 10:40:07, Srirama wrote:
> > On 2016/04/06 12:19:40, philipj wrote:
> > > On 2016/04/01 12:14:23, Srirama wrote:
> > > > Not able to use var 't' here so i removed it everywhere and i am using
> > 'this'.
> > > > It says t.step_func not defined, may be 't' is overridden by some other
> > > variable
> > > > somehow.
> > > 
> > > It looks like you're just missing a "t" above, try "async_test(function(t)
> {"
> > 
> > Because of this issue i have removed 't' from every place. Now i have added
> 't'
> > whereever i can but as you can see, i am using "this.step_func" below as i
> > cannot use 't' there even though i declare 't' here. Same is the case with
one
> > more test (vtt-region-dom-layout.html).
> 
> That sounds weird, why couldn't you use t below? For consistency, maybe just
use
> this everywhere in this test?

Don't know the exact reason, all other test cases are using just one level of
step_func where as these two tests are using multiple levels of step_func, i
thought that may be the only reason.

https://codereview.chromium.org/1854553003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html
(right):

https://codereview.chromium.org/1854553003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/regions-webvtt/vtt-region-dom-layout.html:13:
video.oncanplaythrough = this.step_func_done(function() {
On 2016/04/07 11:07:39, philipj wrote:
> This also mixes t and this, can you use just one of them?

Done.

[philipj_slow, 2016-04-07 11:18:43]

lgtm

[Srirama, 2016-04-07 11:24:30]

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

[commit-bot: I haz the power, 2016-04-07 11:24:46]

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

[commit-bot: I haz the power, 2016-04-07 12:22:42]

Description was changed from

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

Cleaning up tests in media/track/regions-webvtt to use testharness.js
instead of video-test.js. This will enable to upstream these
tests to web-platform-tests.
BUG=588956
==========

to

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

Cleaning up tests in media/track/regions-webvtt 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-07 12:22:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 12:24:07]

Description was changed from

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

Cleaning up tests in media/track/regions-webvtt to use testharness.js
instead of video-test.js. This will enable to upstream these
tests to web-platform-tests.
BUG=588956
==========

to

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

Cleaning up tests in media/track/regions-webvtt 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/5e5ca9d011d519d3ee29f52a1d2ebeeeff57f685
Cr-Commit-Position: refs/heads/master@{#385722}
==========

[commit-bot: I haz the power, 2016-04-07 12:24:08]

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