Bug 109619 – Use fancy new Vector-based String constructors in the WebVTT parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Use fancy new Vector-based String constructors in the WebVTT parser

[Eric Seidel (no email)]

Created attachment 187945 [details]
Patch

[Eric Seidel (no email)]

The build will fail until bug 109617 lands.

[WebKit Review Bot]

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 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 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 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 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)]

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)]

Created attachment 187970 [details]
Patch for landing

[WebKit Review Bot]

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 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)]

Created attachment 188121 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188121 [details]
Patch for landing

Clearing flags on attachment: 188121

Committed r142772: <http://trac.webkit.org/changeset/142772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.