[webkit-reviews] review denied: [Bug 21599] [GTK] WebKit GTK needs a wrapper for ref counted glib/gobject structs : [Attachment 24347] Add GRefPtr and start the conversion to use it
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 15 19:05:18 PDT 2008
Alp Toker <alp at nuanti.com> has denied Marco Barisione
<marco.barisione at collabora.co.uk>'s request for review:
Bug 21599: [GTK] WebKit GTK needs a wrapper for ref counted glib/gobject
structs
https://bugs.webkit.org/show_bug.cgi?id=21599
Attachment 24347: Add GRefPtr and start the conversion to use it
https://bugs.webkit.org/attachment.cgi?id=24347&action=edit
------- Additional Comments from Alp Toker <alp at nuanti.com>
>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 @@
>+2008-10-14 Marco Barisione <marco.barisione at collabora.co.uk>
>+
>+ 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 <ap at webkit.org>
>
> 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 @@
>+2008-10-14 Marco Barisione <marco.barisione at collabora.co.uk>
>+
>+ 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 <hyatt at apple.com>
>
> 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*);
More information about the webkit-reviews
mailing list