Closed Bug 982183 Opened 6 years ago Closed 6 years ago

[webvtt] Put VTTRegion implementation behind a pref.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- unaffected
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Ralph suggested that the pref be called 'media.vttregion.enabled'.
#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.
Assignee: nobody → rick.eyre
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.
Attachment #8389931 - Flags: review?(giles)
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.
Attachment #8389931 - Flags: review?(giles)
Attachment #8389931 - Flags: review?(bzbarsky)
Attachment #8389931 - Flags: review+
> 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.
Attachment #8389931 - Flags: review?(bzbarsky) → review-
parser/tests/test_extended_attributes.py does check for duplicate attributes, just verifies that the one with a string value wins.
Duplicate extended attributes are OK.  Duplicate [Pref] annotations are not.
(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?
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.
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.
> 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...
(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.
Attachment #8389931 - Attachment is obsolete: true
Attachment #8390760 - Flags: review?(bzbarsky)
Comment on attachment 8390760 [details] [diff] [review]
v2: Put VTTRegion DOM API behind a pref r=rillian,bz.

r=me
Attachment #8390760 - Flags: review?(bzbarsky) → review+
Rebasing.

Carrying forward r=rillian,bz
Attachment #8390760 - Attachment is obsolete: true
I had to update test_interfaces.html with the VTTRegion pref.
Attachment #8391220 - Attachment is obsolete: true
Attachment #8391301 - Flags: review?(bzbarsky)
Comment on attachment 8391301 [details] [diff] [review]
v4: Put VTTRegion DOM API behind a pref r=rillian,bz.

r=me
Attachment #8391301 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/0b59bee9d3d2
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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:
Attachment #8391301 - Flags: approval-mozilla-aurora?
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:
Attachment #8391301 - Flags: approval-mozilla-beta?
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.
Attachment #8391301 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8391301 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.