Closed Bug 868509 Opened 11 years ago Closed 11 years ago

[webvtt] Implement VTTCue

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: rillian, Assigned: reyre)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

TextTrackCue was recently split into TextTrackCue and WebVTTCue, to isolate the WebVTT-specific portions. We should update our implementation to reflect the new spec.
Blocks: webvtt
Are we still wanting to do this?
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.
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.)
Summary: [webvtt] Implement WebVTTCue → [webvtt] Implement VTTCue
Assignee: nobody → rick.eyre
Implements Hixie's suggestion.
Attachment #794074 - Flags: review?(giles)
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 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.
Attachment #794074 - Flags: review?(giles) → review-
This is a much easier way which bz suggested we do. Internally TextTrackCue stays the same but the content wrapper is now called VTTCue.
Attachment #794074 - Attachment is obsolete: true
Attachment #794258 - Flags: review?(giles)
Updated test_texttrackcue.html to work with VTTCues now.
Attachment #794258 - Attachment is obsolete: true
Attachment #794258 - Flags: review?(giles)
Attachment #794670 - Flags: review?(giles)
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.
Attachment #794670 - Flags: review?(giles) → review+
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.
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.
Attachment #794670 - Attachment is obsolete: true
Attachment #795450 - Flags: review?(giles)
Blocks: 895091
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.
Attachment #795450 - Flags: review?(giles) → review+
(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.
Removed the typedef in the WebIDL file.
Attachment #795450 - Attachment is obsolete: true
Attachment #796227 - Flags: review?(giles)
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.
Attachment #796227 - Flags: review?(giles) → review+
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.
Try looks green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ac79da1fc88
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: