Bug 225577 – Remove uses of the String::toInt family of functions from WebCore/html and similar directories

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Remove uses of the String::toInt family of functions from WebCore/html and similar directories

[Darin Adler]

Created attachment 428115 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r277245 (237514@main): <https://commits.webkit.org/237514@main>

[Radar WebKit Bug Importer]

<rdar://problem/77717469>