Bug 178319

Summary:

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

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

WebCore Misc.

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ews-watchlist, rniwa, sam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

194994, 195142, 195262, 195533

Bug Blocks:

198471

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews103 for mac-elcapitan	none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews112 for mac-elcapitan	none
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

Description Darin Adler 2017-10-14 19:23:23 PDT

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

Comment 1 Darin Adler 2017-10-14 19:23:47 PDT Comment hidden (obsolete)

Created attachment 323834 [details]
Patch

Comment 2 Build Bot 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.

Comment 3 Darin Adler 2017-10-28 16:24:23 PDT Comment hidden (obsolete)

Created attachment 325269 [details]
Patch

Comment 4 Darin Adler 2017-10-29 18:21:39 PDT Comment hidden (obsolete)

Created attachment 325307 [details]
Patch

Comment 5 Darin Adler 2017-10-29 19:37:11 PDT Comment hidden (obsolete)

Created attachment 325312 [details]
Patch

Comment 6 Darin Adler 2017-10-29 20:25:53 PDT Comment hidden (obsolete)

Created attachment 325316 [details]
Patch

Comment 7 Darin Adler 2017-10-29 22:51:02 PDT Comment hidden (obsolete)

Created attachment 325325 [details]
Patch

Comment 8 Darin Adler 2017-10-29 23:00:24 PDT Comment hidden (obsolete)

Created attachment 325326 [details]
Patch

Comment 9 Build Bot 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

Comment 10 Build Bot 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

Comment 11 Build Bot 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

Comment 12 Build Bot 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

Comment 13 Build Bot 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

Comment 14 Build Bot 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

Comment 15 Build Bot 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

Comment 16 Build Bot 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

Comment 17 Darin Adler 2017-11-01 22:06:27 PDT Comment hidden (obsolete)

Created attachment 325679 [details]
Patch

Comment 18 Build Bot 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.

Comment 19 Darin Adler 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.

Comment 20 Darin Adler 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.

Comment 21 Darin Adler 2019-03-16 14:58:05 PDT Comment hidden (obsolete)

Created attachment 364944 [details]
Patch

Comment 22 Darin Adler 2019-03-17 17:02:39 PDT Comment hidden (obsolete)

Created attachment 364987 [details]
Patch

Comment 23 Darin Adler 2019-05-13 05:47:35 PDT Comment hidden (obsolete)

Created attachment 369725 [details]
Patch

Comment 24 EWS Watchlist 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

Comment 25 EWS Watchlist 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

Comment 26 Darin Adler 2019-05-13 08:05:18 PDT Comment hidden (obsolete)

Created attachment 369731 [details]
Patch

Comment 27 Darin Adler 2019-05-13 20:04:23 PDT Comment hidden (obsolete)

Created attachment 369816 [details]
Patch

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

Created attachment 369918 [details]
Patch

Comment 29 Sam Weinig 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.

Comment 30 Darin Adler 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.

Comment 31 Darin Adler 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.

Comment 32 WebKit Commit Bot 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>

Comment 33 WebKit Commit Bot 2019-05-19 11:17:48 PDT

All reviewed patches have been landed.  Closing bug.

Comment 34 Radar WebKit Bug Importer 2019-05-19 11:18:21 PDT

<rdar://problem/50929253>