Bug 20674

Summary: Very large and small numbers fail to round-trip through CSS
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Severity: Normal CC: barraclough, cmarrin, commit-queue, darin, dino, evan, hyatt, krit, mitz, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Description Flags
smfr's layout test with a failing baseline
smfr's layout test with a failing baseline
smfr's layout test with a failing baseline
Patch zimmermann: review+

Description Simon Fraser (smfr) 2008-09-05 14:54:38 PDT
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.
Comment 1 Simon Fraser (smfr) 2008-09-05 14:55:15 PDT
Created attachment 23202 [details]
Comment 2 Simon Fraser (smfr) 2008-09-08 14:21:26 PDT
See thread starting here:
Comment 3 Simon Fraser (smfr) 2009-01-16 10:50:36 PST
Parsing of pixel values like "-131.33376515546504px" seems broken too.
Comment 4 Evan Martin 2009-10-06 17:51:31 PDT
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.
Comment 5 Evan Martin 2009-10-06 17:52:20 PDT
Can you check in your test?  Or would you mind if I checked it in with appropriate baselines?
Comment 6 Simon Fraser (smfr) 2009-10-06 19:23:26 PDT
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.
Comment 7 Evan Martin 2009-10-06 19:33:10 PDT
Created attachment 40757 [details]
smfr's layout test with a failing baseline
Comment 8 Evan Martin 2009-10-06 19:34:02 PDT
Created attachment 40758 [details]
smfr's layout test with a failing baseline
Comment 9 WebKit Commit Bot 2009-10-06 20:47:16 PDT
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:
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:


    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
Comment 10 Evan Martin 2009-10-07 10:49:04 PDT
Created attachment 40804 [details]
smfr's layout test with a failing baseline
Comment 11 Evan Martin 2009-10-07 10:49:26 PDT
Comment on attachment 40804 [details]
smfr's layout test with a failing baseline

now without tabs.

Comment 12 WebKit Commit Bot 2009-10-07 11:10:33 PDT
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>
Comment 13 WebKit Commit Bot 2009-10-07 11:10:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Evan Martin 2009-10-07 11:14:02 PDT
Patch was only a test; not fixed yet.
Comment 15 Evan Martin 2009-10-14 14:55:16 PDT
fixed in
Comment 16 Simon Fraser (smfr) 2009-12-13 16:10:50 PST
Reopening; the change was reverted. See bug 18994.
Comment 17 Simon Fraser (smfr) 2010-10-16 13:41:01 PDT
I plan to fix CSSPrimitiveValue.cssText() via bug 20674.
Comment 18 Simon Fraser (smfr) 2010-10-16 13:44:14 PDT
Created attachment 70962 [details]
Comment 19 Nikolas Zimmermann 2010-10-17 02:21:58 PDT
Comment on attachment 70962 [details]

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.
Comment 20 Simon Fraser (smfr) 2010-10-17 09:42:15 PDT
I'd like to get this in, and we can do string cleanup later.
Comment 21 Nikolas Zimmermann 2010-10-17 12:39:37 PDT
Comment on attachment 70962 [details]

Looks great, r=me. I'll take your formatNumber code as starting point for rewriting String::number(double).
Comment 22 Simon Fraser (smfr) 2010-10-17 13:06:48 PDT
Comment 23 Simon Fraser (smfr) 2010-10-18 14:48:38 PDT
There's a possible buffer overflow here: bug 47851.