[Bug 28367] Add StreamTube interface support

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Thu Jun 10 18:39:41 CEST 2010


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

--- Comment #7 from Olli Salli <ollisal at gmail.com> 2010-06-10 09:39:41 PDT ---
It's starting to be in a really good shape... But I'm still going to bother you
with some small finishing touches :P

One thing I previously didn't notice is you should have pretty include headers
(TelepathyQt4/ClassName) for all of your public classes. Currently there seems
to be one for just the "main class" of a header (eg.
IncomingStreamTubeChannel). You should also have pretty include headers for the
"auxiliary" classes like PendingStreamTubeConnection, obviously just including
the same header as the main include, in this case
incoming-stream-tube-channel.h.

IncomingStreamTubeChannel::acceptTubeAsTcpSocket(): You allow the case of
allowedAddress != Any but allowedPort == 0 - which doesn't make sense, and
MIGHT fail on the CM, but not necessarily, and would then only fail obscurely
when trying to actually connect to the CM socket. So either error out if an
address is provided but not a port, or better yet don't have default parameters
but a separate overload taking neither and specifying Any, 0 - leading to you
having to specify exactly neither or both.

With these, I'm good for a merge (phew). Andre will likely want to review this
stuff as well though - so stay tuned for his comments. In the meantime, I'll
move on to looking at your dbus stuff. Is there a chance the improvements we've
done here would be applicable there too btw? You know what to do if so ;)

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



More information about the telepathy-bugs mailing list