178319 – Change String::number to not implicitly be "fixed precision 6 digits"

RESOLVED FIXED Bug 178319

Change String::number to not implicitly be "fixed precision 6 digits"

https://bugs.webkit.org/show_bug.cgi?id=178319

Summary Change String::number to not implicitly be "fixed precision 6 digits"

Darin Adler

Reported 2017-10-14 19:23:23 PDT

Change String::number to use "shortest" instead of "fixed precision 6 digits"

Attachments
Patch (183.19 KB, patch) 2017-10-14 19:23 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (197.03 KB, patch) 2017-10-28 16:24 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (198.11 KB, patch) 2017-10-29 18:21 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (204.02 KB, patch) 2017-10-29 19:37 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (204.04 KB, patch) 2017-10-29 20:25 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (206.89 KB, patch) 2017-10-29 22:51 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (206.89 KB, patch) 2017-10-29 23:00 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.04 MB, application/zip) 2017-10-30 00:08 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.20 MB, application/zip) 2017-10-30 00:15 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.80 MB, application/zip) 2017-10-30 00:26 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1.17 MB, application/zip) 2017-10-30 00:31 PDT, Build Bot	no flags	Details
Patch (222.78 KB, patch) 2017-11-01 22:06 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (9.34 KB, patch) 2019-03-16 14:58 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (10.68 KB, patch) 2019-03-17 17:02 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (22.87 KB, patch) 2019-05-13 05:47 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.48 MB, application/zip) 2019-05-13 07:59 PDT, EWS Watchlist	no flags	Details
Patch (23.99 KB, patch) 2019-05-13 08:05 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (23.59 KB, patch) 2019-05-13 20:04 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (25.15 KB, patch) 2019-05-14 19:12 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Show Obsolete (18) View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2017-10-14 19:23:47 PDT

Comment hidden (obsolete)

Created attachment 323834 [details] Patch

Build Bot

Comment 2 2017-10-14 19:25:09 PDT

Comment hidden (obsolete)

Attachment 323834 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 87 files If any of these errors are false positives, please file a bug against check-webkit-style.

Darin Adler

Comment 3 2017-10-28 16:24:23 PDT

Comment hidden (obsolete)

Created attachment 325269 [details] Patch

Darin Adler

Comment 4 2017-10-29 18:21:39 PDT

Comment hidden (obsolete)

Created attachment 325307 [details] Patch

Darin Adler

Comment 5 2017-10-29 19:37:11 PDT

Comment hidden (obsolete)

Created attachment 325312 [details] Patch

Darin Adler

Comment 6 2017-10-29 20:25:53 PDT

Comment hidden (obsolete)

Created attachment 325316 [details] Patch

Darin Adler

Comment 7 2017-10-29 22:51:02 PDT

Comment hidden (obsolete)

Created attachment 325325 [details] Patch

Darin Adler

Comment 8 2017-10-29 23:00:24 PDT

Comment hidden (obsolete)

Created attachment 325326 [details] Patch

Build Bot

Comment 9 2017-10-30 00:08:29 PDT

Comment hidden (obsolete)

Comment on attachment 325326 [details] Patch Attachment 325326 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5037136 New failing tests: js/kde/Number.html js/dom/number-toprecision.html

Build Bot

Comment 10 2017-10-30 00:08:30 PDT

Comment hidden (obsolete)

Created attachment 325330 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 11 2017-10-30 00:15:32 PDT

Comment hidden (obsolete)

Comment on attachment 325326 [details] Patch Attachment 325326 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5037148 New failing tests: js/kde/Number.html js/dom/number-toprecision.html

Build Bot

Comment 12 2017-10-30 00:15:33 PDT

Comment hidden (obsolete)

Created attachment 325332 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 13 2017-10-30 00:26:02 PDT

Comment hidden (obsolete)

Comment on attachment 325326 [details] Patch Attachment 325326 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5037160 New failing tests: js/kde/Number.html js/dom/number-toprecision.html

Build Bot

Comment 14 2017-10-30 00:26:04 PDT

Comment hidden (obsolete)

Created attachment 325333 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 15 2017-10-30 00:31:35 PDT

Comment hidden (obsolete)

Comment on attachment 325326 [details] Patch Attachment 325326 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5037161 New failing tests: js/kde/Number.html js/dom/number-toprecision.html

Build Bot

Comment 16 2017-10-30 00:31:37 PDT

Comment hidden (obsolete)

Created attachment 325334 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Darin Adler

Comment 17 2017-11-01 22:06:27 PDT

Comment hidden (obsolete)

Created attachment 325679 [details] Patch

Build Bot

Comment 18 2017-11-02 00:11:40 PDT

Comment hidden (obsolete)

Attachment 325679 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 95 files If any of these errors are false positives, please file a bug against check-webkit-style.

Darin Adler

Comment 19 2017-11-02 09:17:30 PDT

Comment hidden (obsolete)

Having trouble figuring out what is causing the JavaScript stress tests to fail. Once I resolve that then I guess this is ready.

Darin Adler

Comment 20 2019-03-10 12:46:54 PDT

Comment hidden (obsolete)

Not going to use the large patch I had here. Instead we are doing this more carefully and gradually and will be able to throw the switch soon.

Darin Adler

Comment 21 2019-03-16 14:58:05 PDT

Comment hidden (obsolete)

Created attachment 364944 [details] Patch

Darin Adler

Comment 22 2019-03-17 17:02:39 PDT

Comment hidden (obsolete)

Created attachment 364987 [details] Patch

Darin Adler

Comment 23 2019-05-13 05:47:35 PDT

Comment hidden (obsolete)

Created attachment 369725 [details] Patch

EWS Watchlist

Comment 24 2019-05-13 07:59:45 PDT

Comment hidden (obsolete)

Comment on attachment 369725 [details] Patch Attachment 369725 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12176478 New failing tests: fast/visual-viewport/zoomed-fixed-scroll-down-then-up.html

EWS Watchlist

Comment 25 2019-05-13 07:59:47 PDT

Comment hidden (obsolete)

Created attachment 369730 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4

Darin Adler

Comment 26 2019-05-13 08:05:18 PDT

Comment hidden (obsolete)

Created attachment 369731 [details] Patch

Darin Adler

Comment 27 2019-05-13 20:04:23 PDT

Comment hidden (obsolete)

Created attachment 369816 [details] Patch

Darin Adler

Comment 28 2019-05-14 19:12:34 PDT

Created attachment 369918 [details] Patch

Sam Weinig

Comment 29 2019-05-17 19:33:21 PDT

Comment on attachment 369918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369918&action=review > Source/WTF/wtf/text/StringBuilder.h:236 > // FIXME: Delete in favor of the name appendShortestFormNumber or just appendNumber. Since appendNumber for these types are now 'delete', this comment should be updated to just call for using appendShortestFormNumber. > Source/WTF/wtf/text/WTFString.h:193 > // FIXME: Delete in favor of the name numberToStringShortest or just number. Since number for these types are now 'delete', this comment should be updated to just call for using numberToStringShortest.

Darin Adler

Comment 30 2019-05-19 10:49:21 PDT

Comment on attachment 369918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369918&action=review >> Source/WTF/wtf/text/StringBuilder.h:236 >> // FIXME: Delete in favor of the name appendShortestFormNumber or just appendNumber. > > Since appendNumber for these types are now 'delete', this comment should be updated to just call for using appendShortestFormNumber. Comment refers to the overload for double. In this patch, only the float overload is deleted. It’s there and deleted to prevent implicit float to double type conversion. So I think I am still happy with the comment as is. >> Source/WTF/wtf/text/WTFString.h:193 >> // FIXME: Delete in favor of the name numberToStringShortest or just number. > > Since number for these types are now 'delete', this comment should be updated to just call for using numberToStringShortest. Same thought as above.

Darin Adler

Comment 31 2019-05-19 10:51:02 PDT

Also, thanks for the review. I feel like I’ve been working on this for so long, but it’s almost done.

WebKit Commit Bot

Comment 32 2019-05-19 11:17:46 PDT

Comment on attachment 369918 [details] Patch Clearing flags on attachment: 369918 Committed r245504: <https://trac.webkit.org/changeset/245504>

WebKit Commit Bot

Comment 33 2019-05-19 11:17:48 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 34 2019-05-19 11:18:21 PDT

<rdar://problem/50929253>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Darin Adler

Reported

2017-10-14 19:23 PDT

Modified

2019-06-02 22:01 PDT History

CC List

6 users Show

URL

Keywords InRadar

Depends on

194994 195142 195262 195533

Blocks

198471

Dependancies

tree graph