Bug 21599 – [GTK] WebKit GTK needs a wrapper for ref counted glib/gobject structs

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Marco Barisione]

Add a GRefPtr that call g_object_ref/unref or other appropriate functions depending on the contained type.

[Marco Barisione]

Created attachment 24347 [details]
Add GRefPtr and start the conversion to use it

[Alp Toker]

Comment on attachment 24347 [details]
Add GRefPtr and start the conversion to use it

>Index: JavaScriptCore/GNUmakefile.am
>===================================================================
>--- JavaScriptCore/GNUmakefile.am	(revision 37591)
>+++ JavaScriptCore/GNUmakefile.am	(working copy)
>@@ -245,9 +245,10 @@
> 	JavaScriptCore/wtf/DisallowCType.h \
> 	JavaScriptCore/wtf/FastMalloc.h \
> 	JavaScriptCore/wtf/Forward.h \
>+	JavaScriptCore/wtf/GetPtr.h \
> 	JavaScriptCore/wtf/GOwnPtr.cpp \
> 	JavaScriptCore/wtf/GOwnPtr.h \
>-	JavaScriptCore/wtf/GetPtr.h \
>+	JavaScriptCore/wtf/GRefPtr.h \
> 	JavaScriptCore/wtf/HashCountedSet.h \
> 	JavaScriptCore/wtf/HashFunctions.h \
> 	JavaScriptCore/wtf/HashIterators.h \

^ Just a quick note: the source lists are sorted case-sensitively.

>Index: JavaScriptCore/wtf/GRefPtr.h
>===================================================================
>--- JavaScriptCore/wtf/GRefPtr.h	(revision 0)
>+++ JavaScriptCore/wtf/GRefPtr.h	(revision 0)
>@@ -0,0 +1,165 @@
>+/*
>+ *  Copyright (C) 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
>+ *  Copyright (C) 2008 Collabora Ltd.
>+ *
>+ *  This library is free software; you can redistribute it and/or
>+ *  modify it under the terms of the GNU Library General Public
>+ *  License as published by the Free Software Foundation; either
>+ *  version 2 of the License, or (at your option) any later version.
>+ *
>+ *  This library is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ *  Library General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU Library General Public License
>+ *  along with this library; see the file COPYING.LIB.  If not, write to
>+ *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ *  Boston, MA 02110-1301, USA.
>+ *
>+ */
>+
>+#ifndef WTF_GRefPtr_h
>+#define WTF_GRefPtr_h
>+
>+#include "AlwaysInline.h"
>+#include <algorithm>
>+#include <glib-object.h>

Please use forward declarations instead of including GLib headers in WTF headers. g_object_ref/g_object_unref take a gpointer anyway so the G_OBJECT() cast isn't essential.

Please fix this in the recently-added GOwnPtr.h when you get the opportunity too.

>+
>+namespace WTF {
>+    template <typename T> inline T* refPtr(T* ptr) { if (ptr) g_object_ref(G_OBJECT(ptr)); return ptr; }
>+    template <typename T> inline void derefPtr(T* ptr) { if (ptr) g_object_unref(G_OBJECT(ptr)); }
>+
>+    template <typename T> class GRefPtr {
>+    public:
>+        GRefPtr() : m_ptr(0) { }
>+        // GRefPtr takes the ownership of the ptr, making it easy to use it with GLib.
>+        GRefPtr(T* ptr) : m_ptr(ptr) { }
>+        GRefPtr(const GRefPtr& o) : m_ptr(o.m_ptr) { if (T* ptr = m_ptr) refPtr(ptr); }
>+
>+        ~GRefPtr() { if (T* ptr = m_ptr) derefPtr(ptr); }
>+
>+        template <typename U> GRefPtr(const GRefPtr<U>& o) : m_ptr(o.get()) { if (T* ptr = m_ptr) refPtr(ptr); }
>+
>+        T* get() const { return m_ptr; }
>+
>+        void clear() { if (T* ptr = m_ptr) derefPtr(ptr); m_ptr = 0; }
>+
>+        T& operator*() const { return *m_ptr; }
>+        ALWAYS_INLINE T* operator->() const { return m_ptr; }
>+
>+        bool operator!() const { return !m_ptr; }
>+
>+        // This conversion operator allows implicit conversion to bool but not to other integer types.
>+        typedef T* GRefPtr::*UnspecifiedBoolType;
>+        operator UnspecifiedBoolType() const { return m_ptr ? &GRefPtr::m_ptr : 0; }

You say that you don't allow implicit conversion to other integer types but you don't explain why not. That'd let us drop the '.get()' when calling into GTK+ functions -- pretty convenient and means less code to modify if I understand correctly.

>+
>+        GRefPtr& operator=(const GRefPtr&);
>+        GRefPtr& operator=(T*);
>+        template <typename U> GRefPtr& operator=(const GRefPtr<U>&);
>+
>+        void swap(GRefPtr&);
>+
>+    private:
>+        static T* hashTableDeletedValue() { return reinterpret_cast<T*>(-1); }
>+
>+        T* m_ptr;
>+    };
>+
>+    template <typename T> inline GRefPtr<T>& GRefPtr<T>::operator=(const GRefPtr<T>& o)
>+    {
>+        T* optr = o.get();
>+        if (optr)
>+            refPtr(optr);
>+        T* ptr = m_ptr;
>+        m_ptr = optr;
>+        if (ptr)
>+            derefPtr(ptr);
>+        return *this;
>+    }
>+
>+    template <typename T> template <typename U> inline GRefPtr<T>& GRefPtr<T>::operator=(const GRefPtr<U>& o)
>+    {
>+        T* optr = o.get();
>+        if (optr)
>+            refPtr(optr);
>+        T* ptr = m_ptr;
>+        m_ptr = optr;
>+        if (ptr)
>+            derefPtr(ptr);
>+        return *this;
>+    }
>+
>+    template <typename T> inline GRefPtr<T>& GRefPtr<T>::operator=(T* optr)
>+    {
>+        T* ptr = m_ptr;
>+        m_ptr = optr;
>+        if (ptr)
>+            derefPtr(ptr);
>+        return *this;
>+    }
>+
>+    template <class T> inline void GRefPtr<T>::swap(GRefPtr<T>& o)
>+    {
>+        std::swap(m_ptr, o.m_ptr);
>+    }
>+
>+    template <class T> inline void swap(GRefPtr<T>& a, GRefPtr<T>& b)
>+    {
>+        a.swap(b);
>+    }
>+
>+    template <typename T, typename U> inline bool operator==(const GRefPtr<T>& a, const GRefPtr<U>& b)
>+    {
>+        return a.get() == b.get();
>+    }
>+
>+    template <typename T, typename U> inline bool operator==(const GRefPtr<T>& a, U* b)
>+    {
>+        return a.get() == b;
>+    }
>+
>+    template <typename T, typename U> inline bool operator==(T* a, const GRefPtr<U>& b)
>+    {
>+        return a == b.get();
>+    }
>+
>+    template <typename T, typename U> inline bool operator!=(const GRefPtr<T>& a, const GRefPtr<U>& b)
>+    {
>+        return a.get() != b.get();
>+    }
>+
>+    template <typename T, typename U> inline bool operator!=(const GRefPtr<T>& a, U* b)
>+    {
>+        return a.get() != b;
>+    }
>+
>+    template <typename T, typename U> inline bool operator!=(T* a, const GRefPtr<U>& b)
>+    {
>+        return a != b.get();
>+    }
>+
>+    template <typename T, typename U> inline GRefPtr<T> static_pointer_cast(const GRefPtr<U>& p)
>+    {
>+        return GRefPtr<T>(static_cast<T*>(p.get()));
>+    }
>+
>+    template <typename T, typename U> inline GRefPtr<T> const_pointer_cast(const GRefPtr<U>& p)
>+    {
>+        return GRefPtr<T>(const_cast<T*>(p.get()));
>+    }
>+
>+    template <typename T> inline T* getPtr(const GRefPtr<T>& p)
>+    {
>+        return p.get();
>+    }
>+
>+} // namespace WTF
>+
>+using WTF::GRefPtr;
>+using WTF::refPtr;
>+using WTF::derefPtr;
>+using WTF::static_pointer_cast;
>+using WTF::const_pointer_cast;
>+
>+#endif // WTF_GRefPtr_h

This code seems to have a lot in common with RefPtr.h. I wonder if it'd be possible to generalise that template and base GRefPtr upon it or one of the other existing ones in WTF?

>Index: JavaScriptCore/ChangeLog
>===================================================================
>--- JavaScriptCore/ChangeLog	(revision 37591)
>+++ JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,35 @@
>[phone]  Marco Barisione  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        [GTK] WebKit GTK needs a wrapper for ref counted glib/gobject structs
>+        https://bugs.webkit.org/show_bug.cgi?id=21599
>+
>+        Add a GRefPtr class (similar to RefPtr) to handle ref counted
>+        structures allocated by GLib and start the conversion to use it.
>+
>+        * GNUmakefile.am:
>+        * wtf/GRefPtr.h: Added.
>+        (WTF::refPtr):
>+        (WTF::derefPtr):
>+        (WTF::GRefPtr::GRefPtr):
>+        (WTF::GRefPtr::~GRefPtr):
>+        (WTF::GRefPtr::get):
>+        (WTF::GRefPtr::clear):
>+        (WTF::GRefPtr::operator*):
>+        (WTF::GRefPtr::operator->):
>+        (WTF::GRefPtr::operator!):
>+        (WTF::GRefPtr::operator UnspecifiedBoolType):
>+        (WTF::GRefPtr::hashTableDeletedValue):
>+        (WTF::::operator):
>+        (WTF::::swap):
>+        (WTF::swap):
>+        (WTF::operator==):
>+        (WTF::operator!=):
>+        (WTF::static_pointer_cast):
>+        (WTF::const_pointer_cast):
>+        (WTF::getPtr):
>+
> 2008-10-14  Alexey Proskuryakov  <[e-mail]>
> 
>         Reviewed by Darin Adler.
>Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(revision 37591)
>+++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(working copy)
>@@ -26,6 +26,7 @@
> #include "FrameLoader.h"
> #include "FrameView.h"
> #include "FrameTree.h"
>+#include "GRefPtrGtk.h"
> #include "HTMLFormElement.h"
> #include "HTMLFrameElement.h"
> #include "HTMLFrameOwnerElement.h"
>@@ -266,13 +267,10 @@
>         return;
> 
>     WebKitWebView* webView = getViewFromFrame(m_frame);
>-    WebKitNetworkRequest* request = webkit_network_request_new(resourceRequest.url().string().utf8().data());
>+    GRefPtr<WebKitNetworkRequest> request = webkit_network_request_new(resourceRequest.url().string().utf8().data());
>     WebKitNavigationResponse response;
>+    g_signal_emit_by_name(webView, "navigation-requested", m_frame, request.get(), &response);
> 
>-    g_signal_emit_by_name(webView, "navigation-requested", m_frame, request, &response);
>-
>-    g_object_unref(request);
>-
>     if (response == WEBKIT_NAVIGATION_RESPONSE_IGNORE) {
>         (core(m_frame)->loader()->*policyFunction)(PolicyIgnore);
>         return;
>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(revision 37591)
>+++ WebKit/gtk/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>[phone]  Marco Barisione  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        [GTK] WebKit GTK needs a wrapper for ref counted glib/gobject structs
>+        https://bugs.webkit.org/show_bug.cgi?id=21599
>+
>+        Start the conversion to use GRefPtr.
>+
>+        * WebCoreSupport/FrameLoaderClientGtk.cpp:
>+        (WebKit::FrameLoaderClient::dispatchDecidePolicyForNavigationAction):
>+
> 2008-10-06  David Hyatt  <[e-mail]>
> 
>         Enable viewless Mac WebKit to paint some basic pages.
>Index: WebCore/GNUmakefile.am
>===================================================================
>--- WebCore/GNUmakefile.am	(revision 37591)
>+++ WebCore/GNUmakefile.am	(working copy)
>@@ -1730,6 +1730,8 @@
> 	WebCore/platform/gtk/EventLoopGtk.cpp \
> 	WebCore/platform/gtk/FileChooserGtk.cpp \
> 	WebCore/platform/gtk/FileSystemGtk.cpp \
>+	WebCore/platform/gtk/GRefPtrGtk.cpp \
>+	WebCore/platform/gtk/GRefPtrGtk.h \
> 	WebCore/platform/gtk/KURLGtk.cpp \
> 	WebCore/platform/gtk/KeyEventGtk.cpp \
> 	WebCore/platform/gtk/KeyboardCodes.h \
>Index: WebCore/platform/gtk/PopupMenuGtk.cpp
>===================================================================
>--- WebCore/platform/gtk/PopupMenuGtk.cpp	(revision 37591)
>+++ WebCore/platform/gtk/PopupMenuGtk.cpp	(working copy)
>@@ -36,14 +36,11 @@
> 
> PopupMenu::PopupMenu(PopupMenuClient* client)
>     : m_popupClient(client)
>-    , m_popup(0)
> {
> }
> 
> PopupMenu::~PopupMenu()
> {
>-    if (m_popup)
>-        g_object_unref(m_popup);
> }
> 
> void PopupMenu::show(const IntRect& rect, FrameView* view, int index)
>@@ -53,14 +50,14 @@
>     if (!m_popup) {
>         m_popup = GTK_MENU(gtk_menu_new());
> #if GLIB_CHECK_VERSION(2,10,0)
>-        g_object_ref_sink(G_OBJECT(m_popup));
>+        g_object_ref_sink(G_OBJECT(m_popup.get()));
> #else
>-        g_object_ref(G_OBJECT(m_popup));
>-        gtk_object_sink(GTK_OBJECT(m_popup));
>+        g_object_ref(G_OBJECT(m_popup.get()));
>+        gtk_object_sink(GTK_OBJECT(m_popup.get()));
> #endif

Cann't we support g_object_ref_sink() directly in the smart pointer now?

>-        g_signal_connect(m_popup, "unmap", G_CALLBACK(menuUnmapped), this);
>+        g_signal_connect(m_popup.get(), "unmap", G_CALLBACK(menuUnmapped), this);
>     } else
>-        gtk_container_foreach(GTK_CONTAINER(m_popup), reinterpret_cast<GtkCallback>(menuRemoveItem), this);
>+        gtk_container_foreach(GTK_CONTAINER(m_popup.get()), reinterpret_cast<GtkCallback>(menuRemoveItem), this);
> 
>     int x, y;
>     gdk_window_get_origin(GTK_WIDGET(view->hostWindow()->platformWindow())->window, &x, &y);
>@@ -81,20 +78,20 @@
> 
>         // FIXME: Apply the PopupMenuStyle from client()->itemStyle(i)
>         gtk_widget_set_sensitive(item, client()->itemIsEnabled(i));
>-        gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), item);
>+        gtk_menu_shell_append(GTK_MENU_SHELL(m_popup.get()), item);
>         gtk_widget_show(item);
>     }
> 
>-    gtk_menu_set_active(m_popup, index);
>+    gtk_menu_set_active(m_popup.get(), index);
> 
> 
>     // The size calls are directly copied from gtkcombobox.c which is LGPL
>     GtkRequisition requisition;
>-    gtk_widget_set_size_request(GTK_WIDGET(m_popup), -1, -1);
>-    gtk_widget_size_request(GTK_WIDGET(m_popup), &requisition);
>-    gtk_widget_set_size_request(GTK_WIDGET(m_popup), MAX(rect.width(), requisition.width), -1);
>+    gtk_widget_set_size_request(GTK_WIDGET(m_popup.get()), -1, -1);
>+    gtk_widget_size_request(GTK_WIDGET(m_popup.get()), &requisition);
>+    gtk_widget_set_size_request(GTK_WIDGET(m_popup.get()), MAX(rect.width(), requisition.width), -1);
> 
>-    GList* children = GTK_MENU_SHELL(m_popup)->children;
>+    GList* children = GTK_MENU_SHELL(m_popup.get())->children;
>     if (size)
>         for (int i = 0; i < size; i++) {
>             if (i > index)
>@@ -111,13 +108,13 @@
>         // Center vertically the empty popup in the combo box area
>         m_menuPosition.setY(m_menuPosition.y() - rect.height() / 2);
> 
>-    gtk_menu_popup(m_popup, NULL, NULL, reinterpret_cast<GtkMenuPositionFunc>(menuPositionFunction), this, 0, gtk_get_current_event_time());
>+    gtk_menu_popup(m_popup.get(), NULL, NULL, reinterpret_cast<GtkMenuPositionFunc>(menuPositionFunction), this, 0, gtk_get_current_event_time());
> }
> 
> void PopupMenu::hide()
> {
>     ASSERT(m_popup);
>-    gtk_menu_popdown(m_popup);
>+    gtk_menu_popdown(m_popup.get());
> }
> 
> void PopupMenu::updateFromElement()
>@@ -153,7 +150,7 @@
> void PopupMenu::menuRemoveItem(GtkWidget* widget, PopupMenu* that)
> {
>     ASSERT(that->m_popup);
>-    gtk_container_remove(GTK_CONTAINER(that->m_popup), widget);
>+    gtk_container_remove(GTK_CONTAINER(that->m_popup.get()), widget);
> }
> 
> }
>Index: WebCore/platform/gtk/GRefPtrGtk.h
>===================================================================
>--- WebCore/platform/gtk/GRefPtrGtk.h	(revision 0)
>+++ WebCore/platform/gtk/GRefPtrGtk.h	(revision 0)
>@@ -0,0 +1,36 @@
>+/*
>+ * Copyright (C) 2008 Collabora Ltd.
>+ *
>+ * This library is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Library General Public
>+ * License as published by the Free Software Foundation; either
>+ * version 2 of the License, or (at your option) any later version.
>+ *
>+ * This library is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Library General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Library General Public License
>+ * along with this library; see the file COPYING.LIB.  If not, write to
>+ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ * Boston, MA 02110-1301, USA.
>+ */
>+
>+#ifndef GRefPtrGtk_h
>+#define GRefPtrGtk_h
>+
>+#include <gtk/gtk.h>

Again, please avoid this include in WebCore headers and use forward declarations instead.

>+#include <wtf/GRefPtr.h>
>+
>+namespace WTF {
>+
>+  template <> GtkTargetList* refPtr(GtkTargetList* ptr);
>+  template <> void derefPtr(GtkTargetList* ptr);
>+
>+  template <> GdkCursor* refPtr(GdkCursor* ptr);
>+  template <> void derefPtr(GdkCursor* ptr);
>+
>+} // namespace WTF
>+
>+#endif // GRefPtrGtk_h
>Index: WebCore/platform/gtk/GRefPtrGtk.cpp
>===================================================================
>--- WebCore/platform/gtk/GRefPtrGtk.cpp	(revision 0)
>+++ WebCore/platform/gtk/GRefPtrGtk.cpp	(revision 0)
>@@ -0,0 +1,51 @@
>+/*
>+ *  Copyright (C) 2008 Collabora Ltd.
>+ *
>+ *  This library is free software; you can redistribute it and/or
>+ *  modify it under the terms of the GNU Lesser General Public
>+ *  License as published by the Free Software Foundation; either
>+ *  version 2 of the License, or (at your option) any later version.
>+ *
>+ *  This library is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ *  Lesser General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU Lesser General Public
>+ *  License along with this library; if not, write to the Free Software
>+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>+ */
>+
>+#include "config.h"
>+#include "GRefPtrGtk.h"
>+
>+namespace WTF {
>+
>+template <> GtkTargetList* refPtr(GtkTargetList* ptr)
>+{
>+    if (ptr)
>+        gtk_target_list_ref(ptr);
>+    return ptr;
>+}
>+
>+template <> void derefPtr(GtkTargetList* ptr)
>+{
>+    if (ptr)
>+        gtk_target_list_unref(ptr);
>+}
>+
>+template <> GdkCursor* refPtr(GdkCursor* ptr)
>+{
>+    if (ptr)
>+        gdk_cursor_ref(ptr);
>+    return ptr;
>+}
>+
>+template <> void derefPtr(GdkCursor* ptr)
>+{
>+    if (ptr)
>+        gdk_cursor_unref(ptr);
>+}
>+
>+}
>+
>Index: WebCore/platform/PopupMenu.h
>===================================================================
>--- WebCore/platform/PopupMenu.h	(revision 37591)
>+++ WebCore/platform/PopupMenu.h	(working copy)
>@@ -48,6 +48,7 @@
> typedef struct _GtkMenu GtkMenu;
> typedef struct _GtkMenuItem GtkMenuItem;
> typedef struct _GtkWidget GtkWidget;
>+#include "GRefPtrGtk.h"
> #include <wtf/HashMap.h>
> #include <glib.h>
> #elif PLATFORM(WX)
>@@ -164,7 +165,7 @@
>     bool m_scrollbarCapturingMouse;
> #elif PLATFORM(GTK)
>     IntPoint m_menuPosition;
>-    GtkMenu* m_popup;
>+    GRefPtr<GtkMenu> m_popup;
>     HashMap<GtkWidget*, int> m_indexMap;
>     static void menuItemActivated(GtkMenuItem* item, PopupMenu*);
>     static void menuUnmapped(GtkWidget*, PopupMenu*);

[Kalle Vahlman]

(In reply to comment #2)
> Please use forward declarations instead of including GLib headers in WTF
> headers. g_object_ref/g_object_unref take a gpointer anyway so the G_OBJECT()
> cast isn't essential.

It's actually harmful, since it's not just a cast, but a check for the type (which is in itself trivial, but adds up. g_type_check_instance_cast() is often seen at the top of profiling). Furthermore, g_object_ref/unref() already checks that G_IS_OBJECT() so the check is actually made twice...

[Maciej Stachowiak]

(In reply to comment #2)

> 
> >+
> >+        // This conversion operator allows implicit conversion to bool but not to other integer types.
> >+        typedef T* GRefPtr::*UnspecifiedBoolType;
> >+        operator UnspecifiedBoolType() const { return m_ptr ? &GRefPtr::m_ptr : 0; }
> 
> You say that you don't allow implicit conversion to other integer types but you
> don't explain why not. That'd let us drop the '.get()' when calling into GTK+
> functions -- pretty convenient and means less code to modify if I understand
> correctly.

We follow this pattern in all the WTF smart pointers. The reason is that we want to allow:

if (m_smartPtr) { ... }

but not:

if (m_smartPtr == 1) { ... }

Since it is not type safe and does not really make sense (it shouldn't be the case that all non-null pointers convert to 1). Implementing an implicit bool conversion in the straightforward way will accidentally allow the latter.

Supporting other integer conversions (especially in this bogus way) won't address the .get() issue, which is about implicit conversion to raw pointer.

In general we dont add that to our smart pointers because we want to avoid accidental violation of refcounting safety.

[Martin Robinson]

Created attachment 43030 [details]
Cleaned up patch against ToT

I feel that this functionality is important for preventing memory leaks in the GTK+ port, so I've attached a cleaned up version against ToT.

[Martin Robinson]

Created attachment 43087 [details]
Updated patch which includes the adoptRef idiom

Based on bdash's comments from IRC I'm attaching an updated patch. This version automatically bumps the reference count of incoming gobjects, but also includes an adoptGRef function which will create a GRefPtr without increasing the reference count.

[Benjamin Otte]

Created attachment 43088 [details]
chat log with gtkmm developers

I went to the gtkmm developers asking them about their opinions on this, as they have a lot of experience with wrapping glib API in C++ (it's what they do for a living). The resulting chat is attached, I took the freedom to remove unrelated messages.

For tl;dr people, I guess the main arguments were:
- take a reference in the default constructor (done by the recent patch)
- make the constructor explicit
- don't have an operator=(T*)
- do use the get() idiom
- the WTF API is bad in places
- WTF is a cool namespace

[Eric Seidel (no email)]

Comment on attachment 43087 [details]
Updated patch which includes the adoptRef idiom

Style:
1 template <> GtkTargetList* refGPtr(GtkTargetList* ptr);
 32 template <> void derefGPtr(GtkTargetList* ptr);
 33 
 34 template <> GdkCursor* refGPtr(GdkCursor* ptr);
 35 template <> void derefGPtr(GdkCursor* ptr);

(no arg names)

I wonder if we should write these with a PointerTraits helper type instead:
 31 template <> GtkTargetList* refGPtr(GtkTargetList* ptr);
 32 template <> void derefGPtr(GtkTargetList* ptr);

a stuct which had a ref and deref method, which one single generic refGPtr could call instead.  Although I would be the first to tell you that I *am not* a C++ template expert.
Maybe I'm also just not understanding what those are.

 167     return GRefPtr<T>(p, true);
would be clearer using an Adopt enum instead of a boolean.
See examples like     enum PlacementNewAdoptType { PlacementNewAdopt }; in RefPtr.h

Inconsistent spacing:
33 template<typename T> class GRefPtr;
 34 template <typename T> GRefPtr<T> adoptGRef(T*);

I think we need to go one more round on this.  r-

[Martin Robinson]

Created attachment 44540 [details]
Updated patch with style fixes

I've attached an updated patch with the style fixes. It also moves GRefPtr to the JavaScriptCore/wtf/gtk directory with GOwnPtr. I started implementing the PointerTraits approach that Eric requested, but it seemed to increase the complexity of the patch a bit (as well as the in-memeory size). Perhaps a namespace would be a better approach?

[WebKit Review Bot]

Attachment 44540 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/PopupMenuGtk.cpp:89:  Use std::max() or std::max<type>() instead of the MAX() macro.  [runtime/max_min_macros] [4]
WebCore/platform/gtk/PopupMenuGtk.cpp:108:  Use 0 instead of NULL.  [readability/null] [5]
JavaScriptCore/wtf/gtk/GRefPtr.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3

[Martin Robinson]

Created attachment 44963 [details]
Updated patch with style fixes (rev. 2)

I've added a new patch changes based on the check-style bot output.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44963 [details] without any errors.

[Gustavo Noronha (kov)]

Comment on attachment 44963 [details]
Updated patch with style fixes (rev. 2)

OK, let's go ahead on this!

[Gustavo Noronha (kov)]

Landed as r52246.

[Gustavo Noronha (kov)]

Comment on attachment 44963 [details]
Updated patch with style fixes (rev. 2)

Clearing review flags.

[Martin Robinson]

Created attachment 45065 [details]
Patch with missing GHashTable specialization

The previous patch was missing the template specialization for GHashTable, which was causing crashes in plugin tests. I've attached a patch with the fix.

[Gustavo Noronha (kov)]

Comment on attachment 45065 [details]
Patch with missing GHashTable specialization

ok

[Gustavo Noronha (kov)]

Reopenning so that cq will land the patch.

[WebKit Commit Bot]

Comment on attachment 45065 [details]
Patch with missing GHashTable specialization

Clearing flags on attachment: 45065

Committed r52258: <http://trac.webkit.org/changeset/52258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.