Closed Bug 1276129 Opened 4 years ago Closed 4 years ago

[webvtt] Use PositionAlignSetting type for VTTCue's positionAlign

Categories

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

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(4 files)

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

Now we use AlignSetting for the position, and it's wrong.
Blocks: webvtt
No longer depends on: webvtt
Summary: Use PositionAlignSetting type for VTTCue's position → [webvtt] Use PositionAlignSetting type for VTTCue's position
Summary: [webvtt] Use PositionAlignSetting type for VTTCue's position → [webvtt] Use PositionAlignSetting type for VTTCue's positionAlign
Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56532/diff/1-2/
Attachment #8758197 - Flags: review?(giles)
Attachment #8758198 - Flags: review?(giles)
Attachment #8758199 - Flags: review?(giles)
Attachment #8758200 - Flags: review?(giles)
Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56534/diff/1-2/
Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56536/diff/1-2/
Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56538/diff/1-2/
Hi, Ralph,
Could you help me review these patches?
The purpose of this bug is to use PositionAlignSetting for VTTCue's positionAlign, instead of using AlignSetting [1].
Thanks!

[1] https://w3c.github.io/webvtt/#enumdef-positionalignsetting
Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

https://reviewboard.mozilla.org/r/56532/#review53782

Looks good to me. This will need dom peer review as well.
Attachment #8758197 - Flags: review?(giles) → review+
Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Adding smaug for dom peer review.

Note that similar changes are needed to add the LineAlignSetting enum. That's being tracked in bug 1276830.
Attachment #8758197 - Flags: review?(bugs)
Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

https://reviewboard.mozilla.org/r/56534/#review53784

::: dom/media/webvtt/vtt.jsm:787
(Diff revision 2)
>      // position of the cue box. The reference edge will be resolved later when
>      // the box orientation styles are applied.
>      var textPos = 0;
> -    switch (cue.positionAlign) {
> -    case "start":
> +    switch (cue.computedPositionAlign) {
> +      // TODO : modify these fomula to follow the spec.
> +      case "line-left":

Please file a new bug now and refernce the bug number in the comment here to make sure it's easy to follow-up.
Attachment #8758198 - Flags: review?(giles) → review+
Attachment #8758199 - Flags: review?(giles)
Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

https://reviewboard.mozilla.org/r/56536/#review53790

::: dom/media/test/chrome.ini:8
(Diff revision 2)
> +  seek.webm
> +  seek.webm^headers^
> +  vttPositionAlign.vtt
> +
> +[test_webvtt_positionalign.html]
> +tags = webvtt

chrome.ini tests are not compatible with e10s. Please remove the chrome.ini reference and use `SpecialPowers.wrap()` to access chrome-only attributes like `computedPositionAlign` inside a normal mochitest.

See test_texttrack_cue_moz.html for an example.
Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

https://reviewboard.mozilla.org/r/56538/#review53792

Good idea! Make sure to update this after moving the test in Part 3.
Attachment #8758200 - Flags: review?(giles) → review+
Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56532/diff/2-3/
Attachment #8758197 - Flags: review?(bugs)
Attachment #8758199 - Flags: review?(giles)
Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56534/diff/2-3/
Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56536/diff/2-3/
Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56538/diff/2-3/
Attachment #8758197 - Flags: review?(bugs)
Is this a recent spec change? It is after all not backwards compatible change.
Our implementation didn't follow the spec for a long while, so we need to update it.
Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

https://reviewboard.mozilla.org/r/56532/#review53922

ok, this is worrisome change. If anyone is using the attribute, this will break the usage.
And if no one is using the attribute, why to have it at all?
though, I guess the API still rather new, so I guess that could explain if it isn't being used too much.
So fine, but be careful if we see some regressions because of this.
Attachment #8758197 - Flags: review?(bugs) → review+
Attachment #8758199 - Flags: review?(giles) → review+
Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

https://reviewboard.mozilla.org/r/56536/#review54012

Looks good, thanks!
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #22)
> ok, this is worrisome change. If anyone is using the attribute, this will
> break the usage.
> And if no one is using the attribute, why to have it at all?
> though, I guess the API still rather new, so I guess that could explain if
> it isn't being used too much.
> So fine, but be careful if we see some regressions because of this.

Thanks for your suggestion! 

FYI, the Blink [1] and Webkit [2] doesn't support the "positionAlign" yet.
Blink has the implementation, but it's out-of-date and doesn't be enable.
Webkit doesn't have the implementation.

Therefore, I guess we might not have the regression (because no one use now :p) if we're the only one to support this attribute. 

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/track/vtt/VTTCue.idl&q=VTTCue&sq=package:chromium&type=cs&l=30
[2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/track/VTTCue.idl
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cf9f70e5fe
part1 : introduce PositionAlignSetting. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7eb0adafc6
part2 : change PositionAlign value in vtt.jsm. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb00e0208622
part3 : add & modify test. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa464d5d11b
part4 : add webvtt tag r=rillian
You need to log in before you can comment on or make changes to this bug.