Bug 195253 - Retire legacy dtoa function and DecimalNumber class
Summary: Retire legacy dtoa function and DecimalNumber class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-02 19:02 PST by Darin Adler
Modified: 2019-03-03 19:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (58.27 KB, patch)
2019-03-02 19:11 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (58.38 KB, patch)
2019-03-02 19:27 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.52 MB, application/zip)
2019-03-02 20:40 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.68 MB, application/zip)
2019-03-02 20:53 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (1.89 MB, application/zip)
2019-03-02 21:23 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.60 MB, application/zip)
2019-03-02 21:40 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews205 for win-future (12.85 MB, application/zip)
2019-03-02 21:49 PST, EWS Watchlist
no flags Details
Patch (90.03 KB, patch)
2019-03-02 23:34 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (90.97 KB, patch)
2019-03-02 23:53 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.49 MB, application/zip)
2019-03-03 02:08 PST, EWS Watchlist
no flags Details
Patch (117.31 KB, patch)
2019-03-03 08:50 PST, Darin Adler
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-03-02 19:02:18 PST
Retire legacy dtoa function and DecimalNumber class
Comment 1 Darin Adler 2019-03-02 19:11:08 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2019-03-02 19:27:00 PST Comment hidden (obsolete)
Comment 3 EWS Watchlist 2019-03-02 20:40:39 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-03-02 20:40:41 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-03-02 20:53:45 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-03-02 20:53:47 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-03-02 21:23:14 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-03-02 21:23:16 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-03-02 21:40:30 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-03-02 21:40:32 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-03-02 21:49:19 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-03-02 21:49:31 PST Comment hidden (obsolete)
Comment 13 Darin Adler 2019-03-02 23:34:39 PST Comment hidden (obsolete)
Comment 14 Darin Adler 2019-03-02 23:53:14 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-03-03 02:08:12 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-03-03 02:08:13 PST Comment hidden (obsolete)
Comment 17 Darin Adler 2019-03-03 08:50:02 PST
Created attachment 363457 [details]
Patch
Comment 18 Darin Adler 2019-03-03 11:45:47 PST
Passes all tests, ready for review and landing. Lets us delete a lot of code!
Comment 19 Daniel Bates 2019-03-03 12:21:55 PST
Comment on attachment 363457 [details]
Patch

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

> Source/WTF/wtf/JSONValues.cpp:677
> +        else
> +            output.appendECMAScriptNumber(m_value.number);

Nice.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1015
> +        return "attr(" + String(m_value.string) + ')';

Ok as-is. Could write using uniform initializer syntax.

> LayoutTests/fast/css/large-value-csstext-expected.txt:4
> -100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000%
> -0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000009%
> +1e+254%
> +9e-249%

What nonsense we were emitting. Have to ask though, are these results compatible with other browsers? Closer to other browsers? Farther from?
Comment 20 Darin Adler 2019-03-03 12:40:55 PST
Comment on attachment 363457 [details]
Patch

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

>> LayoutTests/fast/css/large-value-csstext-expected.txt:4
>> +9e-249%
> 
> What nonsense we were emitting. Have to ask though, are these results compatible with other browsers? Closer to other browsers? Farther from?

Someone could investigate, I prefer not to at this time. This is currently the *only* test affected. Not sure there are *practical* considerations with enormous values. CSS serialization of floating point numbers is likely quite inconsistent already due to things like mixing single and double precision so there is a ton of room for improvement and I don’t think we’re truly ready to standardize these to help boost compatibility.
Comment 21 Darin Adler 2019-03-03 12:41:41 PST
Comment on attachment 363457 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:1015
>> +        return "attr(" + String(m_value.string) + ')';
> 
> Ok as-is. Could write using uniform initializer syntax.

I chose to match the style of the thing down one line from here. Didn’t think too deeply about the best syntax.
Comment 22 Darin Adler 2019-03-03 12:43:06 PST
Committed r242330: <https://trac.webkit.org/changeset/242330>
Comment 23 Radar WebKit Bug Importer 2019-03-03 12:50:11 PST
<rdar://problem/48545602>
Comment 24 Simon Fraser (smfr) 2019-03-03 19:42:36 PST
Comment on attachment 363457 [details]
Patch

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

>>> LayoutTests/fast/css/large-value-csstext-expected.txt:4
>>> +9e-249%
>> 
>> What nonsense we were emitting. Have to ask though, are these results compatible with other browsers? Closer to other browsers? Farther from?
> 
> Someone could investigate, I prefer not to at this time. This is currently the *only* test affected. Not sure there are *practical* considerations with enormous values. CSS serialization of floating point numbers is likely quite inconsistent already due to things like mixing single and double precision so there is a ton of room for improvement and I don’t think we’re truly ready to standardize these to help boost compatibility.

Luckily CSS does allow scientific notion in numeric values now, and we support it (that only changed a couple of years go). I am slightly uneasy with the compatibility risk of this change, although anyone using parseFloat() in JS on computed style should end up with the same result.
Comment 25 Simon Fraser (smfr) 2019-03-03 19:52:18 PST
(In reply to Simon Fraser (smfr) from comment #24)
> Comment on attachment 363457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363457&action=review
> 
> >>> LayoutTests/fast/css/large-value-csstext-expected.txt:4
> >>> +9e-249%
> >> 
> >> What nonsense we were emitting. Have to ask though, are these results compatible with other browsers? Closer to other browsers? Farther from?
> > 
> > Someone could investigate, I prefer not to at this time. This is currently the *only* test affected. Not sure there are *practical* considerations with enormous values. CSS serialization of floating point numbers is likely quite inconsistent already due to things like mixing single and double precision so there is a ton of room for improvement and I don’t think we’re truly ready to standardize these to help boost compatibility.
> 
> Luckily CSS does allow scientific notion in numeric values now, and we
> support it (that only changed a couple of years go). I am slightly uneasy
> with the compatibility risk of this change, although anyone using
> parseFloat() in JS on computed style should end up with the same result.

Chrome uses scientific notation so we're probably OK.