[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #70 from Will Thompson <will.thompson at collabora.co.uk> 2011-08-04 08:27:26 PDT ---
Here's a review of the first and last outstanding patches.

TpAccountManager: prepare its TpAccounts before exposing them
=============================================================

-tp_account_manager_ensure_account (TpAccountManager *manager,
+tp_account_manager_ensure_account (TpAccountManager *self,

It's *MUCH* more tedious to review code if you will insist on randomly renaming
variables in an already-enormous patch which makes substantial changes to the
inner workings of functions. Please don't do this. I definitely prefer 'self'
being used consistently, but not in the same patch as fundamental changes.

This actively discourages me from reviewing this code, and I don't think I'm
alone in my reaction.

Even disregarding this, the commitdiff is completely illegible. It must have
been possible to make this change in smaller chunks, rather than in a single
400-line patch.

 tp_account_manager_get_valid_accounts (TpAccountManager *manager)
 {
-  TpAccountManagerPrivate *priv;
-  GList *ret;
-
   g_return_val_if_fail (TP_IS_ACCOUNT_MANAGER (manager), NULL);

-  priv = manager->priv;
-
-  ret = g_hash_table_get_values (priv->accounts);
-
-  return ret;
+  return g_hash_table_get_values (manager->priv->accounts);
 }

This is also *completely* unrelated to the rest of the patch, and has *no
functional effect*!


Anyway, on with the review
--------------------------

+ * All #TpAccount<!-- -->s are guaranteed to have all desired features
prepared
+ * (at least %TP_ACCOUNT_FEATURE_CORE).
+ * See tp_simple_client_factory_add_account_features() to define desired
+ * features.

“The returned #TpAccount<!-- -->s are guaranteed to have
%TP_ACCOUNT_FEATURE_CORE prepared, along with all features previously passed to
tp_simple_client_factory_add_account_features().”?

Ditto the other places where you use this wording.


Legacy accounts
---------------

This should have been a separate patch. It's probably a prerequisite for the
other changes, I agree, but it could have been made independently, first.

+tp_account_manager_ensure_account (TpAccountManager *self,
     const gchar *path)
 {
-  TpAccountManagerPrivate *priv;

Why? A bunch of functions seem to randomly lose their 'priv' locals even though
they use the private structure.


+  if (self->priv->legacy_accounts == NULL)
+    self->priv->legacy_accounts = g_hash_table_new_full (
+        g_str_hash, g_str_equal, g_free, g_object_unref);

Why fuss about lazily initializing a hash table?

+static void
+legacy_account_invalidated_cb (TpProxy *account,
+    guint domain,
+    gint code,
+    gchar *message,
+    gpointer user_data)
+{
+  TpAccountManager *self = user_data;
+
+  g_hash_table_remove (self->priv->legacy_accounts,
+      tp_proxy_get_object_path (account));
 }

What happens if I do this:

  TpAccountManager *am = tp_account_manager_new_with_factory (...);
  tp_account_manager_ensure_account (am, "some valid account path");
  g_object_unref (am);

I *bet* that when the TpAM eventually gets disposed, it'll crash, because this
code will run:

+  tp_clear_pointer (&priv->legacy_accounts, g_hash_table_unref);

tp_clear_pointer first sets priv->legacy_accounts to NULL, and then unrefs the
hash table, which will unref the account, which will make it emit ::invalidated
(bug 22119), which will call legacy_account_invalidated_cb, which will crash
because self->priv->legacy_account == NULL.

(And no, using tp_g_signal_connect_object() does not save you because weak refs
are notified in GObject's dispose, which runs after
_tp_account_manager_dispose() which is where the hash table is destroyed.)

You might get saved by tp_proxy_prepare_async() keeping the legacy account
alive; so change my above example to unref am only after that's finished.

The corresponding callback for normal accounts does not have this problem
because:

  /* We only want to deal with accounts being removed here. */
  if (domain != TP_DBUS_ERRORS || code != TP_DBUS_ERROR_OBJECT_REMOVED)
    return;

TpSimpleClientFactory: add code example of typical app's main()...
==================================================================

+ * A typical application using its own factory would look like this:
+ * |[
+ * int main()
+ * {
+ *   TpSimpleClientFactory *factory;
+ *   TpAccountManager *manager;
+ *
+ *   factory = my_factory_new ();

A typical application will not use its own factory; it will use
#TpAutomaticClientFactory. If an application does use its own factory class, it
will presumably be a subclass of #TpAutomaticClientFactory.

This example won't compile: main needs arguments.

It also will crash because it doesn't call g_type_init().

-- 
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.