[webkit-reviews] review requested: [Bug 15914] [GTK] Implement Unicode functionality using GLib : [Attachment 41866] 2/4 - Moving Text Codecs to GLib (v3)

[bugzilla-daemon at webkit.org]

Dominik Röttsches <dominik.roettsches at access-company.com> has asked  for
review:
Bug 15914: [GTK] Implement Unicode functionality using GLib
https://bugs.webkit.org/show_bug.cgi?id=15914

Attachment 41866: 2/4 - Moving Text Codecs to GLib (v3)
https://bugs.webkit.org/attachment.cgi?id=41866&action=review

------- Additional Comments from Dominik Röttsches
<dominik.roettsches at access-company.com>
(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.