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

Description Eric Seidel (no email) 2013-02-12 14:56:36 PST
Use fancy new Vector-based String constructors in the WebVTT parser
Comment 1 Eric Seidel (no email) 2013-02-12 14:58:26 PST
Created attachment 187945 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-12 15:05:29 PST
The build will fail until bug 109617 lands.
Comment 3 WebKit Review Bot 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
Comment 4 Benjamin Poulain 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.
Comment 5 WebKit Review Bot 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
Comment 6 Peter Beverloo (cr-android ews) 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
Comment 7 EFL EWS Bot 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
Comment 8 Eric Seidel (no email) 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.
Comment 9 Eric Seidel (no email) 2013-02-12 17:20:15 PST
Created attachment 187970 [details]
Patch for landing
Comment 10 WebKit Review Bot 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
Comment 11 Build Bot 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
Comment 12 Eric Seidel (no email) 2013-02-13 10:58:15 PST
Created attachment 188121 [details]
Patch for landing
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2013-02-13 12:25:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Eric Seidel (no email) 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.