[webkit-reviews] review granted: [Bug 216052] Align ISO-2022-JP and Shift_JIS encodings with Chrome, Firefox, and the specification : [Attachment 407712] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 216052: Align ISO-2022-JP and Shift_JIS encodings with Chrome, Firefox, and
the specification
https://bugs.webkit.org/show_bug.cgi?id=216052

Attachment 407712: Patch

https://bugs.webkit.org/attachment.cgi?id=407712&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 407712
  --> https://bugs.webkit.org/attachment.cgi?id=407712
Patch

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

> Source/WebCore/platform/text/TextCodecCJK.cpp:244
> +    auto characters = string.upconvertedCharacters();
> +    for (WTF::CodePointIterator<UChar> iterator(characters.get(),
characters.get() + string.length()); !iterator.atEnd(); ++iterator) {

I didn’t notice this on previous passes. Why not just do this?

    for (auto codePoint : string.codePoints())

I don’t understand why it is better to use StringView::upconvertedCharacters
and WTF::CodePointIterator<UChar> instead.

> Source/WebCore/platform/text/TextCodecCJK.cpp:262
> +    auto characters = string.upconvertedCharacters();
> +    for (WTF::CodePointIterator<UChar> iterator(characters.get(),
characters.get() + string.length()); !iterator.atEnd(); ++iterator) {

Ditto.

> Source/WebCore/platform/text/TextCodecCJK.h:40
>      enum class Encoding : uint8_t {
>	   EUC_JP,
> +	   ISO2022JP,
> +	   Shift_JIS,
>	   Big5
>      };

I guess you don’t agree, but I find that "all on one line" nicer for simple
enumerations like this one.

> Source/WebCore/platform/text/TextCodecCJK.h:60
> +    enum class ISO2022JPEncoderState : uint8_t {
> +	   ASCII,
> +	   Roman,
> +	   jis0208
> +    };

Ditto.