[webkit-reviews] review denied: [Bug 28938] CSS string value is not correctly serialized when it contains binary characters : [Attachment 52372] Patch v5.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yuta Kitamura <yutak at chromium.org>'s
request for review:
Bug 28938: CSS string value is not correctly serialized when it contains binary
characters
https://bugs.webkit.org/show_bug.cgi?id=28938

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

------- Additional Comments from Darin Adler <darin at apple.com>
Great work. It's good to handle these cases correctly.

Looking at the tokenizer it seems that tab characters are legal in a CSS quoted
string. But we are choosing to escape them as "\9". I guess that's OK.

> +	       String escape = String::format("\\%x", static_cast<int>(ch));
> +	       for (unsigned j = 0; j < escape.length(); ++j)
> +		   buffer[index++] = escape[j];

This is excessive memory allocation and complexity to convert a two digit
number to hex.

Instead something like this:

    static const char hexDigits[17] = "0123456789ABCDEF";
    buffer[index++] = '\\';
    if (ch >= 0x10)
	buffer[index++] = hexDigits[ch >> 4];
    buffer[index++] = hexDigits[ch & 0xF];

> +	       // Space characeter may be required to separate backslash-escape
sequence and normal characters.

Typo: "characeter".

These functions don't belong in CSSPrimitiveValue.h. I think they should move
to CSSParser.h.

review- because I'd like to see the code changed to not use String::format.