868509 - [webvtt] Implement VTTCue

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

TextTrackCue was recently split into TextTrackCue and WebVTTCue, to isolate the WebVTT-specific portions. We should update our implementation to reflect the new spec.

Comment 1

[Rick Eyre (:reyre)]

Are we still wanting to do this?

Comment 2

[Ralph Giles (:rillian)]

I'd like to get the old spec working first, and give the new spec time to settle a bit. Soon, but not right now, in other words.

Comment 3

[Hixie (not reading bugmail)]

A stop-gap measure that would make Firefox compatible with code written for the specs without being particularly risky from a code point of view would be just to rename TextTrackCue to VTTCue. That gets us 95% of the way there. Then if there's a compat need, or if we ever add another type of cue, we could actually split it and add TextTrackCue.

(If you do that, I recommend typedefing TextTrackCue to VTTCue so you don't have to change the type where it's used elsewhere where the spec does have them split, like in TextTrackCueList.)

Comment 4

[Rick Eyre (:reyre)]

Attachment: v1: Rename TextTrackCue to VTTCue r=rillian

Implements Hixie's suggestion.

Comment 5

[Rick Eyre (:reyre)]

Please note the patch renames TextTrackCue* files to VTTCue*. Bugzilla doesn't pick up on it though so you'll have to look at the raw diff.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 794074 [details] [diff] [review]
v1: Rename TextTrackCue to VTTCue r=rillian

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

::: content/html/content/src/HTMLTrackElement.h
@@ +151,4 @@
>    // LoadResource().
>    void LoadResource();
>  
> +  friend VTTCue;

not 'friend class' ?

::: content/media/VTTCue.h
@@ +21,4 @@
>  class HTMLTrackElement;
>  class TextTrack;
>  
> +typedef VTTCue TextTrackCue;

I thought Hixie just meant to typedef the webidl. I think if you're renaming the files you might as well change all the types on the C++ slide.

Comment 7

[Rick Eyre (:reyre)]

Attachment: v2: Expose TextTrackCue as VTTCue to content r=rillian

This is a much easier way which bz suggested we do. Internally TextTrackCue stays the same but the content wrapper is now called VTTCue.

Comment 8

[Rick Eyre (:reyre)]

Attachment: v3: v2: Expose TextTrackCue as VTTCue to content r=rillian

Updated test_texttrackcue.html to work with VTTCues now.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 794670 [details] [diff] [review]
v3:  v2: Expose TextTrackCue as VTTCue to content r=rillian

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

I like this better. My intuition is still that we should rename everything, but in the interest of making progress let's land this.

I couldn't 'git am' the patch; it failed to apply in Bindings.conf. Which is weird since 'patch' had no problems.

Can you add a check to test_texttrackcue.html to verify the typedef works? As I understand it we're supporting both 'TextTrackCue' and 'VTTCue' as constructors.

Comment 10

[Rick Eyre (:reyre)]

Thanks for review Ralph.

(In reply to Ralph Giles (:rillian) from comment #9)

> Can you add a check to test_texttrackcue.html to verify the typedef works?
> As I understand it we're supporting both 'TextTrackCue' and 'VTTCue' as
> constructors.

We're only supporting VTTCue as a ctor. So the TextTrackCue will look like VTTCue now. I tested it out on the web console as well. I'll add a test and update.

Comment 11

[Rick Eyre (:reyre)]

Attachment: v4: Expose TextTrackCue as VTTCue to content r=rillian

Thanks for review Ralph.

(In reply to Ralph Giles (:rillian) from comment #9)

> I couldn't 'git am' the patch; it failed to apply in Bindings.conf. Which is
> weird since 'patch' had no problems.

git am -3 seemed to do it.

> Can you add a check to test_texttrackcue.html to verify the typedef works?
> As I understand it we're supporting both 'TextTrackCue' and 'VTTCue' as
> constructors.

Added. Also changed over a few references to TextTrackCue that I missed in other tests.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 795450 [details] [diff] [review]
v4: Expose TextTrackCue as VTTCue to content r=rillian

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

I'm fine with this, but I still don't understand what the typedef in VTTCue.webidl does.

::: content/media/test/test_texttrackcue.html
@@ +40,5 @@
>        todo_is(cueList.length, 4, "Cue list length should be 4.");
>  
> +      // Check that the typedef of TextTrackCue works in Gecko.
> +      is(window.TextTrackCue, undefined, "TextTrackCue should be undefined.");
> +      isnot(window.VTTCue, undefined, "VTTCue should be defined.");

Hmm. I thought the 'typedef VTTCue TextTrackCue' in VTTCue.webidl would make:

  var cue = new TextTrackCue(0, 1, "cue");

work, producing a VTTCue object. Instead I get 'TextTrackCue undefined'.

So the test is correct in verifying the new interface, but either the typedef doesn't do anything, or I misunderstood what it does.

Comment 13

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #12)

> So the test is correct in verifying the new interface, but either the
> typedef doesn't do anything, or I misunderstood what it does.

Originally I made it like this so that we can reference TextTrackCues in the other WebIDLs that use them such as TextTrackCueList. Now that I think about it we might not have to, and probably shouldn't, do that. We should just use VTTCues instead in those WebIDLs and it will work like we want it too. I'll try that and post a patch.

Comment 14

[Rick Eyre (:reyre)]

Attachment: v5: Expose TextTrackCue as VTTCue to content r=rillian

Removed the typedef in the WebIDL file.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 796227 [details] [diff] [review]
v5: Expose TextTrackCue as VTTCue to content r=rillian

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

Ok, thanks. After talking to Hixie and bz it seems I was confused. Hixie's idea was just to implement VTTCue directly without having it inherit from TextTrackCue. 'typedef' in webidl only affects idl; it doesn't create an alias in javascript. So I think removing it entirely is the way to go.

I don't see any reason not to implement TextTrackCue as an abstract type with no constructor, so it appears as VTTCue.__proto__, but that can be a separate bug. Getting all this landed so basic titles display again is more important.

Comment 16

[Rick Eyre (:reyre)]

Okay sounds good Ralph. Thanks for review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4d22ba455407

Bug to implement an abstract TextTrackCue class is filed as bug 909993.

Comment 17

[Rick Eyre (:reyre)]

Try looks green.

Comment 18

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac79da1fc88

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ac79da1fc88