Bug 109619

Summary: Use fancy new Vector-based String constructors in the WebVTT parser
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, dglazkov, ojan.autocc, peter+ews, philn, rniwa, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109617    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Eric Seidel (no email)
Reported 2013-02-12 14:56:36 PST
Use fancy new Vector-based String constructors in the WebVTT parser
Attachments
Patch (3.68 KB, patch)
2013-02-12 14:58 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch for landing (3.72 KB, patch)
2013-02-12 17:20 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Patch for landing (3.67 KB, patch)
2013-02-13 10:58 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2013-02-12 14:58:26 PST
Created attachment 187945 [details] Patch
Eric Seidel (no email)
Comment 2 2013-02-12 15:05:29 PST
The build will fail until bug 109617 lands.
WebKit Review Bot
Comment 3 2013-02-12 15:07:36 PST
Comment on attachment 187945 [details] Patch Attachment 187945 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16440459
Benjamin Poulain
Comment 4 2013-02-12 15:11:12 PST
Comment on attachment 187945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187945&action=review Looks great. > Source/WebCore/html/track/WebVTTParser.cpp:382 > + String content(m_token.characters()); // FIXME: Can this be 8bit? I would not put this Fixme. Without context of your current work, it will be impossible to know what you meant. > Source/WebCore/html/track/WebVTTParser.cpp:422 > + // FIXME: m_token.characters should normally be 8-bit! Maybe? // FIXME: m_token.characters should normally be 8-bit! Should we convert the input string to 8bits if possible.
WebKit Review Bot
Comment 5 2013-02-12 15:18:18 PST
Comment on attachment 187945 [details] Patch Attachment 187945 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16517407
Peter Beverloo (cr-android ews)
Comment 6 2013-02-12 15:33:06 PST
Comment on attachment 187945 [details] Patch Attachment 187945 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16513440
EFL EWS Bot
Comment 7 2013-02-12 15:37:29 PST
Comment on attachment 187945 [details] Patch Attachment 187945 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16532108
Eric Seidel (no email)
Comment 8 2013-02-12 17:19:57 PST
I'm certain that both of those cases should use 8bit strings, I just wanted to separate the behavior changes for later. I've made both FIXMEs stronger, will post.
Eric Seidel (no email)
Comment 9 2013-02-12 17:20:15 PST
Created attachment 187970 [details] Patch for landing
WebKit Review Bot
Comment 10 2013-02-12 18:01:48 PST
Comment on attachment 187970 [details] Patch for landing Rejecting attachment 187970 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: switch ../../Source/WebCore/html/track/WebVTTParser.cpp:380: error: enumeration value 'EndOfFile' not handled in switch ninja: build stopped: subcommand failed. Failed to run "['Tools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 1 ser.cpp:380: error: enumeration value 'Uninitialized' not handled in switch ../../Source/WebCore/html/track/WebVTTParser.cpp:380: error: enumeration value 'EndOfFile' not handled in switch ninja: build stopped: subcommand failed. Full output: http://queues.webkit.org/results/16483476
Build Bot
Comment 11 2013-02-13 10:49:07 PST
Comment on attachment 187970 [details] Patch for landing Attachment 187970 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16542193
Eric Seidel (no email)
Comment 12 2013-02-13 10:58:15 PST
Created attachment 188121 [details] Patch for landing
WebKit Review Bot
Comment 13 2013-02-13 12:25:28 PST
Comment on attachment 188121 [details] Patch for landing Clearing flags on attachment: 188121 Committed r142772: <http://trac.webkit.org/changeset/142772>
WebKit Review Bot
Comment 14 2013-02-13 12:25:33 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 15 2013-02-13 21:59:25 PST
I've since learned/re-learned that this is a change in behavior. Previously String(UChar*, length) will treat length=0 as the empty string. String(Vector) treats vector.size() == 0 as the null string. This patch may need to be amended.
Note You need to log in before you can comment on or make changes to this bug.