[Bug 27676] ContactInfo high-level API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #24 from Xavier Claessens <xclaesse at gmail.com> 2010-06-03 04:06:47 PDT ---
(In reply to comment #22)
> > +      G_OBJECT (self));
> 
> Passing the TpContact through the tp_cli call as the weak_object, when we know
> that it's reffed by the simple result and thus shouldn't be finalized during
> the call, at least deserves a comment.
> 
> When doing this in conjunction with a TpProxyPendingCall, I'd be inclined to
> say that it's risky to have a weak_object (cancelling manually after a
> weak_object might have died is a use-after-free); just put the TpContact in the
> context struct, which you have anyway.

done

> > +  tp_proxy_pending_call_cancel (data->call);
> 
> I think this should be protected by "if (data->call != NULL)"...
> 
> >  contact_info_request_cb (TpConnection *connection,
> 
> ... and this should set data->call to NULL on entry.

I made it assert that data->call != NULL, with the new code it should never
happen.

> > +          g_cancellable_disconnect (data->cancellable, data->cancellable_id);
> 
> The docs say: "Additionally, in the event that a signal handler is currently
> running, this call will block until the handler has finished. Calling this
> function from a "cancelled" signal handler will therefore result in a
> deadlock."
> tp_proxy_pending_call_cancel results in the pending call's data being freed,
> and its destructor calls g_cancellable_disconnect.
> 
> However, tp_proxy_pending_call_cancel frees the pending call in an idle because
> of Bug #14750, which saves us from this potential bug.
> 
> I think this is OK, but very very subtle: it deserves a comment in
> connection-contact-info.c saying that this code only works because of the fix
> for Bug #14750, and tp_proxy_pending_call_cancel() should gain a comment noting
> that this method relies on the idle.

I made the cancel in idle callback, it's safer IMO.

> + * Note that requesting the vCard from network can take significant time, so
> + * a bigger timeout is set on the underlying DBus call. That's also the reason
> + * we provide @cancellable in case the UI is not interested in the vCard
> + * anymore.
> 
> en_GB nitpicking:
> 
> "from the network"
> 
> "underlying D-Bus call"
> 
> and I'd rephrase the second sentence as:
> 
> @cancellable can be cancelled to free resources used in the D-Bus call if the
> caller is no longer interested in the vCard.

fixed

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