Bug 179460 – Remove TEC decoders that duplicate ICU decoders

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Maciej Stachowiak]

Remove TEC decoders that duplicate ICU decoders. Some of them define additional aliases, but we can just copy those. There's no need to implement encodings twice.

[Myles C. Maxfield]

I would love to do this.

[Maciej Stachowiak]

This is the relevant data for when Mac has duplicate encodings and possible extra names:

EXTRA NAME: macOS WebKit knows extra nonstandard name isoceltic for ISO-8859-14
EXTRA NAME: macOS WebKit knows extra nonstandard name iso8859101992 for ISO-8859-10
EXTRA NAME: macOS WebKit knows extra nonstandard name isoir226 for ISO-8859-16
EXTRA NAME: macOS WebKit knows extra nonstandard name iso8859141998 for ISO-8859-14
EXTRA NAME: macOS WebKit knows extra nonstandard name iso8859162001 for ISO-8859-16
EXTRA NAME: macOS WebKit knows extra nonstandard name l10 for ISO-8859-16
EXTRA NAME: macOS WebKit knows extra nonstandard name isoir157 for ISO-8859-10
EXTRA NAME: macOS WebKit knows extra nonstandard name isoir199 for ISO-8859-14
EXTRA NAME: macOS WebKit knows extra nonstandard name latin10 for ISO-8859-16
EXTRA NAME: macOS WebKit knows extra nonstandard name latin8 for ISO-8859-14
EXTRA NAME: macOS WebKit knows extra nonstandard name l8 for ISO-8859-14
MULTIPLE CODECS: macOS WebKit supports encoding csisolatin6 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding iso-8859-16 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding iso-8859-14 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding iso-8859-11 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding iso-8859-10 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding latin6 through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding koi8-u through multiple implementations ['TEC', 'ICU']
MULTIPLE CODECS: macOS WebKit supports encoding l6 through multiple implementations ['TEC', 'ICU']

[Maciej Stachowiak]

Created attachment 326548 [details]
Patch

[Darin Adler]

Comment on attachment 326548 [details]
Patch

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

Fewer encodings in this category than I expected, but I understand that this is just one step on the path.

> Source/WebCore/platform/text/TextEncodingRegistry.cpp:125
> +    ASSERT_WITH_MESSAGE(!oldAtomicName, "Duplicate text encoding name %s for %s (previously registered as %s)", alias, atomicName, oldAtomicName);

Technically, making this an assertion requires changing the #if above to ASSERT_DISABLED rather than ERROR_DISABLED. Makes me think our Assertions.h macros are a bit too complex.

[Darin Adler]

Comment on attachment 326548 [details]
Patch

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

>> Source/WebCore/platform/text/TextEncodingRegistry.cpp:125
>> +    ASSERT_WITH_MESSAGE(!oldAtomicName, "Duplicate text encoding name %s for %s (previously registered as %s)", alias, atomicName, oldAtomicName);
> 
> Technically, making this an assertion requires changing the #if above to ASSERT_DISABLED rather than ERROR_DISABLED. Makes me think our Assertions.h macros are a bit too complex.

Another way to do it is to get rid of this function and at the single call site put this one long line of code:

    ASSERT_WITH_MESSAGE(!textEncodingNameMap->contains(alias), "Duplicate text encoding name %s for %s (previously registered as %s)", alias, atomicName, textEncodingNameMap->get(alias));

We needed a function before only because the code was not a simple one-liner.

[Maciej Stachowiak]

(In reply to Darin Adler from comment #5)
> 
> Another way to do it is to get rid of this function and at the single call
> site put this one long line of code:
> 
>     ASSERT_WITH_MESSAGE(!textEncodingNameMap->contains(alias), "Duplicate
> text encoding name %s for %s (previously registered as %s)", alias,
> atomicName, textEncodingNameMap->get(alias));
> 
> We needed a function before only because the code was not a simple one-liner.

I like that suggestion, I'll do it that way.

[Maciej Stachowiak]

Created attachment 326605 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326605 [details]
Patch for landing

Clearing flags on attachment: 326605

Committed r224700: <https://trac.webkit.org/changeset/224700>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change broke the debug builds:

/Volumes/Data/slave/highsierra-debug/build/Source/WebCore/platform/text/TextEncodingRegistry.cpp:140:150: error: format specifies type 'char *' but the argument has type 'bool' [-Werror,-Wformat]

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20%28Build%29/builds/1292

[Alex Christensen]

http://trac.webkit.org/r224701

[Maciej Stachowiak]

(In reply to Alex Christensen from comment #11)
> http://trac.webkit.org/r224701

Thanks for fixing it for me. I probably should have let the final path run through EWS.

[Radar WebKit Bug Importer]

<rdar://problem/35562144>