Bug 72554 - Implement addCue and removeCue in TextTrack
: Implement addCue and removeCue in TextTrack
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Media Elements
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Eric Carlson
: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-11-16 14:56 PST by Anna Cavender
Modified: 2011-11-25 20:34 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (16.40 KB, patch)
2011-11-18 12:18 PST, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-11-16 14:56:52 PST
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.
Comment 1 Radar WebKit Bug Importer 2011-11-17 12:40:16 PST
<rdar://problem/10464485>
Comment 2 Eric Carlson 2011-11-18 12:18:54 PST
Created attachment 115846 [details]
Proposed patch
Comment 3 Darin Adler 2011-11-18 15:12:01 PST
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.
Comment 4 Eric Carlson 2011-11-25 20:33:49 PST
(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.
Comment 5 Eric Carlson 2011-11-25 20:34:19 PST
http://trac.webkit.org/changeset/101185