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

[bugzilla-daemon at webkit.org]

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


Dominik Röttsches <dominik.roettsches at access-company.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41434|0                           |1
        is obsolete|                            |
  Attachment #41866|                            |review?
               Flag|                            |




--- Comment #87 from Dominik Röttsches <dominik.roettsches at access-company.com>  2009-10-26 07:39:01 PDT ---
Created an attachment (id=41866)
 --> (https://bugs.webkit.org/attachment.cgi?id=41866)
2/4 - Moving Text Codecs to GLib (v3)

(In reply to comment #86)

Thanks a lot for your review, Gustavo!

> (From update of attachment 41434 [details])
> > @@ -114,6 +117,20 @@ CString TextEncoding::encode(const UChar

> > [...]
> > +    GOwnPtr<char> utf8source;
> > [...]
> > +    GOwnPtr<char> utf8normalized;
> > [...]
> > +    GOwnPtr<UChar> utf16normalized;
> 
> Naming of the variables is a bit off our style. UTF8Normalized is more like it,
> for instance.

If I understood the style guide correctly, it should be CamelCase starting with
a lowercase letter for variables, also lowercasing acronyms. so I changed it to
utf8Source, utf8Normalized etc. Hope that's fine now. 

> > +    CString result = newTextCodec(*this)->encode(utf16normalized.get(), utf16length, handling);
> > +
> > +    return result;
> 
> No need for a variable, just return the newTextCodec call directly.

Done.

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

Removed.

> > +namespace WebCore {
> > +
> > +
> > +
> 
> Too many blank lines to my taste =).

Removed.

> > +// 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.

Rephrased this comment as well as the Changelog a little bit.

> > +// 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 =).

Changed to match.

> > +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.

Removed the log statement here and as well as in isEncodingAvailable. Replaced
those by more accurate logging in registerEncodingNames.

> > +    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.

Yes, thanks for the remark. I tried to unclutter this and indeed it should be
much easier to read now.

> > +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?

"Inlined".

> > +    if (err) {
> > +        LOG_ERROR("GIConv conversion error");
> 
> Would it be useful to log the error message here?

Printing code and error message now in encode() and decode().

> > +    CString result = CString(buffer.get(), count);
> > +
> > +    return result;
> 
> No need for a variable here.

Removed.


> > +#ifndef TextCodecGTK_h
> > +#define TextCodecGTK_h
> > +
> > +#include <glib.h>
> > +
> > +#include "TextCodec.h"
> > +#include "TextEncoding.h"
> 
> No need for that blank line between the includes.

Removed.

In addition, due to the log error output I changed the canonical name for Mac
Central Europe Encoding from MACCENTRALEUROPE to MAC-CENTRALEUROPE and EUC_JP
and EUC_CN to EUC-JP and EUC-CN, so that they work with GLib/iconv.

> Another round! =)

Thanks again for your review.

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