Bug 88583

Summary: Update the TextTrackCue Constructor
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Anna Cavender <annacc>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, annacc, eric.carlson, feature-media-reviews, ojan, silviapf, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85035    
Attachments:
Description Flags
Patch
none
adding check for NaN
none
Patch for landing none

Victor Carbune
Reported 2012-06-07 15:22:38 PDT
The TextTrackCue constructor has been changed to: [Constructor(double startTime, double endTime, DOMString text)] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#texttrackcue
Attachments
Patch (9.94 KB, patch)
2012-08-21 22:44 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
adding check for NaN (12.48 KB, patch)
2012-08-22 11:09 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Patch for landing (12.77 KB, patch)
2012-08-22 12:40 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Anna Cavender
Comment 1 2012-08-21 22:44:09 PDT
Created attachment 159863 [details] Patch
Anna Cavender
Comment 2 2012-08-22 11:09:34 PDT
Created attachment 159967 [details] adding check for NaN
Eric Carlson
Comment 3 2012-08-22 11:35:01 PDT
Comment on attachment 159967 [details] adding check for NaN View in context: https://bugs.webkit.org/attachment.cgi?id=159967&action=review > Source/WebCore/html/track/TextTrackCue.cpp:92 > + : m_id(emptyString()) I don't think we need to distinguishable between a null and an empty string so this should be unnecessary. > Source/WebCore/html/track/TextTrackCue.h:93 > + void parseSettings(const String&); parseSettings was OK as a private method name but not so much as public method name because, for example, we don't necessarily need to parse the settings string immediately (although we do now) . The spec talks about "WebVTT cue settings" so how about "setCueSettings" instead?
Anna Cavender
Comment 4 2012-08-22 12:26:58 PDT
Comment on attachment 159967 [details] adding check for NaN View in context: https://bugs.webkit.org/attachment.cgi?id=159967&action=review Thanks! >> Source/WebCore/html/track/TextTrackCue.cpp:92 >> + : m_id(emptyString()) > > I don't think we need to distinguishable between a null and an empty string so this should be unnecessary. OK. Gone. >> Source/WebCore/html/track/TextTrackCue.h:93 >> + void parseSettings(const String&); > > parseSettings was OK as a private method name but not so much as public method name because, for example, we don't necessarily need to parse the settings string immediately (although we do now) . The spec talks about "WebVTT cue settings" so how about "setCueSettings" instead? Done.
Anna Cavender
Comment 5 2012-08-22 12:40:17 PDT
Created attachment 159993 [details] Patch for landing
WebKit Review Bot
Comment 6 2012-08-22 14:24:07 PDT
Comment on attachment 159993 [details] Patch for landing Clearing flags on attachment: 159993 Committed r126350: <http://trac.webkit.org/changeset/126350>
WebKit Review Bot
Comment 7 2012-08-22 14:24:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.