[Webkit-unassigned] [Bug 47896] [GTK] Use GCharsetConverter instead of g_iconv in TextCodecGtk

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47896


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71150|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2010-10-20 09:39:04 PST ---
(From update of attachment 71150)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.