Bug 71334

Summary: Small fixes for WebVTTParser
Product: WebKit Reporter: Anna Cavender <annacc>
Component: MediaAssignee: Anna Cavender <annacc>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric.carlson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Anna Cavender
Reported 2011-11-01 15:50:12 PDT
hasLongWebVTTIdentifier needs to return true even for WebVTT Identifiers that are not long (exactly 6 chars when they match "WEBVTT").
Attachments
Patch (2.44 KB, patch)
2011-11-01 15:57 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.59 KB, patch)
2011-11-01 21:42 PDT, Anna Cavender 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Anna Cavender
Comment 1 2011-11-01 15:57:06 PDT
Created attachment 113246 [details] Patch
Darin Adler
Comment 2 2011-11-01 16:29:39 PDT
Comment on attachment 113246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113246&action=review > Source/WebCore/ChangeLog:8 > + No new tests. This is needed to enable other tests, coming soon. OK. I’m going to hold you to that one! > Source/WebCore/html/track/WebVTTParser.cpp:193 > - if (line[position++] != ' ' && line[position++] != '\t') > + if (line[position] != ' ' && line[position] != '\t') What guarantees we are not at the end of the line here? By the way, I think the ++ was OK, but not two different ++. A local variable would have been another way to do it. > Source/WebCore/html/track/WebVTTParser.cpp:201 > - if (line[position++] != ' ' && line[position++] != '\t') > + if (line[position] != ' ' && line[position] != '\t') Same question/comment.
Anna Cavender
Comment 3 2011-11-01 18:18:54 PDT
Comment on attachment 113246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113246&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. This is needed to enable other tests, coming soon. > > OK. I’m going to hold you to that one! Thanks Darin. >> Source/WebCore/html/track/WebVTTParser.cpp:193 >> + if (line[position] != ' ' && line[position] != '\t') > > What guarantees we are not at the end of the line here? > > By the way, I think the ++ was OK, but not two different ++. A local variable would have been another way to do it. Good point, I'll add a check. I suppose it doesn't matter since the next action is to ignore remaining white space, but it would eliminate one extra check. I'll use a local var like you suggest. >> Source/WebCore/html/track/WebVTTParser.cpp:201 >> + if (line[position] != ' ' && line[position] != '\t') > > Same question/comment. Ditto.
Anna Cavender
Comment 4 2011-11-01 21:42:56 PDT
Created attachment 113276 [details] Patch for landing
WebKit Review Bot
Comment 5 2011-11-01 22:48:29 PDT
Comment on attachment 113276 [details] Patch for landing Clearing flags on attachment: 113276 Committed r99039: <http://trac.webkit.org/changeset/99039>
WebKit Review Bot
Comment 6 2011-11-01 22:48:35 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.