Bug 20674 – Very large and small numbers fail to round-trip through CSS

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

When converting very large or small numbers back to strings, exponential notation is used, so a value like 90010000px is returned from getComputedStyle() as 9.001e+07px.

9.001e+07px does not parse, so such a value cannot be round-tripped.

This is a particular problem with tranformation matrices, which can often contain very large or small numbers.

[Simon Fraser (smfr)]

Created attachment 23202 [details]
Testcase

[Simon Fraser (smfr)]

See thread starting here:
<http://lists.w3.org/Archives/Public/www-style/2008Sep/0049.html>

[Simon Fraser (smfr)]

Parsing of pixel values like "-131.33376515546504px" seems broken too.

[Evan Martin]

In fixing bug 18994 (https://bugs.webkit.org/show_bug.cgi?id=18994) I'm looking at reimplementing the way we generate floats in CSS.  My fix for that bug would fix this as well.

[Evan Martin]

Can you check in your test?  Or would you mind if I checked it in with appropriate baselines?

[Simon Fraser (smfr)]

Please go ahead and check it in. We should also add some tests which read/write style.webkitTransform, since this often contains floating point numbers with lots of digits.

[Evan Martin]

Created attachment 40757 [details]
smfr's layout test with a failing baseline

[Evan Martin]

Created attachment 40758 [details]
smfr's layout test with a failing baseline

[WebKit Commit Bot]

Comment on attachment 40758 [details]
smfr's layout test with a failing baseline

Rejecting patch 40758 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
ayoutTests/fast/css/large-number-round-trip.html
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Evan Martin]

Created attachment 40804 [details]
smfr's layout test with a failing baseline

[Evan Martin]

Comment on attachment 40804 [details]
smfr's layout test with a failing baseline

now without tabs.

FFFFFUUUUUUUUU

[WebKit Commit Bot]

Comment on attachment 40804 [details]
smfr's layout test with a failing baseline

Clearing flags on attachment: 40804

Committed r49254: <http://trac.webkit.org/changeset/49254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Evan Martin]

Patch was only a test; not fixed yet.

[Evan Martin]

fixed in
http://trac.webkit.org/changeset/49585
darin++

[Simon Fraser (smfr)]

Reopening; the change was reverted. See bug 18994.

[Simon Fraser (smfr)]

I plan to fix CSSPrimitiveValue.cssText() via bug 20674.

[Simon Fraser (smfr)]

Created attachment 70962 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 70962 [details]
Patch

This is correct, and I'm tempted to r+. Though I was planning to solve this in a more generic fashion, by moving this code (formatNumber) into WTFString, as default String::number() implementation for double.
We can still move this to String::number() though as a later point, it's good to see less String::format() usage anyway.
If you prefer to land it as is, I'll r+ it later - waiting for your comment first.

[Simon Fraser (smfr)]

I'd like to get this in, and we can do string cleanup later.

[Nikolas Zimmermann]

Comment on attachment 70962 [details]
Patch

Looks great, r=me. I'll take your formatNumber code as starting point for rewriting String::number(double).

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/69928

[Simon Fraser (smfr)]

There's a possible buffer overflow here: bug 47851.