| Summary: | Make Big5 encoder conform to the specification and behavior of Chrome and Firefox | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | annulen, ap, benjamin, cdumez, cmarcelo, darin, ews-watchlist, gyuyoung.kim, mmaxfield, ryuan.choi, sergio, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alex Christensen
2020-08-29 22:47:22 PDT
Created attachment 407562 [details]
Patch
Comment on attachment 407562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407562&action=review > Source/WebCore/platform/text/TextCodecICU.cpp:409 > +static Vector<uint8_t> eucJPEncode(StringView string, Function<void(UChar32, Vector<uint8_t>&)> unencodableHandler) Repeating what I said last time: These functions don’t belong in TextCodecICU. The TextCodec architecture is designed to use separate TextCodec classes for each type of encoding/decoding. This table-based encoding should be a new class. I understand that we require it because of deficiencies in ICU but that does not mean it should be in the ICU source file and class. Please consider making that change. > Source/WebCore/platform/text/TextCodecICU.cpp:416 > + UChar32 c = *iterator; I prefer auto here. > Source/WebCore/platform/text/TextCodecICU.cpp:464 > + UChar32 c = *iterator; auto (In reply to Darin Adler from comment #2) > Comment on attachment 407562 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407562&action=review > > > Source/WebCore/platform/text/TextCodecICU.cpp:409 > > +static Vector<uint8_t> eucJPEncode(StringView string, Function<void(UChar32, Vector<uint8_t>&)> unencodableHandler) > > Repeating what I said last time: These functions don’t belong in > TextCodecICU. The TextCodec architecture is designed to use separate > TextCodec classes for each type of encoding/decoding. This table-based > encoding should be a new class. I understand that we require it because of > deficiencies in ICU but that does not mean it should be in the ICU source > file and class. Ah, I see what you're saying now. I'm going to commit this here, then as my next step I'll implement decoding to make this into its own codec class. |