Bug 205671

Summary:

Simplify StringBuilder API/align with makeString by removing appendFixed* functions and using FormatNumber struct instead

Product:

WebKit

Reporter:

Sam Weinig <sam>

Component:

New Bugs

Assignee:

Sam Weinig <sam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, alecflett, beidson, benjamin, berto, calvaris, cdumez, cgarcia, cmarcelo, commit-queue, darin, dbates, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gustavo, gyuyoung.kim, jamesr, japhet, jer.noble, jsbell, kangil.han, keith_miller, kondapallykalyan, luiz, macpherson, mark.lam, menard, mifenton, mmaxfield, msaboff, pdr, philipj, saam, sabouhallawa, schenney, sergio, simon.fraser, tonikitoo, toyoshim, tzagallo, webkit-bug-importer, yutak

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Sam Weinig

Reported 2019-12-31 20:01:01 PST

Remove numeric append functions from StringBuilder now that append can is capable of handling them

Attachments
Patch (145.83 KB, patch) 2019-12-31 20:18 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (156.58 KB, patch) 2020-01-01 09:01 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (158.14 KB, patch) 2020-01-01 11:25 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (157.40 KB, patch) 2020-01-01 11:43 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (156.65 KB, patch) 2020-01-01 11:44 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (40.06 KB, patch) 2020-01-01 16:52 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Sam Weinig

Comment 1 2019-12-31 20:06:13 PST

The proposed changes are: - StringBuilder::appendNumber(...) -> replaced with StringBuilder::append(...) - StringBuilder::appendFixedPrecisionNumber(...) -> replaced with StringBuilder::append(FormattedNumber::fixedPrecision(...) - StringBuilder::appendFixedWidthNumber(...) -> replaced with StringBuilder::append(FormattedNumber::fixedWidth(...) with the goal of simplifying StringBuilder's API and unifying it more with how makeString(...) works.

Sam Weinig

Comment 2 2019-12-31 20:18:41 PST Comment hidden (obsolete)

Created attachment 386573 [details] Patch

Sam Weinig

Comment 3 2020-01-01 09:01:41 PST Comment hidden (obsolete)

Created attachment 386579 [details] Patch

EWS Watchlist

Comment 4 2020-01-01 09:02:25 PST Comment hidden (obsolete)

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Sam Weinig

Comment 5 2020-01-01 11:25:46 PST Comment hidden (obsolete)

Created attachment 386582 [details] Patch

Sam Weinig

Comment 6 2020-01-01 11:41:53 PST

I think I might scale this back to just the fixed width/precision appends, since I forgot about the UChar vs. uint16_t ambiguity issue, that I am still not sure how best to deal with. In the latest patch, I casted the one uint16_t I knew about, SecurityOriginData's port, to uint32_t, but that doesn't feel like a good solution. Perhaps something like a ForceFormatAsNumber(...) wrapper? I wonder if we could re-define UChar in WebCore to be a "strong typedef", (e.g. struct UChar { explicit UChar(); ... private: uint16_t value; }). Would require a lot of changes when dealing with non-WebCore uses... Not sure it would be worth it.

Sam Weinig

Comment 7 2020-01-01 11:43:36 PST Comment hidden (obsolete)

Created attachment 386583 [details] Patch

Sam Weinig

Comment 8 2020-01-01 11:44:50 PST Comment hidden (obsolete)

Created attachment 386584 [details] Patch

Sam Weinig

Comment 9 2020-01-01 16:52:34 PST

Created attachment 386591 [details] Patch

WebKit Commit Bot

Comment 10 2020-01-02 11:32:05 PST

Comment on attachment 386591 [details] Patch Clearing flags on attachment: 386591 Committed r253975: <https://trac.webkit.org/changeset/253975>