Bug 27424

Created attachment 33050 [details] adds GStringConvert.h

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.

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.

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

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

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()); > +2008-11-30 lkcl <lkcl@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.

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

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

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.

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

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

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"

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?

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

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

Created attachment 34287 [details] updated to include 2009 copyright

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.

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

All reviewed patches have been landed. Closing bug.

Created attachment 34470 [details] uploaded