[webkit-reviews] review denied: [Bug 27424] adding ConvertToGCharPrivate.h : [Attachment 33088] rename to ConvertToCGharPrivate.h and fix up coding standards

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Luke Kenneth Casson Leighton
<lkcl at lkcl.net>'s request for review:
Bug 27424: adding ConvertToGCharPrivate.h
https://bugs.webkit.org/show_bug.cgi?id=27424

Attachment 33088: rename to ConvertToCGharPrivate.h and fix up coding standards
https://bugs.webkit.org/attachment.cgi?id=33088&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> Index: WebKit/gtk/gdom/ConvertToGCharPrivate.h

Based on your previous comments and the fact that this deals with WebCore
types, this sounds like a file that's not intended to be used outside of
building WebKit.  Is there a reason that it needs the "Private" suffix?

> +/*
> + * added under bug #27424 - 19jul2009
> + */
> +
> +/* convenience c++ functions for converting various different webkit
> + * string types into a utf8 glib string.
> + */

These comments don't add anything.

ConvertToGchar doesn't fit our naming conventions, which states that functions
should not start with an initial capital letter.  The "Gchar" strikes me as a
little odd too, but I guess that's what falls out of the lowercase "gchar"
being camel-cased.  Returning a raw pointer that expect the caller to free
seems to be asking for memory leaks.  Is there some way we can make this
interface safer?

> +inline gchar* ConvertToGchar(WebCore::String const& s)
> +{
> +    return g_strdup(s.utf8().data());
> +}

There should be a blank line between each function.

> +inline gchar* ConvertToGchar(WebCore::KURL const& s)
> +{
> +    return g_strdup(s.string().utf8().data());
> +}

This could be expressed more concisely as: return ConvertToGchar(s.string());

> +inline gchar* ConvertToGchar(const JSC::UString& s)
> +{
> +    return g_strdup(s.UTF8String().c_str());
> +}

> +inline gchar* ConvertToGchar(WebCore::AtomicString const&s)
> +{
> +    return g_strdup(s.string().utf8().data());
> +}

Missing a space after the &.  This could be expressed more concisely as: return
ConvertToGchar(s.string());

> +2008-11-30  lkcl  <lkcl at lkcl.net>

Please update this with the current date and your name.

> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   WARNING: NO TEST CASES ADDED OR CHANGED

This line is an intended only as an informative note for the patch author.

> +	   (GStringConvert): bug #27424 - added ConvertToGCharPrivate.h to help
GObject bindings convert various types to gobject gchar* 

Please take a look at the format of our other ChangeLog entries for how your
entry should be structured.  The prepare-ChangeLog script can be used to assist
in the creation of a ChangeLog entry for your change.