[Telepathy] new jingle engine for Gabble - more review

[Simon McVittie]

On Tue, 04 Nov 2008 at 18:33:45 +0100, Senko Rasic wrote:
> Simon McVittie wrote:
> > g_slice_new0
> 
> Changed in 79275e9c7b7da81465210292d3129e447626f260 .
> Since I'm creating the structs in media-stream and freeing them in
> jingle-transport-google and jingle-media-rtp, do you think I should
> have a helper function for creating them, and not having to rememer
> about g_slice in various places?

That might be useful, yes. I usually end up writing a foo_new() and a
foo_free() whenever a complicated struct foo exists.

> >> +#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.
> 
> I'm using these as syntactic sugar because there are a lot of places
> where I have to return errors, so I thought the fewer characters the 
> better (if i have to pass in XMPP_ERROR_{BAD_REQUEST,OUT_OF_ORDER,...}
> each time, there's not much typing saved :)

What I meant was (best viewed in monospace):

added this -----------\
                      |
                      v
#define SET_BAD_REQ(error, txt...) \
g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_BAD_REQUEST, txt)

> >> +      /* FIXME: report error to peer instead of assert */
> >> +      g_assert (g_list_length (cs) == 1);
> > 
> > Please don't assert on network traffic.
> 
> It's not assertion on network traffic, it's a sanity check. Other parts
> of jingle code should ensure that session in google mode never != 1
> content (either by adding, removing, contents, or by putting
> multiple descriptions in session initiate action). If that happens,
> we're in serious problems anyways.

OK, if it's been checked already then an assert is fine.

> I've pushed these patches to my jingle repo. Content direction code,
> timeouts in content-add and <candidates> gtalk3 detection are still in
> the TODO list, I'll attack them now.

Great. Can we have a wiki page somewhere listing known problems with
this branch?

    Simon