[Bug 28366] Add DBusTube interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #27 from Olli Salli <ollisal at gmail.com> 2011-11-11 08:47:03 PST ---
(In reply to comment #22)
> Tests have been updated to be all >91%. I also took the chance of greening out
> TubeChannel, which is >90% as well now.
> ... now tested, and a bug in how the busNamesChanged signal was handled has
> been fixed.

Great! This level of line coverage is excellent and obviously sufficient for
all of our needs. And nice that expanding it also allowed you to squash a real
bug :)

You could however check that an appropriate error is returned for particular
corner cases. expectFailure() stores it in mLastError. The errors you're
currently returning from the early-outs seem to be spot on (except that the
NOT_IMPLEMENTED early-out we might want to replace with something more graceful
entirely).

> > Here's a few things to consider for now. I'll post a full review of the current
> > state of the APIs and implementation tomorrow.
> 
> Thank you :)

So here it goes.

+    gchar *service = g_strdup("org.not.seen.yet");

doesn't this leak? such an issue exists in

+void TestDBusTubeChan::testExtractBusNameMonitoring()

and the other bus name monitoring tests. In general, please verify using the
check-valgrind(-DBusTubeChannel) targets that there are no "definitely leaked"
leaks coming from the test code, so that we can use check-valgrind to check
that the tp-qt4 library proper doesn't leak.

An another general observation I'd like to make on the test: it seems rather
heavily copy-pasted from the stream tube tests. Thus, it has awkward naming for
some things, such as mGotRemoteConnection and mGotConnectionClosed, which makes
the logic hard to follow as you have to try and figure out which DBus tube
concepts they do happen to map to. Please revisit the naming to make it more
appropriate for what's actually being tested here.

Please also revisit the suggestions I made earlier for the documentation. And
well, in general, read the documentation you've written as a whole by yourself.
And convince yourself whether it makes sense in this day and age. I still see
things like:

 * DBusTubeChannel features can be enabled by constructing a ChannelFactory and
enabling the desired features,
 * and passing it to ChannelRequest or ClientRegistrar when creating them as
appropriate...

something which I already mentioned is quite in conflict with reality. And
perhaps anyway rather redundant with the general descriptions of how factories
and features interact. Note that this is just one example, the docs could use
cleanup otherwise.

PendingDBusTubeConnection:

    bool requiresCredentials() const;

Let's change this similarly to how asking for it when accepting/offering
changes.

    uchar credentialByte() const;

And remove this, because, as Simon mentioned, the cred byte must always be
'\0', and the transport sends it for you.

DBusTubeChannel:

    QHash<Tp::ContactPtr, QString> busNames() const;

Mm. Could we change this to be the other way around (contact for bus name)?
That's what the OutgoingStreamTubeChannel's two maps effectively are as well.
The rationale being that apps tend to want to do things like displaying the
face of whoever they're communicating with, instead of displaying bus names
(source addresses for stream tubes) of people who might have one in a tube.

I wonder why the D-Bus construct is a{us} in the first place. But well, that
doesn't need to constrain us in making our API intuitive and efficient. Of
course, because of this if you reverse the map, you need to O(n) remove the
mappings by value, but they're removed only once, and inspected an arbitrary
number of times.

IncomingDBusTubeChannel:

    if (mPriv->initRandom) {
            qsrand(QTime::currentTime().msec());
            mPriv->initRandom = false;
        }
        credentialByte = static_cast<uchar>(qrand());
        accessControlParam.setVariant(qVariantFromValue(credentialByte));

Again, the byte is always '\0' in DBus. So don't generate one. This is also
justified by the fact that PendingDBusTubeConnection doesn't take it
anywhere... copy-pasta from the corresponding code for stream tubes I guess.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.