Bug 27424 – adding ConvertToGCharPrivate.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Luke Kenneth Casson Leighton]

the unusual step of adding one header file as part of the gobject bindings is due to the review requests made.  mark, you were right: #include "unicode/ustring.h" wasn't necessary - well spotted.

[Luke Kenneth Casson Leighton]

Created attachment 33050 [details]
adds GStringConvert.h

[Luke Kenneth Casson Leighton]

important to note: this file is being added as part of a series to break
#16401 down into manageable sections, as per agreement, under an initiative
recommended by david.

[Holger Freyther]

Comment on attachment 33050 [details]
adds GStringConvert.h

This is using WebCore types, so the above header file is internal, this means you should follow the WebCore coding style guidelines, which you don't. The first issue i notice is the placement of the '*'.

On the question of content, I wonder if the gdom/ directory is supposed to hold public API as well? if that is the case you should put the GStringConvert somewhere else, and calling it GStringConvert is a bit misleading as GString != gchar.

please revise.

[Luke Kenneth Casson Leighton]

(In reply to comment #3)
> (From update of attachment 33050 [details])
> This is using WebCore types, so the above header file is internal, this means
> you should follow the WebCore coding style guidelines, which you don't. The
> first issue i notice is the placement of the '*'.

 ack, got it.  also restyled the functions - hope that's what you meant.
 
> On the question of content, I wonder if the gdom/ directory is supposed to hold
> public API as well? 

 the files that are to be public api are copied out of there, to a location
 under INCLUDEDIR/gdom so that the #includes can have "gdom/" in them on
 public builds as well as internal ones.

 so, gdom.h is kept there, as are GdomDOMObject.h - all the rest are private
 and/or internal.

 it's a bit of a grey area to me to be honest and i'm surprised i managed
 to come up with something that doesn't make everything fall over [by
 accidentally referencing out-of-date headers]

 so it's all an area where i welcome input but equally i would be very
 relieved by a decision which recognised that this is all a bit iffy
 and that resolving it requires significant additional thought and
 probably extra code, and said "we'll leave this one for now, flag it
 as an issue under a separate bugreport / TODO".
 
> if that is the case you should put the GStringConvert
> somewhere else, and calling it GStringConvert is a bit misleading as GString !=
> gchar.

 ok renamed to gdom/ConvertToGCharPrivate.h to indicate that it's internal.


> please revise.

 done - what do you think?

 l.

[Luke Kenneth Casson Leighton]

Created attachment 33088 [details]
rename to ConvertToCGharPrivate.h and fix up coding standards

[Mark Rowe (bdash)]

Comment on attachment 33088 [details]
rename to ConvertToCGharPrivate.h and fix up coding standards

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

> [phone]  lkcl  <[e-mail]>

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.

[Luke Kenneth Casson Leighton]

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

> > [phone]  lkcl  <[e-mail]>
> 
> 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.

[Luke Kenneth Casson Leighton]

Created attachment 33388 [details]
addressing review comments, decision on "Private" in name still outstanding decision?

[Eric Seidel (no email)]

Comment on attachment 33388 [details]
addressing review comments, decision on "Private" in name still outstanding decision?

Most of the rest of WebCore follows the "copy rule", which would name these with "copy" in their name since you're copying the data, or create.

copyAsGchar()
or
createGcharFromString()

I've never seen this:
23 /*
 24  * added under bug #27424 - 19jul2009
 25  */

Seems superfluous, given that we have a version control system.

I'm also not sure what Private_h is for, but that was discussed in the long comments above (which I admit to not reading, given their length).

So overall these look fine.  I'm not a huge fan of the convert name, but it's also not a dealbreaker.

Since you're not a committer yet, r- for the unneeded comment.

[Luke Kenneth Casson Leighton]

(In reply to comment #9)
> (From update of attachment 33388 [details])
> Most of the rest of WebCore follows the "copy rule", which would name these
> with "copy" in their name since you're copying the data, or create.
> 
> copyAsGchar()
> or
> createGcharFromString()
> 
> I've never seen this:
> 23 /*
>  24  * added under bug #27424 - 19jul2009
>  25  */
> 
> Seems superfluous, given that we have a version control system.

 gone.  i'm pseudo-maintaining external revision control, and worked out a better manual system [copy what people do with the ChangeLog entries.  duh].
 
> So overall these look fine.  I'm not a huge fan of the convert name, but it's
> also not a dealbreaker.

 renamed to copyAsGchar to fit into WebCore rules.

[Luke Kenneth Casson Leighton]

Created attachment 33979 [details]
addressing review comments, rename function to copyAsGchar to conform to WebCore rules

[Eric Seidel (no email)]

Comment on attachment 33979 [details]
addressing review comments, rename function to copyAsGchar to conform to WebCore rules

LGTM.  I still don't understand why these are named "Private.h"

[Adam Barth]

Comment on attachment 33979 [details]
addressing review comments, rename function to copyAsGchar to conform to WebCore rules

This file does not have the usual license block.  Is that ok?

[Luke Kenneth Casson Leighton]

(In reply to comment #12)
> (From update of attachment 33979 [details])
> LGTM.  I still don't understand why these are named "Private.h"

 the principle was that so they don't get used by anything other than gobject code.  seemed like a good idea - nobody's objected, so... *shrug*

[Luke Kenneth Casson Leighton]

(In reply to comment #13)
> (From update of attachment 33979 [details])
> This file does not have the usual license block.  Is that ok?

 it's been created from scratch, with no input [other than review comments] from any other editors.  therefore, the only copyright holder is: me.

 if by "usual license block" you mean "it doesn't have apple's copyright notice in it" then the reason is simple - nobody from apple has typed any lines of code that contribute to this file, so they don't own any copyright on it.

 it's still an LGPL'd file, and.... ah, it says 2008, not 2008+2009, and i edited it recently.

 will update.

[Luke Kenneth Casson Leighton]

Created attachment 34287 [details]
updated to include 2009 copyright

[Eric Seidel (no email)]

Comment on attachment 34287 [details]
updated to include 2009 copyright

License looks fine.  LGPL 2 is an accepted license for WebKit.  We prefer BSD for new code, but LGPL is OK.

[Adam Barth]

Comment on attachment 34287 [details]
updated to include 2009 copyright

Clearing review flag on attachment: 34287

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebKit/gtk/gdom/ConvertToGCharPrivate.h
Committed r46903
	A	WebKit/gtk/gdom/ConvertToGCharPrivate.h
	M	WebCore/ChangeLog
r46903 = 77f98f171fdb8c7dc1479d33b6e55aeffa9a237e (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46903

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Gour]

Created attachment 34470 [details]
uploaded