[Bug 27833] TpChannelDispatchOperation high-level methods and CORE feature

[bugzilla-daemon at freedesktop.org]

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

--- Comment #8 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-06 10:14:49 PDT ---
> +maybe_set_connection (TpChannelDispatchOperation *self,
[...]
> +  g_object_notify ((GObject *) self, "connection");
> +
> +  if (g_hash_table_lookup (self->priv->immutable_properties,
> +        TP_PROP_CHANNEL_DISPATCH_OPERATION_CONNECTION) != NULL)
> +    return;

This doesn't look right. How about checking whether the path is the same one we
already had before notifying? Same for account and possible-handlers.

> +tp_channel_dispatch_operation_set_property (GObject *object,
...
> +      self->priv->connection = g_value_dup_object (value);
(etc.)

This isn't right - the properties are marked as read-only. Also, unless they're
construct-only, you can overwrite (and leak) an earlier non-NULL pointer.

> +get_dispatch_operation_prop_cb (TpProxy *proxy,

The CDO should be invalidated if its D-Bus properties aren't sane.

> +  if (self->priv->preparing_core)
> +    return;   /* already running */

You don't seem to ever set this to true?

> +   * TpChannelDispatchOperation::channel-lost: (skip)

Why do you (skip) this? It seems just as useful to Python/Javascript.

> + * tp_channel_dispatch_operation_handle_with_async:
...
> + * If successful, this method will cause the #TpProxy::invalidated signal
> + * to be emitted.

Specify which code it'll be? (TP_DBUS_ERROR_OBJECT_REMOVED, I think)

> +  g_assert (!tp_strdiff (tp_proxy_get_object_path (channel),
> +        tp_proxy_get_object_path (test->text_chan_2)));

Better as g_assert_cmpstr (X, !=, Y); for suitable X and Y.

> + * This function must call either tp_add_dispatch_operation_context_accept(),
> + * tp_add_dispatch_operation_context_delay() or
> + * tp_add_dispatch_operation_context_fail() on @context before it returns.

I think it's also worth putting in a reference to the HandleWith and Claim
wrappers.

> +      if (g_error_matches (error, TP_DBUS_ERRORS, TP_DBUS_ERROR_OBJECT_REMOVED))
> +        {
> +          /* There is no point calling the AddDispatchOperation
> +           * implementation; just terminate the D-Bus call right away */
> +          tp_add_dispatch_operation_context_accept (ctx);
> +        }

This isn't meant to happen - ChannelLost and Finished are meant to be delayed
until all approvers have returned from AddDispatchOperation (per the spec), and
we're an approver that hasn't returned. So, I don't think it's worth a special
case.

>     test-base-client: test invalidating one channel while calling AddDispatchOperation

This looks as if it contains a secret bugfix? :-)

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