Implement addCue and removeCue in TextTrack

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

4.8.10.12.5 Text track API

The addCue(cue) method of TextTrack objects, when invoked, must run the following steps:
If the given cue is already associated with a text track other than the method's TextTrack object's text track, then throw an InvalidStateError exception and abort these steps.
Associate cue with the method's TextTrack object's text track, if it is not currently associated with a text track.
If the given cue is already listed in the method's TextTrack object's text track's text track list of cues, then throw an InvalidStateError exception.
Add cue to the method's TextTrack object's text track's text track list of cues.

The removeCue(cue) method of TextTrack objects, when invoked, must run the following steps:
If the given cue is not associated with the method's TextTrack object's text track, then throw an InvalidStateError exception.
If the given cue is not currently listed in the method's TextTrack object's text track's text track list of cues, then throw a NotFoundError exception.
Remove cue from the method's TextTrack object's text track's text track list of cues.

[Radar WebKit Bug Importer]

<rdar://problem/10464485>

[Eric Carlson]

Created attachment 115846 [details]
Proposed patch

[Darin Adler]

Comment on attachment 115846 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115846&action=review

> Source/WebCore/html/HTMLTrackElement.cpp:195
>  bool HTMLTrackElement::canLoadUrl(LoadableTextTrack*, const KURL& url)

The spelling Url here is quite strange.

> Source/WebCore/html/TextTrack.cpp:181
> +    TextTrack* cueTrack = cue.get()->track();

Should not need .get() here. If you do, the reason is probably that you are using PassRefPtr instead of RefPtr for cue.

> Source/WebCore/html/TextTrack.cpp:189
> +    RefPtr<TextTrackCue> newCue = cue;

The normal style is to name the argument prpCue and the local variable just "cue". And do this closer to the start of the function.

> Source/WebCore/html/TextTrack.cpp:194
> +    if (m_cues->contains(newCue.get())) {

Seems inefficient to separately search for the cue before adding it. Instead the add function could also check if the cue is in the array. I’m a little unclear about why it doesn’t.

> Source/WebCore/html/TextTrack.cpp:205
> +void TextTrack::removeCue(PassRefPtr<TextTrackCue> cue, ExceptionCode& ec)

A remove function should take a raw pointer, not a PassRefPtr.

> Source/WebCore/html/TextTrack.cpp:216
> +    if (cue.get()->track() != this) {

Should not need get() here.

> Source/WebCore/html/TextTrack.cpp:223
> +    if (!m_cues->contains(cue.get())) {

It seems inefficient to search for the cue twice, once to check if it’s contained and a second time to actually remove it. It would be better if remove just indicated failure.

> Source/WebCore/html/TextTrack.cpp:233
> +

Extra blank line here.

[Eric Carlson]

(In reply to comment #3)
> (From update of attachment 115846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115846&action=review
> 
> > Source/WebCore/html/HTMLTrackElement.cpp:195
> >  bool HTMLTrackElement::canLoadUrl(LoadableTextTrack*, const KURL& url)
> 
> The spelling Url here is quite strange.
> 
Good point, I will fix this in a follow up patch.

> > Source/WebCore/html/TextTrack.cpp:181
> > +    TextTrack* cueTrack = cue.get()->track();
> 
> Should not need .get() here. If you do, the reason is probably that you are using PassRefPtr instead of RefPtr for cue.
> 
Fixed.

> > Source/WebCore/html/TextTrack.cpp:189
> > +    RefPtr<TextTrackCue> newCue = cue;
> 
> The normal style is to name the argument prpCue and the local variable just "cue". And do this closer to the start of the function.
> 
Fixed.

> > Source/WebCore/html/TextTrack.cpp:194
> > +    if (m_cues->contains(newCue.get())) {
> 
> Seems inefficient to separately search for the cue before adding it. Instead the add function could also check if the cue is in the array. I’m a little unclear about why it doesn’t.
> 
Good point, fixed.

> > Source/WebCore/html/TextTrack.cpp:205
> > +void TextTrack::removeCue(PassRefPtr<TextTrackCue> cue, ExceptionCode& ec)
> 
> A remove function should take a raw pointer, not a PassRefPtr.
> 
Fixed.

> > Source/WebCore/html/TextTrack.cpp:216
> > +    if (cue.get()->track() != this) {
> 
> Should not need get() here.
> 
Fixed.

> > Source/WebCore/html/TextTrack.cpp:223
> > +    if (!m_cues->contains(cue.get())) {
> 
> It seems inefficient to search for the cue twice, once to check if it’s contained and a second time to actually remove it. It would be better if remove just indicated failure.
> 
Another good point, also fixed.

> > Source/WebCore/html/TextTrack.cpp:233
> > +
> 
> Extra blank line here.
Fixed.

[Eric Carlson]

http://trac.webkit.org/changeset/101185