[webkit-reviews] review granted: [Bug 216168] Align EUC-JP, ISO-2022-JP, and Shift_JIS decoding with Chrome, Firefox, and the specification : [Attachment 407944] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 216168: Align EUC-JP, ISO-2022-JP, and Shift_JIS decoding with Chrome,
Firefox, and the specification
https://bugs.webkit.org/show_bug.cgi?id=216168

Attachment 407944: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 407944
  --> https://bugs.webkit.org/attachment.cgi?id=407944
Patch

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

> Source/WebCore/platform/text/TextCodecCJK.cpp:127
> +    return *table;

Why not using NeverDestroyed instead?

> Source/WebCore/platform/text/TextCodecCJK.cpp:130
> +String TextCodecCJK::decodeCommon(const uint8_t* bytes, size_t length, bool
flush, bool stopOnError, bool& sawError, Function<void(uint8_t, StringBuilder&,
bool&)>&& byteParser)

Could be a const Function&.
Also it is not really great to have a bool&.
How about returning a pair or a struct instead?

> Source/WebCore/platform/text/TextCodecCJK.cpp:132
> +    StringBuilder result;

Is there a way to reserveCapacity?  Hopefully error decoding will be rare.

> Source/WebCore/platform/text/TextCodecCJK.cpp:289
> +    auto byteParser = [&] (uint8_t byte, StringBuilder& result, bool&
sawError) {

Would be more natural for byteParser to return a bool instead of changing
sawError.

> Source/WebCore/platform/text/TextCodecCJK.cpp:422
> +    StringBuilder result;

Ditto for preallocating.

> Source/WebCore/platform/text/TextCodecCJK.cpp:467
> +	   }

I guess these two code paths are done for optimisation.
I would use a template with a boolean template parameter to just write it once
and let the compiler optimise this pattern.

> Source/WebCore/platform/text/TextCodecCJK.cpp:516
> +    parseCodePoint = [&] (UChar32 codePoint) {

One liner.

> Source/WebCore/platform/text/TextCodecCJK.h:59
> +    String big5Decode(const uint8_t*, size_t, bool, bool, bool&);

Now I see there is a pattern for bool& sawError.
Still not a fan of it.

>
LayoutTests/imported/w3c/web-platform-tests/encoding/legacy-mb-japanese/iso-202
2-jp/iso2022jp_chars-csiso2022jp.html:35
> +<span data-cp="3B1" data-bytes="1B 24 42 26 41 1B 28 42">$B&A(B</span>

You are modifying files, it is best to land the WPT PR landing this patch.