[webkit-reviews] review denied: [Bug 47896] [GTK] Use GCharsetConverter instead of g_iconv in TextCodecGtk : [Attachment 71308] Updated patch according to review

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 47896: [GTK] Use GCharsetConverter instead of g_iconv in TextCodecGtk
https://bugs.webkit.org/show_bug.cgi?id=47896

Attachment 71308: Updated patch according to review
https://bugs.webkit.org/attachment.cgi?id=71308&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71308&action=review

I have a couple more nits, but it's very close.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:348
> +	   ASSERT(m_iconvDecoder);
> +	   if (!m_iconvDecoder) {

Sorry, I missed this before. Either we should remove the ASSERT here or not
handle the failure case.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:393
> +		   // there is not enough input to fully determine what the
conversion should produce
> +		   // save it to a buffer to prepend it to the next input

Nice comment, but it should be a full sentence with punctuation, etc.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:399
> +	       } else if (g_error_matches(error.get(), G_IO_ERROR,
G_IO_ERROR_NO_SPACE)) {
> +		   bufferWasFull = true;
> +	       } else if (g_error_matches(error.get(), G_IO_ERROR,
G_IO_ERROR_INVALID_DATA)) {

WebKit style dictates that one-line blocks omit their surrounding curly braces
(as much as I dislike it).

> WebCore/platform/text/gtk/TextCodecGtk.cpp:410
> +		   m_numBufferedBytes = 0; // reset state for subsequent calls
to decode

This comment should also be in sentence form.