Bug 101004 - quoteCSSString() always creates a 16 bit string
Summary: quoteCSSString() always creates a 16 bit string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-01 18:27 PDT by Michael Saboff
Modified: 2012-11-06 09:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2012-11-01 18:36 PDT, Michael Saboff
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch with suggested changes and speculative chromium fix (3.80 KB, patch)
2012-11-02 15:23 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-11-01 18:27:31 PDT
quoteCSSString() in CSSParser.cpp should create an 8 bit string for most case.
Comment 1 Michael Saboff 2012-11-01 18:36:31 PDT
Created attachment 171967 [details]
Patch
Comment 2 Darin Adler 2012-11-02 10:56:21 PDT
Comment on attachment 171967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171967&action=review

> Source/WebCore/css/CSSParser.cpp:10640
> +    if (string.length() >= (std::numeric_limits<unsigned>::max() / 3) - 2)

Not new code, but seems to me there are extra parentheses here. Also seems strange to use >= here instead of just >.

> Source/WebCore/css/CSSParser.cpp:10641
> +        return "";

Should probably use emptyString() here since it makes more efficient code.
Comment 3 WebKit Review Bot 2012-11-02 14:36:07 PDT
Comment on attachment 171967 [details]
Patch

Attachment 171967 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14670963

New failing tests:
svg/W3C-SVG-1.1/animate-elem-78-t.svg
svg/W3C-SVG-1.1/animate-elem-52-t.svg
Comment 4 Michael Saboff 2012-11-02 15:23:43 PDT
Created attachment 172157 [details]
Patch with suggested changes and speculative chromium fix
Comment 5 Darin Adler 2012-11-06 09:03:40 PST
Comment on attachment 172157 [details]
Patch with suggested changes and speculative chromium fix

I wonder if it would be better to change this to use StringBuilder instead of StringBuffer at some point.
Comment 6 WebKit Review Bot 2012-11-06 09:44:43 PST
Comment on attachment 172157 [details]
Patch with suggested changes and speculative chromium fix

Clearing flags on attachment: 172157

Committed r133625: <http://trac.webkit.org/changeset/133625>
Comment 7 WebKit Review Bot 2012-11-06 09:44:46 PST
All reviewed patches have been landed.  Closing bug.