[webkit-reviews] review denied: [Bug 118410] Segmentation fault occurred when ICU data library doesn't embed the expected encoding : [Attachment 206390] Add aliases only for known encodings.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Benjamin Dupont
<bdupont at nds.com>'s request for review:
Bug 118410: Segmentation fault occurred when ICU data library doesn't embed the
expected encoding
https://bugs.webkit.org/show_bug.cgi?id=118410

Attachment 206390: Add aliases only for known encodings.
https://bugs.webkit.org/attachment.cgi?id=206390&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206390&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

The patch cannot be landed with this line, a pre-commit hook will block it.

> Source/WebCore/platform/text/TextCodecICU.cpp:-72
> -    registrar("ISO-8859-8-I", "ISO-8859-8-I");

I don't see how moving this line down can work. addToTextEncodingNameMap() gets
an atomic name for encoding name, so if ISO-8859-8-I is registered after
enumerating ICU aliases, it will just map to ISO-8859-8. That is exactly what
we are trying to avoid.

> Source/WebCore/platform/text/TextCodecICU.cpp:69
> +    WTF::HashMap<const char*, char>knownEncodings;

There shouldn't be a WTF prefix when using WTF names. Also, a missing space
before knownEncodings.

Didn't you mean to use HashSet?

> Source/WebCore/platform/text/TextCodecICU.cpp:138
> +    if (knownEncodings.contains("macintosh")) {

This is not the right check. Since registrar uses an atomic name for standard
name, it was fine to call it with any alias as second argument. But this check
will fail.