Bug 72554

Summary: Implement addCue and removeCue in TextTrack
Product: WebKit Reporter: Anna Cavender <annacc>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed patch darin: review+

Anna Cavender
Reported 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.
Attachments
Proposed patch (16.40 KB, patch)
2011-11-18 12:18 PST, Eric Carlson 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2011-11-17 12:40:16 PST
<rdar://problem/10464485>
Eric Carlson
Comment 2 2011-11-18 12:18:54 PST
Created attachment 115846 [details] Proposed patch
Darin Adler
Comment 3 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.
Eric Carlson
Comment 4 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.
Eric Carlson
Comment 5 2011-11-25 20:34:19 PST
http://trac.webkit.org/changeset/101185
Note You need to log in before you can comment on or make changes to this bug.