[Webkit-unassigned] [Bug 27424] adding ConvertToGCharPrivate.h

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Luke Kenneth Casson Leighton <lkcl at lkcl.net>  2009-07-23 15:47:30 PDT ---
(In reply to comment #6)
> (From update of attachment 33088 [details])
> > 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.

 correct.  specifically the gobject bindings.  absolutely nobody else
 should be using it.

 i'd appreciate your advice on where it should go.

>  Is there a reason that it needs the "Private" suffix?

 i added it because i thought it was needed - i don't know all
 of the webkit conventions, so i can't answer.

 if you think it doesn't fit / isn't a good idea, i'll happily
 remove it.


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

 oh, ok.  gone.

> ConvertToGchar doesn't fit our naming conventions, which states that functions
> should not start with an initial capital letter.  

 ack, sorry.

> The "Gchar" strikes me as a
> little odd too, but I guess that's what falls out of the lowercase "gchar"
> being camel-cased.

 yehhh, i don't like it, either, but ... *shrug*.


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

 ah.  no.  you can't.  that's c for you.

 you have to obey the conventions of the framework that you're working with,
 and in this case, that's glib / gobject.

 the conventions are, for glib / gobject, that "caller frees", unless
 otherwise stated.

 i know - it made me nervous, too, but fortunately, the python gobject
 bindings auto-generator perfectly understands these conventions (and
 even supports the "unless otherwise stated" concept) so it auto-generates
 code which takes a copy of the string into a python "str" object and
 then calls g_free()

 g_object-based code is _riddled_ with g_free and g_object_unref() etc.
 calls.

> > +inline gchar* ConvertToGchar(WebCore::String const& s)
> > +{
> > +    return g_strdup(s.utf8().data());
> > +}
> 
> There should be a blank line between each function.

 ack.

> > +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());

 oo yeah, good point.

> > +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());

 well spotted.

> > +2008-11-30  lkcl  <lkcl at lkcl.net>
> 
> Please update this with the current date and your name.

 done.

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

 oh, right - i wondered why it was there.  ok - gone.  cut-and-paste-itis has a
few others still there (amongst the 15 patches) so please bear with me whilst i
update the patches to remove that (and the tabs as well).

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

 yehh, i ran that, originally, back in august / september 2008 - i'm now
maintaining 15 separate patches, with their entries all in the same ChangeLog
file(s), so it's a little challenging to use that automated tool, now, to say
the least.

 your patience really appreciated.

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