[webkit-reviews] review denied: [Bug 5232] Rewrite QTextCodec::fromUnicode to use ICU : [Attachment 5047] revised patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Alexey Proskuryakov <ap at nypop.com>'s
request for review:
Bug 5232: Rewrite QTextCodec::fromUnicode to use ICU
http://bugzilla.opendarwin.org/show_bug.cgi?id=5232

Attachment 5047: revised patch
http://bugzilla.opendarwin.org/attachment.cgi?id=5047&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looking great!

If we want to keep this out of "mainline" then we can't check into tip of tree.
We need TOT to be something suitable for shipping before Leopard -- it's not OK
to knowingly regress something in a way that's only fixed by Leopard.

There's no need to call QByteArray::resize directly -- QCString::resize doesn't
do any call to strlen, and in fact it sets the terminating 0 byte correctly, so
this:

	static_cast<QByteArray&>(result).resize(resultLength + chunkLength +
1);
	memcpy(result.data() + resultLength, buffer, chunkLength);
	result.data()[resultLength + chunkLength] = '\0';

can just be:

	result.resize(resultLength + chunkLength + 1);
	memcpy(result.data() + resultLength, buffer, chunkLength);

I think we really need to consider what to do about the character sets that ICU
can't handle and CF can. First step I suppose would be to inventory which
character sets those are.

I think that the call to ucnv_setSubstChars needs its own ASSERT about the
error code returned.