Bug 215983 - Make Big5 encoder conform to the specification and behavior of Chrome and Firefox
Summary: Make Big5 encoder conform to the specification and behavior of Chrome and Fir...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-29 22:47 PDT by Alex Christensen
Modified: 2020-08-31 14:32 PDT (History)
12 users (show)

See Also:


Attachments
Patch (2.13 MB, patch)
2020-08-29 22:49 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-08-29 22:47:22 PDT
Make Big5 encoder conform to the specification and behavior of Chrome and Firefox
Comment 1 Alex Christensen 2020-08-29 22:49:43 PDT
Created attachment 407562 [details]
Patch
Comment 2 Darin Adler 2020-08-29 23:24:35 PDT
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
Comment 3 Alex Christensen 2020-08-31 08:45:14 PDT
(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.
Comment 4 Alex Christensen 2020-08-31 08:51:32 PDT
http://trac.webkit.org/r266354
Comment 5 Radar WebKit Bug Importer 2020-08-31 08:52:17 PDT
<rdar://problem/68078902>