[Bug 35084] tp-qt4: Design and implement API for easily being a StreamTube Handler

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-09-12 16:34:47 PDT ---
Initial review for current branch state. More to come later.

+    friend class StreamTubeServer; // interim until STS is implemented using
tube wrapping
     TELEPATHY_QT4_NO_EXPORT SharedPtr<RefCounted> _object() const; // TODO:
turn this into _object()
Is this still needed?

++++++ simple-stream-tube-handler

+        foreach (StreamTubeChannelPtr tube, mTubes.keys()) {
const StreamTubeChannelPtr &, not a ConstPtr

+        Q_ASSERT(!p2pServices.isEmpty() || !roomServices.isEmpty());
I don't think we should ASSERT here. Just fail ::create and warn if they are
both empty.
This is internal API, but it's called directly from STubeClient/Server
constructors with user provided params.

+        foreach (QString service, p2pServices)
const QString &

+        foreach (QString service, roomServices)
same here

+            const QString channelType =
+                chan->immutableProperties()[QLatin1String(
+                        TELEPATHY_INTERFACE_CHANNEL
".ChannelType")].toString();
This reminds me we should parse immutable properties on construction and
properly return them on accessors when available. So you could use
chan->channelType() directly here, even if the channel is not ready.

+void SimpleStreamTubeHandler::onReadyOpFinished(Tp::PendingOperation *op)
+{
+    Q_ASSERT(!mInvocations.isEmpty());
+    Q_ASSERT(!op || op->isFinished());
Well I guess this will break when called from here:
+        invocation->message = QLatin1String("Got no suitable channels");
+        onReadyOpFinished(0);
Also here you should properly check that context->setFinished is being called
when
onReadyOpFinished(0) is called.
You could simplify this by just calling ctx->setFinished/WithError directly
when no tube channel was provided in handleChannels()

+        foreach (StreamTubeChannelPtr tube, invocation->tubes) {
+            if (!tube->isValid()) {
const StremTubeChannelPtr &, not a ConstPtr
Please check other loops also for similar "issues".

Also it seems InvocationData leaks here.

Even SimpleStubeHandler being internal I would like to have header guards
there.


++++++ stream-tube-client

+    foreach (TubeWrapper *wrapper, mPriv->tubes.values()) {
+        wrapper->deleteLater();
+    }
This could be avoided by just passing this as parent when creating the wrappers

+        tube->requestClose();
+        return;
+    }
+
+    TubeWrapper *wrapper = 0;
+    
The last line contains extra EOL spaces

Please add accessors for "does it require credentials and is using unix
sockets" and 
rename const TcpSourceAddressGenerator *generator() const; to something more
sensible like
tcpGenerator();

Also I think we should default to accept as unix without credentials. 
+    Q_ASSERT(isRegistered());
The current code will crash if onInvokedForTube is called and setToAcceptAs*
hasn't being called yet or registerClient failed for some reason.

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