1276830 - [webvtt] Use LineAlignSetting for VTTCue's lineAlign

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

https://w3c.github.io/webvtt/#enumdef-linealignsetting

We're using the AlignSetting for lineAlign, but it's wrong.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1276830 - part1 : introduce LineAlignSetting.

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

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1276830 - part2 : modift LineAlignSetting value in vtt.jsm.

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

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1276830 - part3 : modify tests.

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

Comment 4

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

Comment on attachment 8759106 [details]
Bug 1276830 - part1 : introduce LineAlignSetting.

https://reviewboard.mozilla.org/r/57168/#review54298

r+ for the webidl.

Still worried about all these changes which will break whoever has been using the API.

Comment 5

[Alastor Wu [:alwu]]

Maybe we won't need to worry so much about that because Chrome/Opera have already used "LineAlignSetting"? (Safari doesn't implement it yet)

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/track/vtt/VTTCue.idl?l=48

Comment 6

[Alastor Wu [:alwu]]

FYI, there are still some parts in our webidl-interface didn't follow the spec and be different with other browsers. eg. bug1276833, bug1276832, bug1276831.

Comment 7

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

yeah, I'm not as worried about this change as I'm about bug 1276130.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8759106 [details]
Bug 1276830 - part1 : introduce LineAlignSetting.

https://reviewboard.mozilla.org/r/57168/#review54562

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8759107 [details]
Bug 1276830 - part2 : modift LineAlignSetting value in vtt.jsm.

https://reviewboard.mozilla.org/r/57170/#review54566

s/modift/Modify/ in the commit message.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8759108 [details]
Bug 1276830 - part3 : modify tests.

https://reviewboard.mozilla.org/r/57172/#review54568

::: dom/media/test/test_texttrackcue.html
(Diff revision 1)
> -      exceptionHappened = false;
> -      try {
> +      cue.lineAlign = "center";
> +      is(cue.lineAlign, "center", "Cue's line align should be center.");
> -        cue.lineAlign = "left";
> -      } catch(e) {
> -        exceptionHappened = true;
> -        is(e.name, "SyntaxError", "Should have thrown SyntaxError.");

Is it ok to remove this because there's less confusion between types now, and checkEnumValue() takes care of the allowed values?

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8759106 [details]
Bug 1276830 - part1 : introduce LineAlignSetting.

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

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8759107 [details]
Bug 1276830 - part2 : modift LineAlignSetting value in vtt.jsm.

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

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8759108 [details]
Bug 1276830 - part3 : modify tests.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbc23aff362
part1 : introduce LineAlignSetting. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/03958a72e882
part2 : modift LineAlignSetting value in vtt.jsm. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/925a96e53262
part3 : modify tests. r=rillian

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fbc23aff362
https://hg.mozilla.org/mozilla-central/rev/03958a72e882
https://hg.mozilla.org/mozilla-central/rev/925a96e53262