[Spice-devel] [spice-gtk v1] gtk-session: Use GWeakRef

[Victor Toso]

Hi,

On Tue, Feb 21, 2017 at 05:16:26PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 20, 2017 at 03:49:02PM +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > The custom WeakRef structure was introduced in 9baba9fd89 (2012) to
> > fix rhbz#743773. Since glib 2.32, GWeakRef was introduced and it
> > behaves similarly to our WeakRef.
> > 
> > Moving to GWeakRef to remove some code which exists in glib.
> > 
> > Note that I'm keeping to utility functions:

Fixed typo: to -> two

> > - get_weak_ref(gpointer object): which returns a newly allocated
> >   GWeakRef, initialized with @object;
> > - free_weak_ref(gpointer pdata): which frees the GWeakRef and returns
> >   the original object or NULL. It also takes care of an extra
> >   reference to the object.
>
> Yes, this dropping of the strong ref is a bit odd at first, but in the
> end it's no different than how the code was working before, we expect
> the object to stay alive for the duration of the callback if it was
> alive when entering in the callback.
> 
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/spice-gtk-session.c | 52 +++++++++++++++++++------------------------------
> >  1 file changed, 20 insertions(+), 32 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 5688cba..88f4488 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -575,31 +575,26 @@ static const struct {
> >      }
> >  };
> >  
> > -typedef struct _WeakRef {
> > -    GObject *object;
> > -} WeakRef;
> > -
> > -static void weak_notify_cb(WeakRef *weakref, GObject *object)
> > -{
> > -    weakref->object = NULL;
> > -}
> > -
> > -static WeakRef* weak_ref(GObject *object)
> > +static GWeakRef* get_weak_ref(gpointer object)
> >  {
> > -    WeakRef *weakref = g_new(WeakRef, 1);
> > -
> > -    g_object_weak_ref(object, (GWeakNotify)weak_notify_cb, weakref);
> > -    weakref->object = object;
> > -
> > +    GWeakRef *weakref = g_new(GWeakRef, 1);
> > +    g_weak_ref_init(weakref, object);
> >      return weakref;
> >  }
> >  
> > -static void weak_unref(WeakRef* weakref)
> > +static gpointer free_weak_ref(gpointer data)
> >  {
> > -    if (weakref->object)
> > -        g_object_weak_unref(weakref->object, (GWeakNotify)weak_notify_cb, weakref);
> > +    GWeakRef *weakref = data;
> > +    gpointer object = g_weak_ref_get(weakref);
> >  
> > +    g_weak_ref_clear(weakref);
> >      g_free(weakref);
> > +    if (object != NULL) {
> > +        /* The main referece still exists as object is not NULL, so we can
>
> 'reference'

Fixed

>
> Looks good to me, though I'd probably return a ref to the callers, and
> add a g_object_unref() to the end of the callbacks.
>
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Yeah, it was my first option too but I didn't like to outcome as much as
having the unref in the helper function.

Thanks, I'll be pushing it soon

        toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170222/32b59e68/attachment.sig>