Bug 225577 - Remove uses of the String::toInt family of functions from WebCore/html and similar directories
Summary: Remove uses of the String::toInt family of functions from WebCore/html and si...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-09 00:52 PDT by Darin Adler
Modified: 2021-05-09 11:23 PDT (History)
19 users (show)

See Also:


Attachments
Patch (31.75 KB, patch)
2021-05-09 01:02 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-09 00:52:34 PDT
Remove uses of the String::toInt family of functions from WebCore/html and similar directories
Comment 1 Darin Adler 2021-05-09 01:02:14 PDT
Created attachment 428115 [details]
Patch
Comment 2 Sam Weinig 2021-05-09 09:25:45 PDT
Comment on attachment 428115 [details]
Patch

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

> Source/WebCore/html/FTPDirectoryDocument.cpp:169
> +        return makeString(FormattedNumber::fixedWidth(*bytes / 1000., 2), " KB");

I am not sure if our style guide has anything to say about it, but I am not a fan of the lack of a trailing 0 here. I much prefer "1000.0" to "1000.". Same below.

> Source/WebCore/html/FormController.cpp:69
> -    size_t size = stateVector[index++].toUInt();
> +    auto size = parseIntegerAllowingTrailingJunk<size_t>(stateVector[index++]).valueOr(0);

I realize this is keeping existing behavior, but it seems like returning WTF::nullopt on failure (and perhaps not allowing trailing junk as well) would be better here since I believe this is all state that we are serializing and expect to be consistent (though I will admit to not being completely familiar with this. 

That said, this could (and probably should) be done separately.

> Source/WebCore/html/HTMLFrameSetElement.cpp:135
> -            m_border = value.toInt();
> +            m_border = parseIntegerAllowingTrailingJunk<int>(value).valueOr(0);

Again, not to be changed here, and what this patch is doing that is so great is really showing all the places where we should re-evaluate things, but it seems suspect that setting <frameset border="apple"> behaves potentially differently than <frameset> due to  m_borderSet being set to true.
Comment 3 Darin Adler 2021-05-09 10:39:52 PDT
Comment on attachment 428115 [details]
Patch

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

>> Source/WebCore/html/FTPDirectoryDocument.cpp:169
>> +        return makeString(FormattedNumber::fixedWidth(*bytes / 1000., 2), " KB");
> 
> I am not sure if our style guide has anything to say about it, but I am not a fan of the lack of a trailing 0 here. I much prefer "1000.0" to "1000.". Same below.

Will fix.

>> Source/WebCore/html/FormController.cpp:69
>> +    auto size = parseIntegerAllowingTrailingJunk<size_t>(stateVector[index++]).valueOr(0);
> 
> I realize this is keeping existing behavior, but it seems like returning WTF::nullopt on failure (and perhaps not allowing trailing junk as well) would be better here since I believe this is all state that we are serializing and expect to be consistent (though I will admit to not being completely familiar with this. 
> 
> That said, this could (and probably should) be done separately.

Will hold off as you suggest, but I agree. Deserializing should probably be much stricter. In fact, if we had them, I’d want to use a mode that did not allow spaces or a leading "+".

>> Source/WebCore/html/HTMLFrameSetElement.cpp:135
>> +            m_border = parseIntegerAllowingTrailingJunk<int>(value).valueOr(0);
> 
> Again, not to be changed here, and what this patch is doing that is so great is really showing all the places where we should re-evaluate things, but it seems suspect that setting <frameset border="apple"> behaves potentially differently than <frameset> due to  m_borderSet being set to true.

I believe that may be intentional. There’s also <frameset border>, <frameset border="">, <frameset border="0"> etc. etc.

By the way, checked and the HTML specification *does* call for "allowing trailing junk" behavior on numeric attribute parsing, although that is not the topic at hand here!

Hard to tell what right and wrong is, in the context of legacy almost-deprecated elements. If this was important I’d want it to be covered in WPT; so if fixing this would not already be a WPT progression, I would want to add something there.

I agree that we should not bundle this with the refactoring, but it sure would be nice to have it right.
Comment 4 Darin Adler 2021-05-09 11:22:30 PDT
Committed r277245 (237514@main): <https://commits.webkit.org/237514@main>
Comment 5 Radar WebKit Bug Importer 2021-05-09 11:23:14 PDT
<rdar://problem/77717469>