[webkit-reviews] review denied: [Bug 13415] Add UTF-32 support for html/xml documents : [Attachment 15303] patch updated

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jungshik Shin
<jungshik.shin at gmail.com>'s request for review:
Bug 13415: Add UTF-32 support for html/xml documents
http://bugs.webkit.org/show_bug.cgi?id=13415

Attachment 15303: patch updated
http://bugs.webkit.org/attachment.cgi?id=15303&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
It seems that the change to html4.css does not belong to this patch.

+ setEncoding(((ptr - m_buffer.data()) % 4) ? "UTF-32LE" : "UTF-32BE",
AutoDetectedEncoding);

Since you've replaced encoding names with function calls in several places, I
suggest using those here, as well.

+ //else if (numBufferedBytes >= 3 || length >= 3) 

We really do not like commented-out code in sources, please remove this line.

Coming to more important issues: I don't think I understand the algorithm in
TextDecoder::checkForBOM(). Why did you change m_bufferedBytes size to 4 bytes?
If we have already seen 4 bytes of the input, then we are done with BOM
detection, so only 3 bytes need be buffered. And the "encodingConsideringBOM !=
&m_encoding" check looks wrong, comparing TextEncoding by address is not a good
way to detect that we have successfully found a BOM.

This is extremely close to an r+, but I think that another iteration is needed
to iron out these issues.