Summary: | Remove TEC decoders that duplicate ICU decoders | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, ap, commit-queue, darin, mmaxfield, ryanhaddad, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Safari Technology Preview | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 179303 | ||||||||
Attachments: |
|
Description
Maciej Stachowiak
2017-11-08 17:17:14 PST
I would love to do this. 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'] Created attachment 326548 [details]
Patch
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. 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. (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. Created attachment 326605 [details]
Patch for landing
Comment on attachment 326605 [details] Patch for landing Clearing flags on attachment: 326605 Committed r224700: <https://trac.webkit.org/changeset/224700> All reviewed patches have been landed. Closing bug. 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 (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. |