Closed Bug 1276830 Opened 4 years ago Closed 4 years ago

[webvtt] Use LineAlignSetting for VTTCue's lineAlign

Categories

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

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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

We're using the AlignSetting for lineAlign, but it's wrong.
Attachment #8759106 - Attachment description: MozReview Request: Bug 1276830 - part1 : introduce LineAlignSetting. → Bug 1276830 - part1 : introduce LineAlignSetting.
Attachment #8759107 - Attachment description: MozReview Request: Bug 1276830 - part2 : modift LineAlignSetting value in vtt.jsm. → Bug 1276830 - part2 : modift LineAlignSetting value in vtt.jsm.
Attachment #8759108 - Attachment description: MozReview Request: Bug 1276830 - part3 : modify tests. → Bug 1276830 - part3 : modify tests.
Attachment #8759106 - Flags: review?(giles)
Attachment #8759106 - Flags: review?(bugs)
Attachment #8759107 - Flags: review?(giles)
Attachment #8759108 - Flags: review?(giles)
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.
Attachment #8759106 - Flags: review?(bugs) → review+
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
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.
yeah, I'm not as worried about this change as I'm about bug 1276130.
Comment on attachment 8759106 [details]
Bug 1276830 - part1 : introduce LineAlignSetting.

https://reviewboard.mozilla.org/r/57168/#review54562
Attachment #8759106 - Flags: review?(giles) → review+
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.
Attachment #8759107 - Flags: review?(giles) → review+
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?
Attachment #8759108 - Flags: review?(giles) → review+
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 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 on attachment 8759108 [details]
Bug 1276830 - part3 : modify tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57172/diff/1-2/
Pushed by alwu@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.