[Telepathy] new jingle engine for Gabble - more review

[Simon McVittie]

In general (although under the circumstances these aren't merge blockers):

* delete dead code rather than commenting it out
* don't use // comments
* I'd prefer this style for priv stuff:

  /* in header */
  typedef struct _Foo Foo;
  typedef struct _FooPrivate FooPrivate;

  struct _Foo {
      ...
      FooPrivate *priv;
  };

  /* in body */
  struct _FooPrivate {
      Bar *bar;
  };

  void
  foo_do_things (Foo *self)
  {
    bar_do_things_with_foo (self->priv->bar, self);
  }

I think it's worth trying this code out in some "real world" situations to
make sure we don't have any huge regressions. I'll get some builds going and
see what happens; we have Google Talk in the office that we can test interop
with, too.

Senko wrote some code:
> src/Makefile.am

alphabetical order of files please

> connection.[ch]

alphabetical order of private headers please, as per Style wiki page.
Likewise in media-factory.c

> "%s" stuff

Have you fast-tracked it in already? If not, do - I'll review

> +#define NS_JINGLE_RTP_TMP       "urn:xmpp:jingle:apps:rtp:0"

Is this really TMPorary? It looks somewhat final to me... anyway, either
tag the name with a version, or indicate that it's final.

It would be nice if each namespace had a one-line comment, like
/* XEP-0123 v0.32 (last call) */, indicating what it means.

> PRESENCE_CAP_JINGLE_TMP

Do you mean PRESENCE_CAP_JINGLE_032? Please be consistent.

> +# print "FIXME: jingle/test-incoming-call.py disabled due to race
> condition"

Don't leave in dead code, please. If this test has stopped being racy,
let's enable it. Likewise for all the stuff you commented out in
media-stream.h.

> @@ -302,10 +322,12 @@ gabble_media_stream_set_property (GObject *object,
>          {
>            StreamSignallingState old = stream->signalling_state;
>            stream->signalling_state = g_value_get_uint (value);
> -          GMS_DEBUG_INFO (priv->session, "stream %s sig_state %d->%d",
> +          DEBUG ("stream %s sig_state %d->%d",
>                stream->name, old, stream->signalling_state);
> +          /* FIXME 
>            if (stream->signalling_state != old)
>              push_native_candidates (stream);
> +            */

What's going on here? What does the FIXME mean?

> +  param_spec = g_param_spec_uint ("mode", "Signalling mode",

Why is GabbleMediaStream:mode no longer an enum? Isn't this a bit of a
regression?

> +                                  0, G_MAXUINT, MODE_JINGLE, // FIXME

What needs fixing here?

> +      DEBUG ("ignoring native localhost candidate");
> +      /*
>        GMS_DEBUG_WARNING (priv->session,
> -          "%s: ignoring native localhost candidate", G_STRFUNC);
> +          "%s: ignoring native localhost candidate", G_STRFUNC); */

Please eliminate dead code.

> @@ -846,18 +931,28 @@ gabble_media_stream_new_native_candidate (TpSvcMediaStreamHandler *iface,
> +  c = g_new0 (JingleCandidate, 1);

g_slice_new0 would be better for a 7-word struct.

> +  gabble_jingle_content_add_candidates (priv->content, li);
> +
> +  // FIXME: the above instead this: push_native_candidates (self);

Please explain?

> +      c = g_new0 (JingleCodec, 1);

g_slice_new0

> @@ -913,22 +1014,57 @@ gabble_media_stream_set_local_codecs (TpSvcMediaStreamHandler *iface,
> +          5, &params,

The params seem to be discarded rather than passed to the jingle engine. Is
this expected? If so, perhaps don't bother even pulling them out of the struct,
and put a comment indicating that they're ignored.

> @@ -941,10 +1077,14 @@ gabble_media_stream_stream_state (TpSvcMediaStreamHandler *iface,
> +  /* FIXME: we're relying on 1:1 mapping between tp and jingle enums */
> +  gabble_jingle_content_set_transport_state (priv->content,
> +      connection_state);

Is there error checking? D-Bus clients can pass in whatever they like here.

> @@ -1220,93 +1153,38 @@ _gabble_media_stream_post_remote_codecs (GabbleMediaStream *stream,
> +          4, c->channels,
> +            /* FIXME: valgrind shows leak in g_hash_table_new_full - doesn't
> +             * dbus-glib free that? */
> +          5, g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free),

I believe dbus_g_type_struct_set copies its arguments, so you need to assign
this hash table to a temporary, and free it afterwards.

> +content_state_changed_cb (GabbleJingleContent *c,
>...
>+      /* FIXME: check direction for these */
>+      stream->playing = TRUE;
>+      priv->sending = TRUE;

It looks as though this needs fixing?

>+/* FIXME: this doesn't work yet */
> void
> _gabble_media_stream_update_sending (GabbleMediaStream *stream,
>                                      gboolean start_sending)

What doesn't work? Why not? Can it be fixed?

> GabbleJingleContent

What's going on with PROP_READY? If it's not needed, kill it.

> +#define SET_BAD_REQ(txt...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_BAD_REQUEST, txt)

I'd prefer this to take error as a first parameter so it isn't reliant
on naming.

> +send_content_add_or_accept (GabbleJingleContent *self)
>...
> +      /* TODO: set a timer for acknowledgement */

Does this still need doing?

> +++ b/src/jingle-content.h
> +struct _JingleCandidate {

Does this duplicate one of the ones in the media stream?

> +gabble_jingle_content_add_candidates (GabbleJingleContent *self, GList *li)

Please comment in either the header or the source what the GList contains.

> +static JingleAction allowed_actions[6][11] = {

Eek, magic numbers! Can we have some sort of comment please, at least for what
6 means?

> +on_transport_info (GabbleJingleSession *sess, LmMessageNode *node,
>...
> +      /* FIXME: report error to peer instead of assert */
> +      g_assert (g_list_length (cs) == 1);

Please don't assert on network traffic.

Has libjingle 0.3 vs 0.4 detection been implemented? Do we need it?

> +  /* FIXME: we rely on the fact that JingleContent only signals
> +   * "ready" on session contents; this is wrong, session contents
> +   * added/accepted *after* session-accept are handled the same
> +   * as early media */

Has this been fixed? Should it?

> +  // FIXME: klass->produce = produce_candidates;

Has this been implemented for the Google transport? Is it needed?

It appears to be assumed that all Google dialects are numerically less than
all Jingle dialects. Either the enum of dialects should have a comment that
says so, or the code that uses <= should be changed to use a function
jingle_dialect_is_google().

Regards,
    Simon