982183 - [webvtt] Put VTTRegion implementation behind a pref.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

The VTTRegion portion of the spec is in a lot of flux so it would be good to have this behind a pref for now. See bug 978163 for more information.

Comment 1

[Rick Eyre (:reyre)]

Ralph suggested that the pref be called 'media.vttregion.enabled'.

Comment 2

[Ralph Giles (:rillian)]

#content is in favour:

11:45 < reyre> Ms2ger: captions display, but none of the special layout 
               positioning for cues with regions is implemented
11:45 < Ms2ger> Oh
11:46 < Ms2ger> And we still implement a DOM API to change them, but that 
                doesn't do anything?
11:46 < reyre> yes
11:46 < Ms2ger> Disable the API :)
11:49 < sicking> rillian: yes, if the DOM API is a bunch of no-ops, then 
                 definitely disable it.

Given the timing, I'd develop a patch against the aurora branch first and push it to try, then work on the nightly version.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v1: Put VTTRegion DOM API behind a pref r=rillian

This puts the DOM API behind a pref. The other part of this is having vtt.js only do layout for Cues, disregarding their Regions. Right now we don't have to do anything for this since vtt.js just does layout for Cues. However, if this pref is still around when we enable Region layout then we need to do something about that.

I've added a note on the relevant vtt.js issue for this and I think we should also keep this bug open to track it in Gecko. See https://github.com/mozilla/vtt.js/issues/267 for details.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 8389931 [details] [diff] [review]
v1: Put VTTRegion DOM API behind a pref r=rillian

Review of attachment 8389931 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me modulo the multiple prefs. Passing to bz for dom peer review.

::: dom/webidl/VTTRegion.webidl
@@ +6,5 @@
>   * The origin of this IDL file is
>   *  http://dev.w3.org/html5/webvtt/#extension-of-the-texttrack-interface-for-region-support
>   */
>  
> +[Constructor, Pref="media.webvtt.enabled", Pref="media.webvtt.regions.enabled"]

Does this work? It appears to only generate code to check the second pref.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Does this work? It appears to only generate code to check the second pref.

Yikes.  We should make that fail codegen.  That does not work, indeed.

Comment 6

[Ralph Giles (:rillian)]

parser/tests/test_extended_attributes.py does check for duplicate attributes, just verifies that the one with a string value wins.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Duplicate extended attributes are OK.  Duplicate [Pref] annotations are not.

Comment 8

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) on vacation until March 31 from comment #4)

> Does this work? It appears to only generate code to check the second pref.

That's strange because I tested it manually as well and turning them both on and off together/separately  had the expected behaviour.

(In reply to Boris Zbarsky [:bz] from comment #7)
> Duplicate extended attributes are OK.  Duplicate [Pref] annotations are not.

So is this a problem with codegen or with the way I've structured this? What's the best way to fix this?

Comment 9

[Ralph Giles (:rillian)]

To make it do what you want you need to fix the codegen to union the prefs. Sounds like bz maybe doesn't want that. I suppose you could just use webvtt.region.enabled pref only; the question is what breaks if you enable that but not webvtt.

Comment 10

[Ralph Giles (:rillian)]

I can certainly do 'var r = new VTTRegion;' and get a TextTrack object even with media.webvtt.enabled set to false. Not ideal, but I wasn't able to create any damage with it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> So is this a problem with codegen or with the way I've structured this? 

The codegen problem is that it allows you to write IDL that then doesn't do what you'd think it does in this case.

If you want to check two separate prefs, use Func and check the prefs directly in the function.  For one thing, the codegen has no way to tell whether you wanted to enable if both are true or if either one is true...

Comment 12

[Rick Eyre (:reyre)]

Attachment: v2: Put VTTRegion DOM API behind a pref r=rillian,bz.

(In reply to Boris Zbarsky [:bz] from comment #11)
> If you want to check two separate prefs, use Func and check the prefs
> directly in the function.  For one thing, the codegen has no way to tell
> whether you wanted to enable if both are true or if either one is true...
Thanks for the clarification Boris.

I've updated the VTTRegion.webidl so that it uses Func and checks the prefs directly.

Carrying forward r=rillian.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8390760 [details] [diff] [review]
v2: Put VTTRegion DOM API behind a pref r=rillian,bz.

r=me

Comment 14

[Rick Eyre (:reyre)]

Attachment: v3: Put VTTRegion DOM API behind a pref r=rillian,bz.

Rebasing.

Carrying forward r=rillian,bz

Comment 15

[Rick Eyre (:reyre)]

Attachment: v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

I had to update test_interfaces.html with the VTTRegion pref.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8391301 [details] [diff] [review]
v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

r=me

Comment 17

[Rick Eyre (:reyre)]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=19c7c6bd14ce

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b59bee9d3d2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b59bee9d3d2

Comment 20

[Rick Eyre (:reyre)]

Comment on attachment 8391301 [details] [diff] [review]
v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): VTTRegions
User impact if declined: VTTRegion DOM API is a no-op because they don't currently effect subtitle layout. It's also confusing to web devs to have the DOM API exist, but for it to do nothing.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch:

Comment 21

[Rick Eyre (:reyre)]

Comment on attachment 8391301 [details] [diff] [review]
v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): VTTRegions
User impact if declined: VTTRegion DOM API is a no-op because they don't currently effect subtitle layout. It's also confusing to web devs to have the DOM API exist, but for it to do nothing.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch:

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8391301 [details] [diff] [review]
v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

We'll approve this for 29 Beta once the merge is complete.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5e919d74591