Convert track-webvtt-tc[024-026] tests to testharness.js

Convert track-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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/34fe887b69a885a7fad103136ed7015f9c1a94ac
Cr-Commit-Position: refs/heads/master@{#399394}

[Srirama, 2016-06-10 11:51:15]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-06-10 11:52:07]

Description was changed from

==========
fix


fix

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

to

==========
Convert track-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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-06-10 11:53:50]

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

[Srirama, 2016-06-10 11:53:51]

PTAL

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:53: function
assert_cue_fragment_with_style(cue, children, style) {
Should we merge this with "assert_cue_fragment" with a null check for style or
separate function is fine?

[fs, 2016-06-10 12:52:19]

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:48: function
assert_cue_fragment_text(cue, children) {
Maybe '...as_text' or '...as_textcontent'?

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:53: function
assert_cue_fragment_with_style(cue, children, style) {
On 2016/06/10 at 11:53:51, Srirama wrote:
> Should we merge this with "assert_cue_fragment" with a null check for style or
separate function is fine?

Merging sounds like a good idea. Maybe even make it so that style can more
easily be applied "per node" (ought to be possible to just move the for-of loop
to cloneChildrenToFragment and specify in the same dictionary as 'type' and
'value'.)

[Srirama, 2016-06-10 13:57:04]

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:53: function
assert_cue_fragment_with_style(cue, children, style) {
On 2016/06/10 12:52:18, fs wrote:
> On 2016/06/10 at 11:53:51, Srirama wrote:
> > Should we merge this with "assert_cue_fragment" with a null check for style
or
> separate function is fine?
> 
> Merging sounds like a good idea. Maybe even make it so that style can more
> easily be applied "per node" (ought to be possible to just move the for-of
loop
> to cloneChildrenToFragment and specify in the same dictionary as 'type' and
> 'value'.)

"Maybe even make it so that style can more" is some word missing after *make
it*?

[fs, 2016-06-10 14:04:38]

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:53: function
assert_cue_fragment_with_style(cue, children, style) {
On 2016/06/10 at 13:57:04, Srirama wrote:
> On 2016/06/10 12:52:18, fs wrote:
> > On 2016/06/10 at 11:53:51, Srirama wrote:
> > > Should we merge this with "assert_cue_fragment" with a null check for
style or
> > separate function is fine?
> > 
> > Merging sounds like a good idea. Maybe even make it so that style can more
> > easily be applied "per node" (ought to be possible to just move the for-of
loop
> > to cloneChildrenToFragment and specify in the same dictionary as 'type' and
> > 'value'.)
> 
> "Maybe even make it so that style can more" is some word missing after *make
it*?

No =). 'it' refers to the (helper) function. Does s/make/change/ make it any
more clear for you?

[Srirama, 2016-06-10 15:19:09]

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:48: function
assert_cue_fragment_text(cue, children) {
On 2016/06/10 12:52:18, fs wrote:
> Maybe '...as_text' or '...as_textcontent'?

Done.

https://codereview.chromium.org/2051233004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:53: function
assert_cue_fragment_with_style(cue, children, style) {
On 2016/06/10 14:04:38, fs wrote:
> On 2016/06/10 at 13:57:04, Srirama wrote:
> > On 2016/06/10 12:52:18, fs wrote:
> > > On 2016/06/10 at 11:53:51, Srirama wrote:
> > > > Should we merge this with "assert_cue_fragment" with a null check for
> style or
> > > separate function is fine?
> > > 
> > > Merging sounds like a good idea. Maybe even make it so that style can more
> > > easily be applied "per node" (ought to be possible to just move the for-of
> loop
> > > to cloneChildrenToFragment and specify in the same dictionary as 'type'
and
> > > 'value'.)
> > 
> > "Maybe even make it so that style can more" is some word missing after *make
> it*?
> 
> No =). 'it' refers to the (helper) function. Does s/make/change/ make it any
> more clear for you?

Understood :)

[fs, 2016-06-11 18:04:18]

Almost =)

https://codereview.chromium.org/2051233004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:64: } else if
(child.type == "style") {
I was actually thinking more like:

...
{ type: "span", style: { className: "..." }, value: [ ... ] }
...

and then here (in the "else" of type == "text"):

var styles = child.style || {};
for (var attr of Object.getOwnPropertyNames(styles))
    childElement[attr] = styles[attr];

[Srirama, 2016-06-12 06:05:37]

Patchset #3 (id:60001) has been deleted

[Srirama, 2016-06-12 06:06:17]

https://codereview.chromium.org/2051233004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/track/track-helpers.js (right):

https://codereview.chromium.org/2051233004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-helpers.js:64: } else if
(child.type == "style") {
On 2016/06/11 18:04:18, fs wrote:
> I was actually thinking more like:
> 
> ...
> { type: "span", style: { className: "..." }, value: [ ... ] }
> ...
> 
> and then here (in the "else" of type == "text"):
> 
> var styles = child.style || {};
> for (var attr of Object.getOwnPropertyNames(styles))
>     childElement[attr] = styles[attr];

Done.

[fs, 2016-06-12 13:49:07]

LGTM w/ nits

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc024-timestamp.html
(right):

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc024-timestamp.html:10:
// TODO(srirama.m): Timestamps are handled as ProcessingInstructions,
Comment for future improvements: Would Document.createProcessingInstruction
(https://dom.spec.whatwg.org/#dom-document-createprocessinginstruction) work
here? Should work on HTML documents too nowadays I think.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html
(right):

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:20:
assert_cue_fragment(track.cues[1], children, { className: "green" });
Nit: Third argument no longer used.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:28:
assert_cue_fragment(track.cues[2], children, { className: "red uppercase" });
Ditto.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:45:
assert_cue_fragment(track.cues[1], children, { className: "red&large" });
Ditto.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:53:
assert_cue_fragment(track.cues[2], children, { className: "9red upper+case" });
Ditto.

[Srirama, 2016-06-12 15:19:48]

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc024-timestamp.html
(right):

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc024-timestamp.html:10:
// TODO(srirama.m): Timestamps are handled as ProcessingInstructions,
On 2016/06/12 13:49:06, fs wrote:
> Comment for future improvements: Would Document.createProcessingInstruction
> (https://dom.spec.whatwg.org/#dom-document-createprocessinginstruction) work
> here? Should work on HTML documents too nowadays I think. 

Acknowledged.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html
(right):

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:20:
assert_cue_fragment(track.cues[1], children, { className: "green" });
On 2016/06/12 13:49:07, fs wrote:
> Nit: Third argument no longer used.

Done.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:28:
assert_cue_fragment(track.cues[2], children, { className: "red uppercase" });
On 2016/06/12 13:49:06, fs wrote:
> Ditto.

Done.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:45:
assert_cue_fragment(track.cues[1], children, { className: "red&large" });
On 2016/06/12 13:49:07, fs wrote:
> Ditto.

Done.

https://codereview.chromium.org/2051233004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc025-class-markup.html:53:
assert_cue_fragment(track.cues[2], children, { className: "9red upper+case" });
On 2016/06/12 13:49:07, fs wrote:
> Ditto.

Done.

[Srirama, 2016-06-12 15:20:28]

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

[Srirama, 2016-06-12 15:20:29]

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/2051233004/#ps100001
(title: "address nits")

[commit-bot: I haz the power, 2016-06-12 15:20:33]

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

[commit-bot: I haz the power, 2016-06-12 17:43:58]

Description was changed from

==========
Convert track-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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-06-12 17:43:59]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-12 17:45:36]

Description was changed from

==========
Convert track-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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-webvtt-tc[024-026] tests to testharness.js

Cleaning up track-webvtt-tc[024-026] 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/34fe887b69a885a7fad103136ed7015f9c1a94ac
Cr-Commit-Position: refs/heads/master@{#399394}
==========

[commit-bot: I haz the power, 2016-06-12 17:45:37]

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