1276832 - [webvtt] VTTCue's line should be auto or double

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

https://w3c.github.io/webvtt/#vttcue

Now we return "long" or "auto", but it's wrong.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1276832 - part1 : modify VTTCue's line to double.

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

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1276832 - part2 : modify vtt.jsm.

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

Comment 3

[Alastor Wu [:alwu]]

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

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1276832 - part3 : modify test.

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

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8759621 [details]
Bug 1276832 - part1 : modify VTTCue's line to double.

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

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8759622 [details]
Bug 1276832 - part2 : modify vtt.jsm.

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8759621 [details]
Bug 1276832 - part1 : modify VTTCue's line to double.

https://reviewboard.mozilla.org/r/57562/#review54732

r+ for the .webidl

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8759621 [details]
Bug 1276832 - part1 : modify VTTCue's line to double.

https://reviewboard.mozilla.org/r/57562/#review56048

r=me with issue addressed.

::: dom/media/TextTrackCue.cpp:183
(Diff revision 2)
> +      (mLine < 0.0 || mLine > 100.0)) {
> +    return 100.0;
> +  } else if (!mLineIsAutoKeyword) {
> +    return mLine;
> +  } else if (mLineIsAutoKeyword) {
> +    return 100.0;

This should be `if (!mSnapToLines)`, I think. otherwise the rest of the code is unreachable.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8759622 [details]
Bug 1276832 - part2 : modify vtt.jsm.

https://reviewboard.mozilla.org/r/57564/#review56050

I don't mind the change, bug I am curious. Why did you move the computed line function into C++ from Javascript? If vtt.js is a general implementation of WebVTT this seems a useful thing to include.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8760111 [details]
Bug 1276832 - part3 : modify test.

https://reviewboard.mozilla.org/r/57822/#review56052

Comment 11

[Alastor Wu [:alwu]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #9)
> I don't mind the change, bug I am curious. Why did you move the computed
> line function into C++ from Javascript? If vtt.js is a general
> implementation of WebVTT this seems a useful thing to include.

The "computed line" is defined as cue's internal property [1], the same as "computed position", "computed position alignment". Therefore, I think it's more appropriate to move them to C++.

[1] https://w3c.github.io/webvtt/#cue-computed-line

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8759621 [details]
Bug 1276832 - part1 : modify VTTCue's line to double.

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

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8759622 [details]
Bug 1276832 - part2 : modify vtt.jsm.

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

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8760111 [details]
Bug 1276832 - part3 : modify test.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/581aeb0a9dbe
part1 : modify VTTCue's line to double. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f20d18028c
part2 : modify vtt.jsm. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa4e924a16c
part3 : modify test. r=rillian

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581aeb0a9dbe
https://hg.mozilla.org/mozilla-central/rev/c3f20d18028c
https://hg.mozilla.org/mozilla-central/rev/dfa4e924a16c