867823 - [webvtt] implement TextTrack cue add/remove

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Placeholder for XXX commments in bug 833385.

- Handle de-dup in TextTrack::AddCue

- Throw NotFoundError on invalid remove (https://github.com/RickEyre/mozilla-central/commit/0826f21201e08153cad744a6cb54ed6a25303e5c)

- Implement TextTrack::CueChanged event.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Implement TextTrack Add/RemoveCue

Right now I'm comparing cues via reference comparing. So if two cues with the exact same data are added it won't catch it. I don't know if we want it this way or not.

I've left out all the logic in TextTrack::AddCue that deals with the abstraction of TextTrackCues and WebVTTCues as we're currently not supporting that.

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 769039 [details] [diff] [review]
v1: Implement TextTrack Add/RemoveCue

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

Looks ok to me. I don't see that the spec defines how to compare cues, so comparing by reference seems fine for now. I asked for clarification on #whatwg.

You could presumedly compare by value just by implementing operator== on TextTrackCue. I do note the spec specifies replacement rather than skipping the add when addCue() is called with a duplicate cue. That's equivalent when you're comparing by value, but you should probably as a comment explaining it's an optimization in case we change it later and forget.

Comment 3

[Ralph Giles (:rillian)]

What about tests?

Comment 4

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #2)
> Looks ok to me. I don't see that the spec defines how to compare cues, so
> comparing by reference seems fine for now. I asked for clarification on
> #whatwg.

Hixie just responded that we should be comparing by "identity (ref)" which I would think is comparing by reference. So I think we're good for now.

> You could presumedly compare by value just by implementing operator== on
> TextTrackCue. I do note the spec specifies replacement rather than skipping
> the add when addCue() is called with a duplicate cue. That's equivalent when
> you're comparing by value, but you should probably as a comment explaining
> it's an optimization in case we change it later and forget.

 I'll add in a comment about why we don't do it just to make sure, like you suggest.


(In reply to Ralph Giles (:rillian) from comment #3)
> What about tests?

I'll add those in as well.

I'll try to put a patch up for this bug as soon as I can, it might come sometime later next week depending on how the parser writing goes.

Thanks for the review Ralph.

Comment 5

[Rick Eyre (:reyre)]

Attachment: v2: Implement TextTrack Add/RemoveCue

I've re-enabled the text track cue tests that were set as todo. Is this fine for tests Ralph?

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 807783 [details] [diff] [review]
v2: Implement TextTrack Add/RemoveCue

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

Yep, I can't think of anything else to test here. r=me with doc nits.

::: content/media/test/test_texttrackcue.html
@@ -83,4 @@
>        is(cue.endTime, 4, "Cue's end time should be 4.");
>        is(cue.text, "foo", "Cue's text should be foo.");
>  
> -      // Adding the same cue again should not increase the cue count.

Please keep the first line of this comment, or say something similar in the assert.

@@ +98,2 @@
>        } catch (e) {
> +        is(e.name, "NotFoundError", "We should have caught an error.");

"Should have thrown NotFoundError."

Comment 7

[Rick Eyre (:reyre)]

Attachment: v3: Implement TextTrack Add/RemoveCue

Thanks for review Ralph. Carrying forward r=rillian.

Comment 8

[Rick Eyre (:reyre)]

Attachment: v3: Implement TextTrack Add/RemoveCue r=rillian

Updating commit message to contain r=rillian.

Comment 9

[Rick Eyre (:reyre)]

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

Comment 10

[Rick Eyre (:reyre)]

Looks green.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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