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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 29 02:13:00 PDT 2007

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

Attachment 15303: patch updated

------- 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",

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.

More information about the webkit-reviews mailing list