[webkit-reviews] review denied: [Bug 15914] [GTK] Implement Unicode functionality using GLib : [Attachment 23263] Replacing ICU with glib

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 23 04:34:39 PDT 2008

Eric Seidel <eric at webkit.org> has denied Dominik Röttsches
<dominik.roettsches at access-company.com>'s request for review:
Bug 15914: [GTK] Implement Unicode functionality using GLib

Attachment 23263: Replacing ICU with glib

------- Additional Comments from Eric Seidel <eric at webkit.org>
There are a bunch of webkit-style violations in this patch, which make it
difficult to r+.

It concerns me that you'd need to copy all those macros from the ICU headers. 
Does GLIB not provide any equivalents?

Things like where * and & go, relative to the variable name.
Single line if/else statements don't use {}
Use of c-style casts (Foo*)foo instead of static_cast<Foo*>(foo)
Each variable gets its own declaration line, no int foo, bar;
4-space indents
0 instead of NULL when referring to pointers in c++ code
true and false instead of TRUE and FALSE when dealing with c++ "bool" types
Header #define names should be identical to the header, like #define

Anything copied from Glib should really be in its own header file, IMO. 
Separating things out into their own file makes it easier to update them if
they ever change upstream, or for platforms which have the necessary headers to
not use our local copies of that data.

Probably the same goes of the ICU macros. UnicodeMacrosFromICU.h would be one
possible name. :)

WebKit source code files are generally UT8, I'm not sure what encoding you're
editing with:
+ * Copyright (C) 2008 Jürg Billeter <j at bitron.ch>

There is a lot of code in UnicodeGLib.h, much of that could be moved into a
corresponding .cpp file.

:(  Even though this code touches Gtk, it would be best if we could avoid
manual-menory management.  Manual memory management is *evil* (IMO) and gets
you into all sorts of trouble.	I think there was a GOwnPtr landed recently,
no?  That would know how to call g_free on its contents when it went out of
scope?	If so, one could use that instead of all the manual g_free calls.

Thanks for the patch, but I think we need another round of cleanup to this one.

More information about the webkit-reviews mailing list