[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #22 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2011-07-14 03:27:44 PDT ---
Review of attachment 49071:
 --> (https://bugs.freedesktop.org/review?bug=38142&attachment=49071)

::: telepathy-glib/proxy.c
@@ +1340,3 @@
+   * TpProxy:factory:
+   *
+   * The #TpSimpleClientFactory that have been used to create this proxy,

"that has been used"

@@ +1401,3 @@
+ * Returns: (transfer none): the same value as #TpProxy:factory property
+ *
+ * Since: 0.UNRELEASED

We usually use this pattern for getters's doc:


 * Return the #TpTextChannel:is-sms-channel property
 *
 * Returns: the value of #TpTextChannel:is-sms-channel property

::: telepathy-glib/simple-client-factory-internal.h
@@ +30,3 @@
+
+TpAccount *_tp_account_new_with_factory (TpDBusDaemon *bus_daemon,
+    const gchar *object_path, GError **error, TpSimpleClientFactory *factory);

one arg per line.

::: telepathy-glib/simple-client-factory.c
@@ +33,3 @@
+ * construct its #TpConnection, it will ensure that all desired features are
+ * prepared.
+ *

This should describe the type of objects that will be created (mostly
interesting for TpChannel but still) and the features asked to be prepared.

@@ +99,3 @@
+{
+  TpDBusDaemon *dbus;
+  GHashTable *proxy_cache;

Please describe this hash table.

@@ +129,3 @@
+    gpointer proxy)
+{
+  if (proxy == NULL)

g_return_if_fail()? Why would it be NULL?

@@ +135,3 @@
+      (gpointer) tp_proxy_get_object_path (proxy), proxy);
+  tp_g_signal_connect_object (proxy, "invalidated",
+      G_CALLBACK (proxy_invalidated_cb), self, 0);

You rely on "invalidated" being fired when the proxy is destroyed. Is that
guaranteed? If it is at least add a comment (but I think wjt and/or smcv would
like to change that at some point). If it's not you should add a weak pointer.

@@ +220,3 @@
+
+  array = g_array_new (FALSE, FALSE, sizeof (TpContactFeature));
+  g_array_append_vals (array, self->priv->desired_contact_features->data,

You could use g_array_sized_new()

@@ +256,3 @@
+    {
+    case PROP_DBUS_DAEMON:
+      g_assert (self->priv->dbus == NULL);

we usually use:

      g_assert (self->priv->dbus == NULL);  /* construct only */

@@ +270,3 @@
+  TpSimpleClientFactory *self = (TpSimpleClientFactory *) object;
+
+  g_assert (self->priv->dbus != NULL);

g_assert (TP_IS_DBUS_DAEMON (..));

@@ +318,3 @@
+  self->priv->desired_contact_features = g_array_new (FALSE, FALSE,
+      sizeof (TpContactFeature));
+  contact_feature = TP_CONTACT_FEATURE_SUBSCRIPTION_STATES;

Any reason to only request this feature?

@@ +381,3 @@
+ * @self: a #TpSimpleClientFactory object
+ *
+ * <!-- -->

Same comment about getters' doc.

@@ +401,3 @@
+ *
+ * If a #TpAccountManager proxy has already been created using this method,
+ * it will be returned. Otherwise a new one will be created.

Phrasing is a bit weird. Make it more like tp_account_manager_dup's doc?

@@ +411,3 @@
+ *
+ * Returns: (transfer full): a new or existing #TpAccountManager proxy;
+ *  see tp_account_manager_new().

"a reference on a new or existing #TpAccountManager.." ?

@@ +426,3 @@
+    return g_object_ref (manager);
+
+  manager = TP_SIMPLE_CLIENT_FACTORY_GET_CLASS (self)->create_account_manager
(self);

this line is too long.

@@ +439,3 @@
+ *
+ * If a #TpAccount proxy has already been created using this method for the
+ * given @object_path, it will be returned. Otherwise a new one will be
created.

Same comment, I find the "has already been created using this method" a bit
weird; and false actually as one may have been created but has been destroyed
since.

@@ +740,3 @@
+ * responsability to keep a strong ref as long as needed.
+ *
+ * For this to work properly @connection must have immortal handles.

tp_connection_has_immortal_handles () has to return TRUE for @connection?

@@ +772,3 @@
+      connection, handle, identifier);
+
+  if (contact != NULL)

Why would it be NULL?

@@ +808,3 @@
+ * @n_features: The number of features in @features (may be 0)
+ * @features: (array length=n_features) (allow-none): an array of desired
+ *  features (may be %NULL if @n_features is 0)

Any reason why contact features are not GQuark? Should we open a API-break bug
about this?

@@ +821,3 @@
+void
+tp_simple_client_factory_add_contact_features (TpSimpleClientFactory *self,
+    guint n_features, const TpContactFeature *features)

one arg per line.

::: telepathy-glib/simple-client-factory.h
@@ +40,3 @@
+
+    /* TpAccountManager */
+    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory
*self);

no dup_features() for this one?

@@ +101,3 @@
+
+/* TpAccountManager */
+TpAccountManager *tp_simple_client_factory_dup_account_manager (

_dup_ sounds like a bad name to me. We usually use it for property getters
allocating memory or reffing an object. _ensure_ may be better here.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.