Bug 72554 - Implement addCue and removeCue in TextTrack
: Implement addCue and removeCue in TextTrack
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
: 43668
  Show dependency treegraph
 
Reported: 2011-11-16 14:56 PST by
Modified: 2011-11-25 20:34 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-17 12:40:16 PST -------
<rdar://problem/10464485>
------- Comment #2 From 2011-11-18 12:18:54 PST -------
Created an attachment (id=115846) [details]
Proposed patch
------- Comment #3 From 2011-11-18 15:12:01 PST -------
(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.

> 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 From 2011-11-25 20:33:49 PST -------
(In reply to comment #3)
> (From update of attachment 115846 [details] [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 From 2011-11-25 20:34:19 PST -------
http://trac.webkit.org/changeset/101185