Bug 200861 – Tidy up checks to see if a character is in the Latin-1 range by using isLatin1 consistently

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Tidy up checks to see if a character is in the Latin-1 range by using isLatin1 consistently

[Darin Adler]

Created attachment 376612 [details]
Patch

[Darin Adler]

Noticed this mix of idioms while working on some tweaks to StringBuilder, and decided it was good to land this separately.

[Ross Kirsling]

I think there are potentially more places that we can hit:

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.h#L244
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.h#L292

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1049
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1162

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/LiteralParser.cpp#L456

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/yarr/YarrPattern.cpp#L1187

https://github.com/WebKit/webkit/blob/master/Source/WebCore/editing/TextIterator.cpp#L2047
https://github.com/WebKit/webkit/blob/master/Source/WebCore/editing/TextIterator.cpp#L2073

[Ross Kirsling]

Also:

https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/parser/HTMLToken.h#L381
https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/parser/HTMLToken.h#L415

https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/HTTPParsers.cpp#L105

https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/ParsedContentType.cpp#L49

https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/text/TextCodecLatin1.cpp#L137

And maybe here, I guess:
https://github.com/WebKit/webkit/blob/master/Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm#L52

(Sorry if there were any other concerns at play here that I overlooked.)

[EWS Watchlist]

Comment on attachment 376612 [details]
Patch

Attachment 376612 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12932817

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases

[Darin Adler]

(In reply to Ross Kirsling from comment #3)
> I think there are potentially more places that we can hit:
> 
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.h#L244
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/LiteralParser.cpp#L456
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/editing/TextIterator.cpp#L2047
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/HTTPParsers.cpp#L105
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/text/TextCodecLatin1.cpp#L137
> https://github.com/WebKit/webkit/blob/master/Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm#L52

These all seem like good ones that I missed and that I should convert.

> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1162

Saw this and wasn’t sure it was actually a Latin-1 test, but I think you convinced me by citing it.

> https://github.com/WebKit/webkit/blob/master/Source/WebCore/editing/TextIterator.cpp#L2073

Borderline case. I suppose the array is called a "Latin-1 table" so we should use isLatin1. But it seems like we should be checking against the size of the array instead of doing a Latin-1 check.

> https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/parser/HTMLToken.h#L381
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/parser/HTMLToken.h#L415

Borderline cases in my mind. Not sure if "all 8-bit data" is exactly the same thing. But maybe I was being too weird about it and sensitive to the terminology.

> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.h#L292
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1049

I am not sure I like using the isLatin1 function on a value that's not a single character, but rather a bunch of characters or'ed together. I intentionally didn’t use it in cases like that, but I could be convinced.

> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/yarr/YarrPattern.cpp#L1187

I intentionally didn't do this one. It should *not* be a Latin-1 check since logging and consoles are usually UTF-8, not Latin-1, and furthermore non-printable characters like U+007F should not be written to the console. I think it should probably be a "printable ASCII" check instead. I think I won't change it to isLatin1 at this time.

> https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/ParsedContentType.cpp#L49

This one is close but doesn't actually have an isLatin1 check in it that I can see. It has a check for the 0x80-0xFF range.

[Darin Adler]

Created attachment 376616 [details]
Patch

[Darin Adler]

Created attachment 376626 [details]
Patch

[Ross Kirsling]

Comment on attachment 376626 [details]
Patch

Your counterpoints all make sense to me. :)

[Darin Adler]

https://trac.webkit.org/changeset/248831/webkit

[Radar WebKit Bug Importer]

<rdar://problem/54443547>