Bug 78487 – [GTK] Add GSList to the list of GObject types in GOwnPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

This would be a nice addition to handle GSList's as returned from some functions (e.g. gtk_file_chooser_get_uris).

[Mario Sanchez Prada]

Created attachment 126759 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 126759 [details]
Patch

Looks good

[Philippe Normand]

Comment on attachment 126759 [details]
Patch

Ok! Where will that new smart pointer will be used? Can you "migrate" some existing code as part of this patch?

[Mario Sanchez Prada]

Created attachment 126763 [details]
Patch

(In reply to comment #3)
> (From update of attachment 126759 [details])
> Ok! Where will that new smart pointer will be used? Can you "migrate" some existing code as part of this patch?

It will be used in the code I'm now writing to implement the WebUIClient's runOpenPanel() feature in WebKit2GTK+, when handling the list of fileURIs as returned by gtk_file_chooser_get_uris(), which happens to be an GSList.

The same kind of usage is already in WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp, so I guess it makes sense to migrate that code along with this patch, thus I'm attaching a new one incorporating that suggestion.

[Philippe Normand]

Comment on attachment 126763 [details]
Patch

Awesome! Thanks!

[Mario Sanchez Prada]

Committed r107566: <http://trac.webkit.org/changeset/107566>

[Martin Robinson]

Comment on attachment 126759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126759&action=review

> Source/JavaScriptCore/wtf/gobject/GOwnPtr.cpp:42
> +    g_slist_free(ptr);

You should check if the value is null before freeing it. While g_slist_free may properly handle null values, it isn't documented, so a check is safer.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 126759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126759&action=review
> 
> > Source/JavaScriptCore/wtf/gobject/GOwnPtr.cpp:42
> > +    g_slist_free(ptr);
> 
> You should check if the value is null before freeing it. While g_slist_free may properly handle null values, it isn't documented, so a check is safer.

No, you shoulnd't, NULL is a valid empty GSList.

[Martin Robinson]

(In reply to comment #8)

> > You should check if the value is null before freeing it. While g_slist_free may properly handle null values, it isn't documented, so a check is safer.
> 
> No, you shoulnd't, NULL is a valid empty GSList.

The documentation for g_slist_free seems lacking, then. :/

[Carlos Garcia Campos]

"There is no function to create a GList. NULL is considered to be the empty list so you simply set a GList* to NULL."

[Martin Robinson]

(In reply to comment #10)
> "There is no function to create a GList. NULL is considered to be the empty list so you simply set a GList* to NULL."

The g_slist_free makes no reference to this fact nor does it explicitly say that it handles empty GLists.

[Carlos Garcia Campos]

Because all GList and GSList API accept NULL due to the fact that NULL is a valid empty list as documented in the description section of the documentation.

[Martin Robinson]

(In reply to comment #12)
> Because all GList and GSList API accept NULL due to the fact that NULL is a valid empty list as documented in the description section of the documentation.

This is just an assumption one would make from reading the documentation though. All that's explicitly stated is that NULL is a valid GList/GSList. Indeed here is someone who made a mistake because of this: http://code.google.com/codesearch#-q96OtJF3X4/trunk/xdelta1/libedsio/edsio.c&q=g_list_free&type=cs&l=1501 It's rare that documentation is too-explicit. I don't think situations like these are the users' fault. This debate probably doesn't belong on this bug though...

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > Because all GList and GSList API accept NULL due to the fact that NULL is a valid empty list as documented in the description section of the documentation.
> 
> This is just an assumption one would make from reading the documentation though. All that's explicitly stated is that NULL is a valid GList/GSList. 

g_list_free() receives a GList, NULL is a valid empty GList as documented, so GList can receive NULL.