[webkit-reviews] review denied: [Bug 45594] Add AtomicString::fromUTF8 : [Attachment 70357] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 45594: Add AtomicString::fromUTF8
https://bugs.webkit.org/show_bug.cgi?id=45594

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70357&action=review

We'll need some unicode experts to look at this patch.

> JavaScriptCore/wtf/text/AtomicString.cpp:232
> +static inline bool equal(StringImpl* string, const char* characters,
unsigned length)
> +{
> +    if (string->length() != length)
> +	   return false;
> +
> +    const UChar* stringCharacters = string->characters();
> +    for (unsigned i = 0; i != length; ++i) {
> +	   if (*stringCharacters++ != *characters++)
> +	       return false;
> +    }
> +
> +    return true;
> +}

I don't understand how you're comparing UChars with chars.  They're in
different encodings...

> JavaScriptCore/wtf/text/AtomicString.cpp:255
> +	   if (buffer.rawLength == buffer.utf16Length) // buffer contains only
ASCII characters.
> +	       return WTF::equal(string, buffer.characters, buffer.rawLength);

I see what you're trying to do, but WTF::equal has way too general of a name. 
Also, this check isn't correct.  Recall that some UTF16 characters are
multibyte characters.

> JavaScriptCore/wtf/text/AtomicString.cpp:391
> +    // Try converting into the buffer.
> +    if (convertUTF8ToUTF16(&characters, characters + length, &buffer,
bufferEnd) != conversionOK)
> +	   return AtomicString();
> +
> +    // stringBuffer is full (the input must have been all ascii), use the
string.
> +    if (buffer == bufferEnd)
> +	   return AtomicString(stringBuffer.get());

This seems really sketchy.  Why not just figure out what the correct length is?