[webkit-reviews] review denied: [Bug 47896] [GTK] Use GCharsetConverter instead of g_iconv in TextCodecGtk : [Attachment 71150] Patch to use GCharsetConverter

[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 71150: Patch to use GCharsetConverter
https://bugs.webkit.org/attachment.cgi?id=71150&action=review

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

Great patch! Needs a few fixes, but I like where this is headed.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:322
> +    , m_iconvDecoder(0)
> +    , m_iconvEncoder(0)

No need to initialize these. The default constructor sets the RefPtr to 0.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:359
> +    gsize bytesRead, bytesWritten;
> +    const gchar *input = bytes;

bytesRead and bytesWritten should be initialized and on separate lines. Should
be const gchar* input = bytes.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:362
> +    int flags = !length ?  G_CONVERTER_INPUT_AT_END : G_CONVERTER_NO_FLAGS;

There's an extra space after ?

> WebCore/platform/text/gtk/TextCodecGtk.cpp:392
> +	   GOwnPtr<GError> err;
> +	   GConverterResult res;
> +
> +	   res = g_converter_convert(G_CONVERTER(m_iconvDecoder.get()),
> +				     input,
> +				     inputLength,
> +				     buffer,
> +				     sizeof(buffer),
> +				     (GConverterFlags)flags,
> +				     &bytesRead,
> +				     &bytesWritten,
> +				     &err.outPtr());

Please change "err" to "error".

No need to define and declare res separately. In general, I like this style of
indenting, but in this case I think that it makes the method harder to read,
because it is so long. Perhaps it could be like this?

GConverterResult res = g_converter_convert(G_CONVERTER(m_iconvDecoder.get()),
input, inputLength
					   buffer, sizeof(buffer), ...

I think you should also use a static_cast for flags.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:399
> +	       if (g_error_matches(err.get(), G_IO_ERROR,
G_IO_ERROR_PARTIAL_INPUT)) {
> +		   memcpy(m_bufferedBytes, input, inputLength);
> +		   m_numBufferedBytes = inputLength;

You might want to also leave a comment here about why this might happen and
what the strategy is for dealing with it, since it's not obvious from an
initial inspection.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:440
> +    gsize bytesRead, bytesWritten;
> +    const gchar *input = reinterpret_cast<const char*>(characters);

bytesRead and bytesWritten should be initialized and on separate lines. Careful
with the * again.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:444
>      GOwnPtr<GError> err;

Probably a good idea to get rid of this abbreviation now.

> WebCore/platform/text/gtk/TextCodecGtk.cpp:458
> +	   res = g_converter_convert(G_CONVERTER(m_iconvEncoder.get()),
> +				     input,
> +				     inputLength,
> +				     buffer,
> +				     sizeof(buffer),
> +				     G_CONVERTER_INPUT_AT_END,
> +				     &bytesRead,
> +				     &bytesWritten,
> +				     &err.outPtr());

Same comment here as above.