Bug 225575 - Remove uses of the String::toInt family of functions from the WebCore/platform directory
Summary: Remove uses of the String::toInt family of functions from the WebCore/platfor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-08 22:09 PDT by Darin Adler
Modified: 2021-05-09 13:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (33.78 KB, patch)
2021-05-08 22:24 PDT, Darin Adler
sam: 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 2021-05-08 22:09:01 PDT
Remove uses of the String::toInt family of functions from the WebCore/platform directory
Comment 1 Darin Adler 2021-05-08 22:24:59 PDT
Created attachment 428112 [details]
Patch
Comment 2 Sam Weinig 2021-05-09 10:34:26 PDT
Comment on attachment 428112 [details]
Patch

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

> Source/WebCore/platform/DateComponents.cpp:108
> +// Does not allow leading or trailing whitespace unlike parseInteger<int>.

This, and just thinking about this more, makes me wonder if the new `parseInteger<int>` really should be called `parseIntegerAllowingLeadingWhitespace<int>` to make it clear, and to have another one that doesn't allow leading whitespace be `parseInteger<int>`. I would more than a few callers don't actually need that behavior.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1264
> +    return parseIntegerAllowingTrailingJunk<uint64_t>(trackIDString) == m_enabledVideoTrackID

Is this correct without the .valueOr(0)?
Comment 3 Darin Adler 2021-05-09 11:05:28 PDT
Comment on attachment 428112 [details]
Patch

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

>> Source/WebCore/platform/DateComponents.cpp:108
>> +// Does not allow leading or trailing whitespace unlike parseInteger<int>.
> 
> This, and just thinking about this more, makes me wonder if the new `parseInteger<int>` really should be called `parseIntegerAllowingLeadingWhitespace<int>` to make it clear, and to have another one that doesn't allow leading whitespace be `parseInteger<int>`. I would more than a few callers don't actually need that behavior.

Absolutely worth considering. As I mention in the StringToIntegerConversion.h header with FIXMEs there are these variations:

1) Trailing junk or spaces.
2) Leading spaces.
3) Leading "+".

And which definition of spaces? Confusingly, there are three relevant ASCII space definitions:

- The HTTP version of space is only U+0009 CHARACTER TABULATION, U+000A LINE FEEED, U+000D CARRIAGE RETURN, and U+0020 SPACE.
- The HTML specification ASCII space adds U+000C FORM FEED.
- The "C" locale version of <ctype.h> isspace adds U+000B LINE TABULATION.
- There are various more-extensive Unicode space and space-like definitions that go beyond ASCII.

Long ago I chose to make our ASCIICType.h isASCIISpace function match <ctype.h> isspace, but I am not sure it’s ever needed. Might be nice to merge isASCIISpace and the isHTMLSpace function. Do we need to use a template or have many multiple versions of "allow leading spaces"?

Main question is which parseInteger variation deserves the "short normal" function name, and which get the "longer more explicit" names. Would like to do the same for parseFloat and parseDouble, by the way. Or many rename to parseNumber and overload instead of having integer and floating point separate?

- HTML’s rules for parsing integers are "allows trailing junk, leading HTML spaces, +", so just like the current parseIntegerAllowingTrailingJunk, but with isHTMLSpace, not isASCIISpace.
- For deserializing and places like this DateComponents function, we’d want the strictest: "no leading or trailing spaces or junk, no leading '+'", and that does seem like the purest version, but not sure how common it would be.
- Unclear to me how many other call would need some variation besides those two, so maybe those two should be given compelling short-ish nicknames.
- Note that neither of those two *exactly* match the two we currently have in StringToIntegerConversion.h!

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1264
>> +    return parseIntegerAllowingTrailingJunk<uint64_t>(trackIDString) == m_enabledVideoTrackID
> 
> Is this correct without the .valueOr(0)?

I think it is, but I could be wrong.

Old code would treat a parse failure as track ID 0. The new code treats a parse failure as "does not match any track ID". given how Optional == is defined. I think it’s highly unlikely that "create parse failure as track ID 0" was ever wanted. But if we wanted it we *could* add .valueOr(0). I believe that in practice, we never have parse failures here. We could assert that by writing .value().

I guess I will write .valueOr(0) to stay out of the "possible progressions in the refactoring that could be regressions" business.
Comment 4 Darin Adler 2021-05-09 11:15:48 PDT
At least one thing I am pretty sure of that makes this less than infinitely complicated:

Any caller that wants to tolerate leading spaces also wants to tolerate trailing spaces of the same type (although if it wants to tolerate trailing junk, that doesn't make a material difference).

We can think of "not tolerating leading spaces" on the same spectrum as "type of tolerable space"; it’s sort of "no such thing as a tolerable space".
Comment 5 Darin Adler 2021-05-09 11:17:05 PDT
Also seems likely that any caller that wants to tolerate trailing junk also wants to tolerate leading spaces and leading "+".

For some contexts might want a version that consumes what it parses and indicates how many characters are consumed.
Comment 6 Darin Adler 2021-05-09 11:31:45 PDT
Committed r277246 (237515@main): <https://commits.webkit.org/237515@main>
Comment 7 Radar WebKit Bug Importer 2021-05-09 11:32:13 PDT
<rdar://problem/77717553>
Comment 8 Darin Adler 2021-05-09 11:45:57 PDT
Patch was not landed correctly.
Comment 9 Darin Adler 2021-05-09 11:54:51 PDT
Committed r277249 with the second half.
Comment 10 Darin Adler 2021-05-09 13:20:17 PDT
And ... now I rediscovered parseHTMLInteger.