Bug 46513

(In reply to comment #0)
> I'm sorry it's so big, and I'm also sorry that it doesn't end with a commit
> which actually moves the code to Wocky, but there we go.

Haha, I was like "okay I'll take a look"

...

http://oi41.tinypic.com/qxjapx.jpg

> @@ -2374,14 +2361,14 @@ session_terminated_cb (GabbleJingleSession *session,
>   TpGroupMixin *mixin = TP_GROUP_MIXIN (channel);
>   guint terminator;
>   JingleState state;
> - TpHandle peer;
> + TpHandle peer = gabble_jingle_session_get_peer_handle (priv->session);
>   TpIntSet *set;
>
>   DEBUG ("called");
>
> + peer = gabble_jingle_session_get_peer_handle (priv->session);

Duplicate call to gabble_jingle_session_get_peer_handle() (in "MediaChannel: use gabble_jingle_session_get_peer_handle()", d884caf42e3)

Everything else up to and including that commit looks good. There might be a million patches but at least they're easy to review :-)

"Send session-{initiate,accept} IQs using porter directly.", 72ea73343252: I think you're leaking sess in the callbacks, except in the early-return cases where the state has become inappropriate?

Everything else up to and including that point looks good, except as already reviewed.

> +static void
> +gabble_jingle_info_dispose (GObject *object)
> +{
> + GabbleJingleInfo *self = GABBLE_JINGLE_INFO (object);
> + GabbleJingleInfoPrivate *priv = self->priv;
> + GObjectClass *parent_class = gabble_jingle_info_parent_class;
> +
> + if (parent_class->dispose != NULL)
> + parent_class->dispose (object);

Surely dispose should chain up at the end, not the beginning? I see dispose and finalize as progressively breaking the functionality of the object's classes, most-derived first.

jingle_info_reply_cb looked crashy, but you fixed that in the next commit.

Everything else up to 39b90872eb "JingleContent: don't store a GabbleConnection" looks good except as noted already.

> + /* Only emitted for new incoming sessions, mainly for legacy reasons */
> + signals[NEW_SESSION] = g_signal_new ("new-session",

Can't the JingleMint's new signal be "incoming-session" or something?

In da9f352766e "Represent Jingle relays by a struct, not an aa{sv}":

> + GPtrArray *tp_relays = gabble_build_tp_relay_info (relays);
> + tp_base_media_call_stream_set_relay_info (stream, tp_relays);

Nitpicking: I'd prefer a blank line between initialized locals and other code.

All reviewed now, looks good apart from the things already noted.

(In reply to comment #2)
> > @@ -2374,14 +2361,14 @@ session_terminated_cb (GabbleJingleSession *session,
> >   TpGroupMixin *mixin = TP_GROUP_MIXIN (channel);
> >   guint terminator;
> >   JingleState state;
> > - TpHandle peer;
> > + TpHandle peer = gabble_jingle_session_get_peer_handle (priv->session);
> >   TpIntSet *set;
> >
> >   DEBUG ("called");
> >
> > + peer = gabble_jingle_session_get_peer_handle (priv->session);
> 
> Duplicate call to gabble_jingle_session_get_peer_handle() (in "MediaChannel:
> use gabble_jingle_session_get_peer_handle()", d884caf42e3)

This local gets removed entirely in a later patch, because the MediaChannel tracks its own peer.

> Everything else up to and including that commit looks good. There might be a
> million patches but at least they're easy to review :-)

I try!

(In reply to comment #3)
> "Send session-{initiate,accept} IQs using porter directly.", 72ea73343252: I
> think you're leaking sess in the callbacks, except in the early-return cases
> where the state has become inappropriate?

Aye. Patch fixing this pushed.

(In reply to comment #4)
> > +static void
> > +gabble_jingle_info_dispose (GObject *object)
> > +{
> > + GabbleJingleInfo *self = GABBLE_JINGLE_INFO (object);
> > + GabbleJingleInfoPrivate *priv = self->priv;
> > + GObjectClass *parent_class = gabble_jingle_info_parent_class;
> > +
> > + if (parent_class->dispose != NULL)
> > + parent_class->dispose (object);
> 
> Surely dispose should chain up at the end, not the beginning? I see dispose and
> finalize as progressively breaking the functionality of the object's classes,
> most-derived first.

Yup. I even called someone out on this the other day. Fixed.

(In reply to comment #5)
> > + /* Only emitted for new incoming sessions, mainly for legacy reasons */
> > + signals[NEW_SESSION] = g_signal_new ("new-session",
> 
> Can't the JingleMint's new signal be "incoming-session" or something?

Good idea. Renamed.

> In da9f352766e "Represent Jingle relays by a struct, not an aa{sv}":
> 
> > + GPtrArray *tp_relays = gabble_build_tp_relay_info (relays);
> > + tp_base_media_call_stream_set_relay_info (stream, tp_relays);
> 
> Nitpicking: I'd prefer a blank line between initialized locals and other code.

Fixed.

This looks good now.

It is merged! http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=0a7c66f48e2a4f5f1b80237def4063fa952ccc71

A disappointing diffstat in the end:

 53 files changed, 2177 insertions(+), 1205 deletions(-)

Oh well.