1278748 - [webvtt] Fix the fail for "webvtt/webvtt-file-format-parsing/webvtt-file-parsing/001.html"

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Alastor Wu [:alwu]]

Need to pass more tests!

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part1 : cue's attribute can be set multiple times.

Review commit: https://reviewboard.mozilla.org/r/61194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61194/

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part2 : correct default value of webidl's property.

Review commit: https://reviewboard.mozilla.org/r/61196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61196/

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part3 : change align from 'middle' to 'center'.

Review commit: https://reviewboard.mozilla.org/r/61198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61198/

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

Review commit: https://reviewboard.mozilla.org/r/61200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61200/

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part5 : the value of position and line are double.

Review commit: https://reviewboard.mozilla.org/r/61202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61202/

Comment 6

[Alastor Wu [:alwu]]

This file still has 9 fails, I'm working on that.

Comment 7

[Alastor Wu [:alwu]]

The remaining fails are related with the parsing algorithm, it's little complex.
Therefore, I open the bug1283803 to solve it independently.

Comment 8

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7efcc469411

Comment 9

[Alastor Wu [:alwu]]

There are some mistakes in the parsing rule part in the spec, so I'm discussing it [1] with the spec's author.

[1] https://github.com/w3c/webvtt/issues/310

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part6 : modify timestamp parsing rule.

Review commit: https://reviewboard.mozilla.org/r/62338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62338/

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8766205 [details]
Bug 1278748 - part1 : cue's attribute can be set multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61194/diff/1-2/

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8766206 [details]
Bug 1278748 - part2 : correct default value of webidl's property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61196/diff/1-2/

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8766207 [details]
Bug 1278748 - part3 : change align from 'middle' to 'center'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61198/diff/1-2/

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8766208 [details]
Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61200/diff/1-2/

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8766209 [details]
Bug 1278748 - part5 : the value of position and line are double.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61202/diff/1-2/

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 17

[Alastor Wu [:alwu]]

Attachment: Bug 1278748 - part7 : vertical tab isn't a space characters.

Definition of the space character : https://html.spec.whatwg.org/multipage/infrastructure.html#space-character

Review commit: https://reviewboard.mozilla.org/r/62598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62598/

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8766205 [details]
Bug 1278748 - part1 : cue's attribute can be set multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61194/diff/2-3/

Comment 19

[Alastor Wu [:alwu]]

Comment on attachment 8766206 [details]
Bug 1278748 - part2 : correct default value of webidl's property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61196/diff/2-3/

Comment 20

[Alastor Wu [:alwu]]

Comment on attachment 8766207 [details]
Bug 1278748 - part3 : change align from 'middle' to 'center'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61198/diff/2-3/

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8766208 [details]
Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61200/diff/2-3/

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8766209 [details]
Bug 1278748 - part5 : the value of position and line are double.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61202/diff/2-3/

Comment 23

[Alastor Wu [:alwu]]

Comment on attachment 8767939 [details]
Bug 1278748 - part6 : modify timestamp parsing rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62338/diff/1-2/

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8766205 [details]
Bug 1278748 - part1 : cue's attribute can be set multiple times.

https://reviewboard.mozilla.org/r/61194/#review59536

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 8766206 [details]
Bug 1278748 - part2 : correct default value of webidl's property.

https://reviewboard.mozilla.org/r/61196/#review59540

Comment 26

[Ralph Giles (:rillian)]

Comment on attachment 8766207 [details]
Bug 1278748 - part3 : change align from 'middle' to 'center'.

https://reviewboard.mozilla.org/r/61198/#review59542

Comment 27

[Ralph Giles (:rillian)]

Comment on attachment 8766208 [details]
Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

https://reviewboard.mozilla.org/r/61200/#review59544

Comment 28

[Ralph Giles (:rillian)]

Comment on attachment 8766209 [details]
Bug 1278748 - part5 : the value of position and line are double.

https://reviewboard.mozilla.org/r/61202/#review59546

Comment 29

[Ralph Giles (:rillian)]

Comment on attachment 8767939 [details]
Bug 1278748 - part6 : modify timestamp parsing rule.

https://reviewboard.mozilla.org/r/62338/#review59556

::: dom/media/webvtt/vtt.jsm:77
(Diff revision 2)
> +      }
>        return (h | 0) * 3600 + (m | 0) * 60 + (s | 0) + (f | 0) / 1000;
>      }
>  
> -    var m = input.match(/^(\d+):(\d{2})(:\d{2})?\.(\d{3})/);
> -    if (!m) {
> +    var timestamp = input.match(/^(\d+:)?\d{2}:\d{2}\.\d+/);
> +    if (!timestamp) {

Do you need to capture the first stanza here? I don't see `timestamp[1]` referenced.

Might be worth capturing the milliseconds field though, so you can use the regexp to do the work of the later split to check the length.

Comment 30

[Ralph Giles (:rillian)]

Comment on attachment 8768301 [details]
Bug 1278748 - part7 : vertical tab isn't a space characters.

https://reviewboard.mozilla.org/r/62598/#review59564

Nice that the test caught this error!

Comment 31

[Alastor Wu [:alwu]]

Comment on attachment 8766205 [details]
Bug 1278748 - part1 : cue's attribute can be set multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61194/diff/3-4/

Comment 32

[Alastor Wu [:alwu]]

Comment on attachment 8766206 [details]
Bug 1278748 - part2 : correct default value of webidl's property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61196/diff/3-4/

Comment 33

[Alastor Wu [:alwu]]

Comment on attachment 8766207 [details]
Bug 1278748 - part3 : change align from 'middle' to 'center'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61198/diff/3-4/

Comment 34

[Alastor Wu [:alwu]]

Comment on attachment 8766208 [details]
Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61200/diff/3-4/

Comment 35

[Alastor Wu [:alwu]]

Comment on attachment 8766209 [details]
Bug 1278748 - part5 : the value of position and line are double.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61202/diff/3-4/

Comment 36

[Alastor Wu [:alwu]]

Comment on attachment 8767939 [details]
Bug 1278748 - part6 : modify timestamp parsing rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62338/diff/2-3/

Comment 37

[Alastor Wu [:alwu]]

Comment on attachment 8768301 [details]
Bug 1278748 - part7 : vertical tab isn't a space characters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62598/diff/1-2/

Comment 38

[Alastor Wu [:alwu]]

Just for record, we fix 17 web-platform-tests in this bug.

Comment 39

[Ralph Giles (:rillian)]

Comment on attachment 8767939 [details]
Bug 1278748 - part6 : modify timestamp parsing rule.

https://reviewboard.mozilla.org/r/62338/#review60488

Thanks for fixing all this up. 14 new tests passing is really good!

::: dom/media/webvtt/vtt.jsm:81
(Diff revision 3)
> +      }
>        return (h | 0) * 3600 + (m | 0) * 60 + (s | 0) + (f | 0) / 1000;
>      }
>  
> -    var m = input.match(/^(\d+):(\d{2})(:\d{2})?\.(\d{3})/);
> -    if (!m) {
> +    var timestamp = input.match(/^((\d+:)?(\d{2}):(\d{2}))\.(\d+)/);
> +    if (!timestamp || timestamp.length !== 6) {

That's better, thanks. The logic of the timestamp[1] capture is a little non-obvious, but I'm not sure a comment helps with a regex unless it's a whole paragraph explaining the technique.

Comment 40

[Alastor Wu [:alwu]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #39)
> That's better, thanks. The logic of the timestamp[1] capture is a little
> non-obvious, but I'm not sure a comment helps with a regex unless it's a
> whole paragraph explaining the technique.

Ah, I think we could modify the regex not to capture the timestamp[1]. 
Thanks!

Comment 41

[Alastor Wu [:alwu]]

Comment on attachment 8766205 [details]
Bug 1278748 - part1 : cue's attribute can be set multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61194/diff/4-5/

Comment 42

[Alastor Wu [:alwu]]

Comment on attachment 8766206 [details]
Bug 1278748 - part2 : correct default value of webidl's property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61196/diff/4-5/

Comment 43

[Alastor Wu [:alwu]]

Comment on attachment 8766207 [details]
Bug 1278748 - part3 : change align from 'middle' to 'center'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61198/diff/4-5/

Comment 44

[Alastor Wu [:alwu]]

Comment on attachment 8766208 [details]
Bug 1278748 - part4 : should not throw error if the correct signature doesn't follow with new line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61200/diff/4-5/

Comment 45

[Alastor Wu [:alwu]]

Comment on attachment 8766209 [details]
Bug 1278748 - part5 : the value of position and line are double.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61202/diff/4-5/

Comment 46

[Alastor Wu [:alwu]]

Comment on attachment 8767939 [details]
Bug 1278748 - part6 : modify timestamp parsing rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62338/diff/3-4/

Comment 47

[Alastor Wu [:alwu]]

Comment on attachment 8768301 [details]
Bug 1278748 - part7 : vertical tab isn't a space characters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62598/diff/2-3/

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd048078af43
part1 : cue's attribute can be set multiple times. r=rillian
https://hg.mozilla.org/integration/autoland/rev/7c48311a55d2
part2 : correct default value of webidl's property. r=rillian
https://hg.mozilla.org/integration/autoland/rev/c32d76936446
part3 : change align from 'middle' to 'center'. r=rillian
https://hg.mozilla.org/integration/autoland/rev/59c445c3e604
part4 : should not throw error if the correct signature doesn't follow with new line. r=rillian
https://hg.mozilla.org/integration/autoland/rev/2905747bae6e
part5 : the value of position and line are double. r=rillian
https://hg.mozilla.org/integration/autoland/rev/3414ed32d520
part6 : modify timestamp parsing rule. r=rillian
https://hg.mozilla.org/integration/autoland/rev/9b1032ae8fac
part7 : vertical tab isn't a space characters. r=rillian

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd048078af43
https://hg.mozilla.org/mozilla-central/rev/7c48311a55d2
https://hg.mozilla.org/mozilla-central/rev/c32d76936446
https://hg.mozilla.org/mozilla-central/rev/59c445c3e604
https://hg.mozilla.org/mozilla-central/rev/2905747bae6e
https://hg.mozilla.org/mozilla-central/rev/3414ed32d520
https://hg.mozilla.org/mozilla-central/rev/9b1032ae8fac