[Bug 55109] GVariant-based factory instantiation

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=55109

--- Comment #14 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> ---
(In reply to comment #7)
> Comment on attachment 94874 [details] [review]
> client-factory: take a vardict for new account props
> 
> Review of attachment 94874 [details] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/client-factory.c
> @@ +483,4 @@
> >  
> > +  if (immutable_properties != NULL)
> > +    {
> > +      g_variant_ref_sink (immutable_properties);
> 
> Please sink this *before* passing it to create_account, so that the
> create_account vfunc can rely on being called with a non-floating vardict.

done.

> (I'd also be tempted to create an empty a{sv} if it's NULL, so the vfunc
> doesn't need to cope with NULL.)

done. (In the 3 functions).

> ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c
> @@ +93,4 @@
> >  
> >    fixture->account = tp_client_factory_ensure_account (fixture->factory,
> >        tp_asv_get_string (params, "account-path"),
> > +      tp_asv_to_vardict (params),
> 
> Leaked. tp_asv_to_vardict() returns a new ref, not a floating ref.
> 
> (If you want to change its API, now is the time.)

Yeah I think it's best to return a floating ref: easier to use and more
coherent with GVariant's API.
I've done that in next already.


(In reply to comment #8)
> ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c
> @@ +830,2 @@
> >    g_test_add ("/log-iter-xml/get-events",
> > +      PidginTestCaseFixture, create_params (),
> 
> This is floating, and is consumed by setup(). That's weird.
> 
> Either sink it and explicitly unref it after g_test_run() like the old code
> did with the GHashTable, or have the user_data be a static or constant
> string containing the GVariant string serialization, and parse it in setup().

Fixed.

Branch updated:
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-factory-55109
(sorry the !fixup have been merged when I rebased on top of the new 'next'
branch).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.