Bug 109619 - Use fancy new Vector-based String constructors in the WebVTT parser
Summary: Use fancy new Vector-based String constructors in the WebVTT parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 109617
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-12 14:56 PST by Eric Seidel (no email)
Modified: 2013-02-13 21:59 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2013-02-12 14:58 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (3.72 KB, patch)
2013-02-12 17:20 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (3.67 KB, patch)
2013-02-13 10:58 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.