[Bug 34091] Allow TpSimplePasswordManager to be used with custom channels

[bugzilla-daemon at freedesktop.org]

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

Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://git.collabora.co.uk/
                   |                            |?p=user/jonathon/telepathy-
                   |                            |glib;a=shortlog;h=refs/head
                   |                            |s/sasl-may-save-response

--- Comment #5 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2011-02-24 01:55:21 PST ---
(In reply to comment #4)
> (In reply to comment #3)
> I felt that TpBaseSimplePasswordChannel was a bit long, and in some ways 'Base'
> implies that it's simple, so I decided to rename this class to
> TpBasePasswordChannel.  If you disagree with that, I can still rename it to
> TpBaseSimple...

Yeah that's fine.

> > + * @channel: an output location to retrieve the custom password channel that
> > + * was passed to tp_simple_password_manager_prompt_for_channel_async()
> > Should be annotate with (transfer none)
> > Or maybe it should ref it and be (transfer full), I *think* we usually return
> > a ref in such case.
> 
> I added an annotation for the current behavior (transfer none).  I think it's
> analogous to e.g.  g_socket_listener_accept_finish() where the 'source_object'
> that the user set earlier is passed back out but without a ref.  I think that's
> kind of nice since it won't accidentally leak, but the user can still easily
> take a ref if necessary.  But I'm willing to be convinced otherwise.

Yeah I think that's ok, the user should already have a ref on the channel
anyway.

> >  * Returns: a #GString with the password (or byte-blob) retrieved
> > + *  by @manager
> > 
> > + * Returns: a #GString with the password (or byte-blob) retrieved
> > + *  by @manager
> > Shouldn't we return a copy of this string? We should have a transfer
> > annotation anyway.
> 
> In this case, the API is already in released tp-glib, so changing it to return
> a copy of the string would cause leaks in existing code.  So I don't think we
> can change that.  And I made my prompt_for_channel_finish() function behave the
> same as prompt_finish().  I've added the (transfer none) annotations though.

You're right, I didn't notice that was already public API (as said, I'm new to
this code :).

> > Also, the lifetime of the GString stored in the GSimpleAsyncResult isn't very
> > clear to me. We get it from a signal, so it would be cleaner/safer to copy the
> > GString stored in the result and pass a GDestroyNotify to
> > g_simple_async_result_set_op_res_gpointer().
> > I'm all new to this code so please let me know if I missed something.
> 
> Yes, it would probably be a little bit more future-proof to copy the GString as
> you suggest.  I guess that the string is only guaranteed to live until the end
> of the signal.  This is not a problem in the current implementation since we
> call g_simple_async_result_complete() from within the signal handler, so by the
> time it returns, the string will still be alive.  By contrast, if we copied the
> string and used a GDestroyNotify, the string would actually be destroyed
> slightly earlier (e.g. when the GSimpleAsyncResult object is unreffed at the
> end of this function).  So it didn't seem all that important to me to copy the
> string.  The only situation where I can see this becoming important is if we
> change to using complete_in_idle() instead of complete(), and that seems
> unlikely to me.

Ok, as long as you know what you're doing that's fine with me.

I reviewed the credentials-storage branch yesterday, but as you pushed your
new commits on top of sasl-may-save-response I guess I should review the extra
2 commits as well.

e3c4afbd6535e458a217afb7921d83ad9052a7b7
Is that in a spec release? We usually update the whole spec to a new version
rather than just one file; so we can say in tp-glib NEW file that the spec has
been update to this specific version.

0e97beb778367111acafe68974ca1b88ec7e1050
looks good

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