[webkit-reviews] review granted: [Bug 216016] Align Big5 decoding with spec, Chrome, and Firefox : [Attachment 407633] 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 216016: Align Big5 decoding with spec, Chrome, and Firefox
https://bugs.webkit.org/show_bug.cgi?id=216016

Attachment 407633: Patch

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




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

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

> Source/WebCore/platform/text/EncodingTables.h:32
> +constexpr size_t jis0208Size { 7724 };

This should not be needed. There’s std::begin, std::end, std::size, and
range-based for loops that all work with standard C arrays without requiring a
separate constant.

> Source/WebCore/platform/text/EncodingTables.h:35
> +constexpr size_t big5EncodingMapSize { 14686 };

Ditto.

> Source/WebCore/platform/text/EncodingTables.h:37
> +constexpr size_t big5DecodingExtrasSize { 3904 };

Ditto.

> Source/WebCore/platform/text/TextCodecCJK.cpp:218
> +    static std::array<std::pair<uint16_t, UChar32>, big5DecodingExtrasSize +
big5EncodingMapSize>* table { nullptr };
> +    static std::once_flag flag;
> +    std::call_once(flag, [&] {

call_once should not be required. Should just be able to initialize it like
this:

    static std::array<std::pair<uint16_t, UChar32>, big5DecodingExtrasSize +
big5EncodingMapSize>* table = []{

But also, why is this on the heap?

> Source/WebCore/platform/text/TextCodecCJK.cpp:228
> +	   for (size_t i = 0; i < big5DecodingExtrasSize; i++)
> +	       (*table)[i] = big5DecodingExtras[i];
> +	   for (size_t i = 0; i < big5EncodingMapSize; i++) {
> +	       (*table)[i + big5DecodingExtrasSize].first =
big5EncodingMap[i].second;
> +	       (*table)[i + big5DecodingExtrasSize].second =
big5EncodingMap[i].first;
> +	   }
> +	   std::sort(table->begin(), table->end(), [] (auto& a, auto& b) {
> +	       return a.first < b.first;
> +	   });

Can we do this at compile time instead of runtime?

> Source/WebCore/platform/text/TextCodecCJK.h:39
> +    enum class Encoding : uint8_t {
> +	   EUC_JP,
> +	   Big5
> +    };
> +    
> +    explicit TextCodecCJK(Encoding);

Does this need to be public? Maybe so makeUnique can see it? In concept this is
private.

> Source/WebCore/platform/text/TextCodecCJK.h:40
> +    virtual ~TextCodecCJK() = default;

I suggest just omitting this; compiler will generate exactly this without you
having to write anything.

> Source/WebCore/platform/text/TextCodecCJK.h:56
> +    std::unique_ptr<TextCodec> m_icuCodec;

Wait, what? Is this permanent for EUC-JP or just temporary?