[Webkit-unassigned] [Bug 15914] [GTK] Implement Unicode functionality using GLib

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 25 11:54:16 PDT 2009


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


Gustavo Noronha (kov) <gns at gnome.org> changed:

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




--- Comment #86 from Gustavo Noronha (kov) <gns at gnome.org>  2009-10-25 11:54:12 PDT ---
(From update of attachment 41434)
> @@ -114,6 +117,20 @@ CString TextEncoding::encode(const UChar
>      QString str(reinterpret_cast<const QChar*>(characters), length);
>      str = str.normalized(QString::NormalizationForm_C);
>      return newTextCodec(*this)->encode(reinterpret_cast<const UChar *>(str.utf16()), str.length(), handling);
> +#elif USE(GLIB_UNICODE)
> +    GOwnPtr<char> utf8source;
> +    utf8source.set(g_utf16_to_utf8(characters, length, 0, 0, 0));
> +
> +    GOwnPtr<char> utf8normalized;
> +    utf8normalized.set(g_utf8_normalize(utf8source.get(), -1, G_NORMALIZE_NFC));
> +
> +    long utf16length;
> +    GOwnPtr<UChar> utf16normalized;
> +    utf16normalized.set(g_utf8_to_utf16(utf8normalized.get(), -1, 0, &utf16length, 0));

Naming of the variables is a bit off our style. UTF8Normalized is more like it,
for instance.

> +    CString result = newTextCodec(*this)->encode(utf16normalized.get(), utf16length, handling);
> +
> +    return result;

No need for a variable, just return the newTextCodec call directly.

> +#include "gtk/TextCodecGtk.h"
> +#endif
>  #if PLATFORM(WINCE) && !PLATFORM(QT)
>  #include "TextCodecWince.h"
>  #endif
>  
> +
>  using namespace WTF;

Why this space?

> +namespace WebCore {
> +
> +
> +

Too many blank lines to my taste =).

> +// list of text encodings and their aliases
> +// the first entry/"alias" is the canonical name, each alias list must be terminated by a 0

Comments should in general be full sentences, starting with a capital letter,
and ending in a period.

> +// Unicode
> +TextCodecGtk::codecAliasList TextCodecGtk::m_codecAliases_UTF_8            = { "UTF-8", 0}; 

You're missing a space before } on all of these. I don't think we have a style
rule for this, but it looks inconsistent with the fact that you have one at the
start =).

> +static PassOwnPtr<TextCodec> newTextCodecGtk(const TextEncoding& encoding, const void*)
> +{
> +#ifndef NDEBUG
> +    // too noisy if each UTF-8 codec creation is logged
> +    // if (strcmp(encoding.name(), "UTF-8"))
> +    //     LOG(TextConversion, "creating new text codec for encoding name %s", encoding.name());
> +#endif

This is dead code. Either let it log stuff, or remove the code altogether,
please. I think it may make sense to log in a debug build, but up to you.

> +    for (unsigned int i = 0; i < listLength; ++i) {
> +        codecAliasList *codecAliases = static_cast<codecAliasList*>(encodingList[i]);
> +        int codecCount = -1;
> +        const gchar *canonicalName, *currentAlias;
> +        bool canonicalAvailable = true;
> +        
> +        // for each of the alias lists:
> +        // bail out if canonical is not available,
> +        // then continue testing each alias, 
> +        // if available, add it to the list, mapping it to its canonical name
> +
> +        while ((currentAlias = (*codecAliases)[++codecCount]) && canonicalAvailable) {
> +            bool currentAvailable = isEncodingAvailable(currentAlias);
> +
> +            if (codecCount == 0 && currentAvailable) {
> +                canonicalName = currentAlias;
> +                canonicalAvailable = true;
> +                LOG_VERBOSE(TextConversion, "registering encoding canonical %s", canonicalName);
> +                registrar(canonicalName, canonicalName);
> +            } else if (codecCount == 0 && !currentAvailable)
> +                canonicalAvailable = false;

This feels a bit convoluted. Have you considered moving these two condition
checks before the while loop? There's no need to check them on every loop run,
since they are only useful in the first one.

> +TextCodecGtk::~TextCodecGtk()
> +{
> +    releaseIConv();
> +}
> +
> +void TextCodecGtk::releaseIConv() const
> +{
> +    if (m_iconvDecoder != reinterpret_cast<GIConv>(-1)) {
> +        g_iconv_close(m_iconvDecoder);
> +        m_iconvDecoder = reinterpret_cast<GIConv>(-1);
> +    }
> +    if (m_iconvEncoder != reinterpret_cast<GIConv>(-1)) {
> +        g_iconv_close(m_iconvEncoder);
> +        m_iconvEncoder = reinterpret_cast<GIConv>(-1);
> +    }
> +}

Why is this a separate function? Do you call releaseIConv anywhere else? Why
not put this code inside the destructor, directly?

> +    if (err) {
> +        LOG_ERROR("GIConv conversion error");

Would it be useful to log the error message here?

> +    CString result = CString(buffer.get(), count);
> +
> +    return result;

No need for a variable here.
> +#ifndef TextCodecGTK_h
> +#define TextCodecGTK_h
> +
> +#include <glib.h>
> +
> +#include "TextCodec.h"
> +#include "TextEncoding.h"

No need for that blank line between the includes.

Another round! =)

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



More information about the webkit-unassigned mailing list