| Summary: | GVariant-based factory instantiation | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Simon McVittie <smcv> |
| Component: | tp-glib | Assignee: | Telepathy bugs list <telepathy-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
| Severity: | enhancement | ||
| Priority: | medium | Keywords: | patch |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 30422 | ||
| Attachments: |
client-factory: take a vardict for new account props
logger tests: use a GVariant as params client-factory: take a vardict for new connection props file-transfer-chan: set "" as default URI channel-introspect: pass an empty array as Interfaces prop client-factory: take a vardict for new channel props |
||
|
Description
Simon McVittie
2012-09-19 15:25:58 UTC
Created attachment 94874 [details] [review] client-factory: take a vardict for new account props Created attachment 94875 [details] [review] logger tests: use a GVariant as params Created attachment 94876 [details] [review] client-factory: take a vardict for new connection props Created attachment 94877 [details] [review] file-transfer-chan: set "" as default URI Created attachment 94878 [details] [review] channel-introspect: pass an empty array as Interfaces prop Created attachment 94879 [details] [review] client-factory: take a vardict for new channel props 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. (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.) ::: 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.) ::: tests/logger/dbus/test-tpl-log-walker.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. Comment on attachment 94875 [details] [review] logger tests: use a GVariant as params Review of attachment 94875 [details] [review]: ----------------------------------------------------------------- I've only commented on the Pidgin test, but the log walker has the same issue. ::: 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(). Comment on attachment 94876 [details] [review] client-factory: take a vardict for new connection props Review of attachment 94876 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/client-factory.c @@ +623,4 @@ > > + if (immutable_properties != NULL) > + { > + g_variant_ref_sink (immutable_properties); As with the account, sink this before calling the vfunc, so the vfunc doesn't have to care. Comment on attachment 94877 [details] [review] file-transfer-chan: set "" as default URI Review of attachment 94877 [details] [review]: ----------------------------------------------------------------- > NULL isn't a valid D-Bus property, breaking tp_asv_to_vardict(). It's silently treated as "" in most of dbus-glib. The implementation of dbus_g_value_build_g_variant() was wrong in dbus-glib < 0.102, but Xavier fixed that in 0.102 (Bug #71811). Comment on attachment 94877 [details] [review] file-transfer-chan: set "" as default URI Review of attachment 94877 [details] [review]: ----------------------------------------------------------------- ::: tests/lib/file-transfer-chan.c @@ +694,4 @@ > param_spec = g_param_spec_string ("uri", > "URI", > "The URI property of this channel", > + "", This change is fine though. Comment on attachment 94878 [details] [review] channel-introspect: pass an empty array as Interfaces prop Review of attachment 94878 [details] [review]: ----------------------------------------------------------------- Again, this change is fine, but if you upgrade to dbus-glib 0.102 it's also unnecessary. Comment on attachment 94879 [details] [review] client-factory: take a vardict for new channel props Review of attachment 94879 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/base-client.c @@ +1542,4 @@ > _tp_connection_set_account (*connection, *account); > > *channel = tp_client_factory_ensure_channel (self->priv->factory, > + *connection, chan_path, tp_asv_to_vardict (chan_props), error); Non-floating, leaked ::: telepathy-glib/channel-dispatch-operation.c @@ +277,5 @@ > return; > > self->priv->channel = tp_client_factory_ensure_channel ( > + tp_proxy_get_factory (self), self->priv->connection, path, > + tp_asv_to_vardict (properties), &error); Non-floating, leaked ::: telepathy-glib/channel-request.c @@ +222,4 @@ > } > > channel = tp_client_factory_ensure_channel (tp_proxy_get_factory (self), > + connection, chan_path, tp_asv_to_vardict (chan_props), &error); Non-floating, leaked ::: telepathy-glib/client-factory.c @@ +776,4 @@ > > + if (immutable_properties != NULL) > + { > + g_variant_ref_sink (immutable_properties); Sink before calling the vfunc ::: tests/dbus/call-channel.c @@ +154,4 @@ > g_assert_no_error ((GError *) error); > > test->chan = tp_client_factory_ensure_channel (test->factory, > + connection, object_path, tp_asv_to_vardict (immutable_properties), leaked @@ +874,4 @@ > g_assert (test->chan == NULL); > > test->chan = tp_client_factory_ensure_channel (test->factory, > + conn, object_path, tp_asv_to_vardict (properties), &error); leaked ::: tests/dbus/dbus-tube.c @@ +139,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->tube = (TpDBusTubeChannel *) tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/file-transfer-channel.c @@ +216,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->channel = TP_FILE_TRANSFER_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/stream-tube.c @@ +208,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->tube = TP_STREAM_TUBE_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/text-channel.c @@ +83,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->channel = TP_TEXT_CHANNEL (tp_client_factory_ensure_channel (factory, > + test->connection, chan_path, tp_asv_to_vardict (props), &test->error)); leaked @@ +107,4 @@ > NULL); > > test->sms_channel = TP_TEXT_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/lib/util.c @@ +678,4 @@ > > factory = tp_proxy_get_factory (conn); > return tp_client_factory_ensure_channel (factory, conn, > + object_path, tp_asv_to_vardict (immutable_properties), error); leaked (In reply to comment #7) > Comment on attachment 94874 [details] [review] [review] > client-factory: take a vardict for new account props > > Review of attachment 94874 [details] [review] [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). (In reply to comment #14) > > 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. Only in the log walker test; Pidgin too, please. Otherwise looks good. > + immutable_properties = g_variant_new ("a{sv}", NULL); I didn't know that worked. Nice trick. (In reply to comment #15) > (In reply to comment #14) > > > 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. > > Only in the log walker test; Pidgin too, please. Oh yeah sorry. Done and merged. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.