978163 - [webvtt] Update VTTRegion implementation to spec

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

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.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Remove VTTRegionList and VTTRegion TextTrack extensions r=rillian,bz

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.

Comment 2

[Boris Zbarsky [: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.

Comment 3

[Ralph Giles (:rillian)]

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.

Comment 4

[Rick Eyre (:reyre)]

(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

Comment 5

[Rick Eyre (:reyre)]

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?

Comment 6

[Rick Eyre (:reyre)]

(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.

Comment 7

[Rick Eyre (:reyre)]

Attachment: Part 1 v1: Remove VTTRegionList and VTTRegion TextTrack extensions r=rillian,bz

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.

Comment 8

[Ralph Giles (:rillian)]

(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?

Comment 9

[Rick Eyre (:reyre)]

(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 10

[Ralph Giles (:rillian)]

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.

Comment 11

[Ralph Giles (:rillian)]

(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.

Comment 12

[Rick Eyre (:reyre)]

(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.

Comment 13

[Ralph Giles (:rillian)]

Yes, a new media.vttregion.enabled is what I was suggesting, just to turn off the region features.

Comment 14

[Rick Eyre (:reyre)]

VTTRegion pref filed as bug 982183.

Comment 15

[Rick Eyre (:reyre)]

Attachment: Part 2 v1: VTTCue should have Region property r=rillian

Comment 16

[Rick Eyre (:reyre)]

Attachment: Part 3 v1: Vtt.js should add VTTRegions directly to the VTTCue r=rillian

Comment 17

[Rick Eyre (:reyre)]

Attachment: Part 4 v1: Add test for VTTCue::Region r=rillian

Comment 18

[Rick Eyre (:reyre)]

Attachment: Part 4 v1: Add test for VTTCue::Region r=rillian

Comment 19

[Ralph Giles (:rillian)]

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

Comment 20

[Ralph Giles (:rillian)]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: VTTCue should have Region property r=rillian,bz

Thanks for review Boris.

Carrying forward r=rillian,bz

Comment 23

[Rick Eyre (:reyre)]

Attachment: Part 4 v2: Add test for VTTCue::Region r=rillian.

Just a rebase.

Carrying forward r=rillian.

Comment 24

[Rick Eyre (:reyre)]

(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.

Comment 25

[Rick Eyre (:reyre)]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cd5b6b1ec2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3712a12729
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35718f7ef73
https://hg.mozilla.org/integration/mozilla-inbound/rev/93aefdb374e7

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0cd5b6b1ec2
https://hg.mozilla.org/mozilla-central/rev/3f3712a12729
https://hg.mozilla.org/mozilla-central/rev/a35718f7ef73
https://hg.mozilla.org/mozilla-central/rev/93aefdb374e7