[webvtt] Implement VTTCue

RESOLVED FIXED in mozilla26

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rillian, Assigned: reyre)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla26
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

Updated

5 years ago
Blocks: 629350
(Assignee)

Comment 1

5 years ago
Are we still wanting to do this?
(Reporter)

Comment 2

5 years ago
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)

Updated

5 years ago
Assignee: nobody → rick.eyre
(Assignee)

Comment 4

5 years ago
Created attachment 794074 [details] [diff] [review]
v1: Rename TextTrackCue to VTTCue r=rillian

Implements Hixie's suggestion.
Attachment #794074 - Flags: review?(giles)
(Assignee)

Comment 5

5 years ago
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.
(Reporter)

Comment 6

5 years ago
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-
(Assignee)

Comment 7

5 years ago
Created attachment 794258 [details] [diff] [review]
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.
Attachment #794074 - Attachment is obsolete: true
Attachment #794258 - Flags: review?(giles)
(Assignee)

Comment 8

5 years ago
Created attachment 794670 [details] [diff] [review]
v3:  v2: Expose TextTrackCue as VTTCue to content r=rillian

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)
(Reporter)

Comment 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 795450 [details] [diff] [review]
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.
Attachment #794670 - Attachment is obsolete: true
Attachment #795450 - Flags: review?(giles)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 796227 [details] [diff] [review]
v5: Expose TextTrackCue as VTTCue to content r=rillian

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+
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Try looks green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ac79da1fc88
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.