[Telepathy] review: Gabble hash caps

[Dafydd Harries]

Ar 28/05/2008 am 12:22, ysgrifennodd Alban Crequy:
> Branch:
> http://monkey.collabora.co.uk/telepathy-gabble-alban-xep0115/
> 
> XEP-0115 v1.3 (Entity Capabilities) was already implemented in Gabble.
> This branch implements XEP-0115 v1.5 (the new version) and remains
> compatible with Jabber client using the old version. The new version
> hopefully uses less bandwidth for the caps and helps prevent poisoning
> of caps.
> 
> Thanks for the review. Comments below:
> 
> 
> Le Tue, 27 May 2008 20:18:05 +0100,
> Dafydd Harries <dafydd.harries at collabora.co.uk> a écrit :
> 
> > +  waiter_self = NULL;
> > +  for (i = waiters; NULL != i;  i = i->next)
> > 
> > Perhaps we should be using a hash table here.
> 
> Maybe.
> (I did not change the code)

That's fine.

> > -        self.remote_caps = { 'ext': 'voice-v1 jingle-audio
> > jingle-video', 'ver': '0.6.0',
> > -                 'node': 'http://telepathy.freedesktop.org/caps' }
> > +        self.remote_caps = { 'ext': '', 'ver': '0.0.0',
> > +                 'node': 'http://example.com/fake-client0' }
> > 
> > Hmm, was this causing some problem?
> 
> In previous versions of Gabble, Gabble has a bundle cache pre-filled
> with "jingle-audio", etc. Now with this branch, Gabble does not has this
> cache pre-filled, so it has to ask the caps of the remote contact
> (the remote contact is simulated by the twisted test). It involves
> some additional round trips to ask the caps of each bundle. But this
> test is not about caps but about jingle, so in order to keep Jingle
> tests simple, I don't use bundles and advertise all features directly.
> 
> The alternative would be to add some code in Jingle tests to expect
> Gabble requesting each cap bundles, and reply to theses requests.
> 
> Unrelated note: the remote contact in this test suite use XEP-0115 v1.3.

Makes sense; than's for explaining.

> > +    _test_without_hash(q, bus, conn, stream, 'bob1 at foo.com/Foo', 2L,
> > client, 1)
> > 
> > s/1/True/.
> 
> Fixed
> 
> > +caps_changed_flag = 0
> > 
> > s/0/False/.
> 
> Fixed

I see you replaced all the other instances too; great. :)

> > +def dbus_sync(bus, q, conn):
> > +    # Dummy D-Bus method call
> > +    call_async(q, conn, "InspectHandles", 1, [])
> > +
> > +    event = q.expect('dbus-return', method='InspectHandles')
> > 
> > I think I've written this before, but using org.freedesktop.Ping.
> 
> I don't see any test using org.freedesktop.Ping. Does Gabble really
> implement the interface org.freedesktop.Ping? I don't see it in d-feet.
> 
> I use dbus_sync() to be sure Gabble finished to send its D-Bus signals
> and that the twisted suite received all of them. So I need
> Ping/InspectHandles/whatever to be implemented by Gabble (I cannot
> just ping the dbus-daemon).

Sorry, I meant org.freedesktop.DBus.Ping.

Perhaps we should name it sync_dbus() to be consistent with sync_stream().

> > Perhaps worth moving into servicetest.py.
> 
> Yes, done. It still uses 'InspectHandles' though.o

+gchar *
+caps_hash_compute_from_lm_node (LmMessageNode *node)

This function is pretty long; perhaps it would be nicer if the g_str_equal
(child->name, "x") branch were factored out.

Otherwise, +1.

Your code is very nicely written, with especially good use of comments and
thorough and clear tests. I like the way you decomposed similar parts of your
tests into functions.

-- 
Dafydd