Closed Bug 978163 Opened 6 years ago Closed 6 years ago

[webvtt] Update VTTRegion implementation to spec

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 4 obsolete files)

There are a few things that have changed:

- VTTRegion no longer has id or text track properties.
- VTTRegionList has been removed.
- VTTCues now hold a direct reference to a VTTRegion.

The relevant parser changes for vtt.js are ready to land. That should happen before the change to the VTTCue class.
Blocks: webvtt
Blocks: 882664
Boris, I've asked for review from you as I've modified one of the test_interfaces.html files.

As I mentioned in the commit message I haven't removed VTTRegion::Id yet, as I'm unconvinced that we don't need it.
Assignee: nobody → rick.eyre
Status: NEW → ASSIGNED
Attachment #8388553 - Flags: review?(giles)
Attachment #8388553 - Flags: review?(bzbarsky)
Comment on attachment 8388553 [details] [diff] [review]
v1: Remove VTTRegionList and VTTRegion TextTrack extensions r=rillian,bz

r=me on the test_interfaces.html changes.
Attachment #8388553 - Flags: review?(bzbarsky) → review+
Comment on attachment 8388553 [details] [diff] [review]
v1: Remove VTTRegionList and VTTRegion TextTrack extensions r=rillian,bz

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

So this removes VTTRegionList and textTrack.addRegion and .removeRegion, but 'new VTTRegion()' still works? Shouldn't the parser still be hooked up even if there's no way to access the region data? Maybe this whole thing should go behind a pref while the spec is in flux?

Spec bug reference in the commit message, please.
Attachment #8388553 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #3)
 
> So this removes VTTRegionList and textTrack.addRegion and .removeRegion, but
> 'new VTTRegion()' still works? Shouldn't the parser still be hooked up even
> if there's no way to access the region data? Maybe this whole thing should
> go behind a pref while the spec is in flux?

new VTTRegion() still works as VTTCues now have a direct reference to a VTTRegion. So you can instantiate a new region and then start adding it to a set of cues. We still have to add that functionality in. That means changing the relevant bits for vtt.js. I already have a patch waiting to land for that: https://github.com/mozilla/vtt.js/pull/244
I'll add the relevant updates to VTTCue in another patch along with the changes to vtt.js.

I think it may make sense to have the VTTRegion stuff behind a pref. If we do that though I wonder if we should uplift the pref to Beta/Aurora because we've already shipped VTTRegionList and the old VTTRegion there. What do you think Ralph?
(In reply to Ralph Giles (:rillian) from comment #3)

> 'new VTTRegion()' still works? Shouldn't the parser still be hooked up even
> if there's no way to access the region data?

To clarify, the parser is still hooked up, it's just that the parser itself adds the Region to the Cue now, instead of that being taken care of in the callback.
Carrying forward r=bz.

On second thought I have removed VTTRegion::Id. I agree with the reasons for why it changed and it is listed for v1 as well. See the spec bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=24380.
Attachment #8388553 - Attachment is obsolete: true
Attachment #8389236 - Flags: review?(giles)
(In reply to Rick Eyre (:reyre) from comment #6)
> (In reply to Ralph Giles (:rillian) from comment #3)

> To clarify, the parser is still hooked up, it's just that the parser itself
> adds the Region to the Cue now, instead of that being taken care of in the
> callback.

Ah I see. Or it will be when the vtt.js changes land? Will that remove the onregion method entirely?
(In reply to Ralph Giles (:rillian) from comment #8)

> Ah I see. Or it will be when the vtt.js changes land? Will that remove the
> onregion method entirely?

It will be when vtt.js lands. I'm working on that right now. I think keeping onregion in vtt.js is useful for other applications. I'm not sure if we need it anymore for Gecko though, so we could remove that now or sometime in the future.
Comment on attachment 8389236 [details] [diff] [review]
Part 1 v1: Remove VTTRegionList and VTTRegion TextTrack extensions r=rillian,bz

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

Thanks for adding the bug reference.
Attachment #8389236 - Flags: review?(giles) → review+
(In reply to Rick Eyre (:reyre) from comment #5)

> I think it may make sense to have the VTTRegion stuff behind a pref. If we
> do that though I wonder if we should uplift the pref to Beta/Aurora because
> we've already shipped VTTRegionList and the old VTTRegion there. What do you
> think Ralph?

We should definitely port the pref to FF29. I expect it's too late to do it for FF28 since we're in the last week of beta. We could turn off webvtt entirely, but that's it.
(In reply to Ralph Giles (:rillian) from comment #11)

> We should definitely port the pref to FF29. I expect it's too late to do it
> for FF28 since we're in the last week of beta. We could turn off webvtt
> entirely, but that's it.

So this would be a new pref specifically for VTTRegions, correct? I can open a new bug for that.
Yes, a new media.vttregion.enabled is what I was suggesting, just to turn off the region features.
VTTRegion pref filed as bug 982183.
Attachment #8389300 - Flags: review?(giles)
Attachment #8389306 - Flags: review?(giles)
Attachment #8389308 - Flags: review?(giles)
Attachment #8389308 - Attachment is obsolete: true
Attachment #8389308 - Flags: review?(giles)
Comment on attachment 8389300 [details] [diff] [review]
Part 2 v1: VTTCue should have Region property r=rillian

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

LGTM. Boris, do you want to see things like this for DOM review? Spec ref is http://dev.w3.org/html5/webvtt/#vttcue-interface
Attachment #8389300 - Flags: review?(giles)
Attachment #8389300 - Flags: review?(bzbarsky)
Attachment #8389300 - Flags: review+
Attachment #8389304 - Flags: review?(giles) → review+
Comment on attachment 8389306 [details] [diff] [review]
Part 4 v1: Add test for VTTCue::Region r=rillian

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

Thanks for writing a test. I'm fine with this landing as is, but if we're making region support optional you'll have to rewrite this test. Up to you whether you want to worry about that later or split them into separate region.vtt and test_vttregion.html files now.
Attachment #8389306 - Flags: review?(giles) → review+
Comment on attachment 8389300 [details] [diff] [review]
Part 2 v1: VTTCue should have Region property r=rillian

Getting a DOM peer review on changes like this is good, yes.

>+  if (mRegion.get() == aRegion) {

No need for the ".get()" bit there.

r=me with that.
Attachment #8389300 - Flags: review?(bzbarsky) → review+
Thanks for review Boris.

Carrying forward r=rillian,bz
Attachment #8389300 - Attachment is obsolete: true
Just a rebase.

Carrying forward r=rillian.
Attachment #8389306 - Attachment is obsolete: true
(In reply to Rick Eyre (:reyre) from comment #23)

> Just a rebase.

Actually, sorry, I also had to modify test_texttrackregion.html a bit to account for the changes to Regions.

Carrying forward r=rillian.
Try looks green: Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=9483cf9f04e7

The patches are in here although they're not showing up as I did a few tests before this one.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.