Bug 78487

Summary: [GTK] Add GSList to the list of GObject types in GOwnPtr
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: WebKitGTKAssignee: Mario Sanchez Prada <mario>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, mrobinson, pnormand
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
pnormand: review+, mrobinson: commit-queue-
Patch pnormand: review+

Description Mario Sanchez Prada 2012-02-13 05:17:37 PST
This would be a nice addition to handle GSList's as returned from some functions (e.g. gtk_file_chooser_get_uris).
Comment 1 Mario Sanchez Prada 2012-02-13 05:24:10 PST
Created attachment 126759 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-02-13 05:34:00 PST
Comment on attachment 126759 [details]
Patch

Looks good
Comment 3 Philippe Normand 2012-02-13 05:37:55 PST
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?
Comment 4 Mario Sanchez Prada 2012-02-13 06:04:01 PST
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.
Comment 5 Philippe Normand 2012-02-13 06:07:57 PST
Comment on attachment 126763 [details]
Patch

Awesome! Thanks!
Comment 6 Mario Sanchez Prada 2012-02-13 06:18:30 PST
Committed r107566: <http://trac.webkit.org/changeset/107566>
Comment 7 Martin Robinson 2012-02-13 08:11:13 PST
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.
Comment 8 Carlos Garcia Campos 2012-02-13 08:14:20 PST
(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.
Comment 9 Martin Robinson 2012-02-13 08:16:47 PST
(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. :/
Comment 10 Carlos Garcia Campos 2012-02-13 08:20:28 PST
"There is no function to create a GList. NULL is considered to be the empty list so you simply set a GList* to NULL."
Comment 11 Martin Robinson 2012-02-13 08:25:23 PST
(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.
Comment 12 Carlos Garcia Campos 2012-02-13 08:31:20 PST
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.
Comment 13 Martin Robinson 2012-02-13 08:35:06 PST
(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...
Comment 14 Carlos Garcia Campos 2012-02-13 08:41:39 PST
(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.