[webkit-reviews] review denied: [Bug 49581] Cleanup UTF8.cpp : [Attachment 73969] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 49581: Cleanup UTF8.cpp
https://bugs.webkit.org/show_bug.cgi?id=49581

Attachment 73969: Patch
https://bugs.webkit.org/attachment.cgi?id=73969&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73969&action=review

Thanks for making these changes. I think these are an improvement!

> JavaScriptCore/wtf/unicode/UTF8.cpp:290
> +		   *target++ = static_cast<UChar>(character); // normal case

Is this typecast really needed? Why? Doesn’t narrowing happen automatically
without explicit typecasting.

> JavaScriptCore/wtf/unicode/UTF8.cpp:300
> +	       character -= 0x0010000UL;
> +	       *target++ = U16_LEAD(character);
> +	       *target++ = U16_TRAIL(character);

This is wrong. The U16_LEAD macro works on an actual character, not a character
that has had subtraction done. So this change will break things! We need to
make sure we have testing coverage.

> JavaScriptCore/wtf/unicode/UTF8.cpp:307
> +		   *target++ = 0xFFFD;

We should consider moving CharacterNames.h from WebCore into WTF. Then we could
use the name replacementCharacter here instead of the constant 0xFFFD.