Bug 127431

Summary: [GLIB] Use GUniquePtr instead of GOwnPtr
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, bunhere, cdumez, cfleizach, commit-queue, danw, dmazzoni, eric.carlson, glenn, gustavo, gyuyoung.kim, jcraig, jdiggs, jer.noble, mario, menard, mrobinson, pnormand, rakuco, samuel_white
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patch mrobinson: review+

Description Carlos Garcia Campos 2014-01-22 10:27:32 PST
GUniquePtr is just a template alias of std::unique_ptr with a custo deleter that replaces GOwnPtr.
Comment 1 Carlos Garcia Campos 2014-01-22 10:37:32 PST
Created attachment 221873 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-22 10:38:52 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 WebKit Commit Bot 2014-01-22 10:39:14 PST
Attachment 221873 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:447:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 121 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Carlos Garcia Campos 2014-01-22 10:47:31 PST
Created attachment 221874 [details]
Updated patch

Sorry, I forgot to git add a file.
Comment 5 WebKit Commit Bot 2014-01-22 10:50:15 PST
Attachment 221874 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:447:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 122 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Martin Robinson 2014-01-22 10:54:25 PST
Comment on attachment 221874 [details]
Updated patch

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

> Source/WebCore/platform/gtk/GOwnPtrGtk.cpp:-31
> -template <> void freeOwnedGPtr<GdkEvent>(GdkEvent* ptr)
> -{
> -    if (ptr)
> -        gdk_event_free(ptr);
> -}
> -

A specialization isn't necessary for GUniquePtr?
Comment 7 Carlos Garcia Campos 2014-01-22 10:58:09 PST
(In reply to comment #6)
> (From update of attachment 221874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221874&action=review
> 
> > Source/WebCore/platform/gtk/GOwnPtrGtk.cpp:-31
> > -template <> void freeOwnedGPtr<GdkEvent>(GdkEvent* ptr)
> > -{
> > -    if (ptr)
> > -        gdk_event_free(ptr);
> > -}
> > -
> 
> A specialization isn't necessary for GUniquePtr?

Yes, good catch! I forgot to git add another file :-P
Comment 8 Carlos Garcia Campos 2014-01-22 11:01:17 PST
Created attachment 221876 [details]
Updated patch
Comment 9 WebKit Commit Bot 2014-01-22 11:04:01 PST
Attachment 221876 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:446:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:447:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 123 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Martin Robinson 2014-01-22 17:28:57 PST
Comment on attachment 221876 [details]
Updated patch

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

This is a very large patch, but appears to be largely mechanical.

> Source/WebCore/platform/network/soup/GUniquePtrSoup.h:2
> + *  Copyright (C) 2010 Igalia S.L

Probably want to use 2014 here.

> Source/WebCore/platform/network/soup/GUniquePtrSoup.h:24
> +#include <libsoup/soup.h>
> +#include <wtf/gobject/GUniquePtr.h>

One disadvantage of this is that we include third-party headers in more places, so this has the potential to impact compilation time. I suppose that isn't a *huge* issue though.
Comment 11 Carlos Garcia Campos 2014-01-22 23:46:04 PST
(In reply to comment #10)
> (From update of attachment 221876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221876&action=review
> 
> This is a very large patch, but appears to be largely mechanical.

Yeah, thanks for the quick review.

> > Source/WebCore/platform/network/soup/GUniquePtrSoup.h:2
> > + *  Copyright (C) 2010 Igalia S.L
> 
> Probably want to use 2014 here.

Sure.

> > Source/WebCore/platform/network/soup/GUniquePtrSoup.h:24
> > +#include <libsoup/soup.h>
> > +#include <wtf/gobject/GUniquePtr.h>
> 
> One disadvantage of this is that we include third-party headers in more places, so this has the potential to impact compilation time. I suppose that isn't a *huge* issue though.

Yes, I thought about that too, the advantage is that you can use those just including the header. I haven't noticed any regression in the build time, but I'll check if we can improve it anyway.
Comment 12 Carlos Garcia Campos 2014-01-22 23:49:33 PST
Committed r162599: <http://trac.webkit.org/changeset/162599>