Summary: | Small fixes for WebVTTParser | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anna Cavender <annacc> | ||||||
Component: | Media | Assignee: | 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
Anna Cavender
2011-11-01 15:50:12 PDT
Created attachment 113246 [details]
Patch
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. 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. Created attachment 113276 [details]
Patch for landing
Comment on attachment 113276 [details] Patch for landing Clearing flags on attachment: 113276 Committed r99039: <http://trac.webkit.org/changeset/99039> All reviewed patches have been landed. Closing bug. |