[webkit-reviews] review denied: [Bug 34180] [Qt] Enable websockets support in QtWebKit : [Attachment 47476] Patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 27 01:04:10 PST 2010
Simon Hausmann <hausmann at webkit.org> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 34180: [Qt] Enable websockets support in QtWebKit
https://bugs.webkit.org/show_bug.cgi?id=34180
Attachment 47476: Patch v2
https://bugs.webkit.org/attachment.cgi?id=47476&action=review
------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
Great that you're starting with this, Yael!
> +#include <QSslSocket>
> +#include <QTcpSocket>
> +
> +class QSslSocket;
Why include QSslSocket and also forward declare it? :)
> +class SocketStreamHandlePrivate : public QObject {
> +Q_OBJECT
The Q_OBJECT macro us usually indented.
> +public:
> + SocketStreamHandlePrivate(SocketStreamHandle* streamHandle, const KURL&
url);
WebKit style suggests to remove the names of the arguments here, as they're
redundant (The type is the same as the variable name).
> + connect(m_socket, SIGNAL(connected()), this, SLOT(socketConnected()));
> + connect(m_socket, SIGNAL(readyRead()), this, SLOT(socketReadyRead()));
> + connect(m_socket, SIGNAL(disconnected()), this, SLOT(socketClosed()));
> + connect(m_socket, SIGNAL(error(QAbstractSocket::SocketError)), this,
SLOT(socketError(QAbstractSocket::SocketError)));
> + if (isSecure)
> + connect(m_socket, SIGNAL(sslErrors(const QList<QSslError>&)), this,
SLOT(socketSslErrors(const QList<QSslError>&)));
> + unsigned int port = url.hasPort() ? url.port() : isSecure ? 443 : 80;
This looks rather convoluted :). Do you mind splitting this up a little bit or
adding parentheses?
> + QString host = url.host();
> + if (isSecure)
> + qobject_cast<QSslSocket*>(m_socket)->connectToHostEncrypted(host,
port);
I suggest to do a static_cast here, it's faster and if something is wrong it'll
crash either way ;-)
> +void SocketStreamHandlePrivate::socketReadyRead()
> +{
> + if (m_streamHandle && m_streamHandle->client()) {
> + QByteArray data = m_socket->readAll();
> + m_streamHandle->client()->didReceiveData(m_streamHandle,
data.constData(), data.size());
> + }
> +}
Is it intentional to read _all_ data here and block until all is available?
I wonder if it should be done like in QNetworkReplyHandler instead:
QByteArray data = m_socket->read(m_socket->bytesAvailable());
Performance wise it's sad that we have to copy the data here, but I don't see
any obvious solution here (as well as in QNetworkReplyHandler).
> +int SocketStreamHandlePrivate::send(const char* data, int len)
> +{
> + m_lastSentSize = m_socket->write(data, len);
Looks like one whitespace too much here :)
> + QMetaObject::invokeMethod(this, "socketSentData", Qt::QueuedConnection);
What's the reason for this queued connection? Is the goal to call
sendPendingData() only once the previous data has been written? (That itself
may not be guaranteed with a queued connection that just goes through the event
loop once)
> +void SocketStreamHandlePrivate::socketClosed()
> +{
> + QMetaObject::invokeMethod(this, "socketClosedCallback",
Qt::QueuedConnection);
> +}
> +
> +void SocketStreamHandlePrivate::socketError(QAbstractSocket::SocketError
error)
> +{
> + QMetaObject::invokeMethod(this, "socketError", Qt::QueuedConnection,
Q_ARG(int, error));
> +}
Out of curiousity again, why the queued connections?
> +void SocketStreamHandlePrivate::socketErrorCallback(int error)
> +{
> + // FIXME - in the future, we might not want to treat all errors as
fatal.
> + if (m_streamHandle && m_streamHandle->client()) {
> + m_streamHandle->client()->didClose(m_streamHandle);
Since this is called in case of socket errors, shouldn't we call didFail() on
the stream handle client instead of just didClose()?
> +void SocketStreamHandlePrivate::socketSslErrors(const QList<QSslError>&)
> +{
> + qobject_cast<QSslSocket*>(m_socket)->ignoreSslErrors();
> +}
Hmmm, doesn't that effectively remove any security from encrypted connections?
If someone spoofs a certificate we ignore it and still happily connect,
allowing a man-in-the-middle to read all the traffic.
> Index: LayoutTests/platform/qt/Skipped
> ===================================================================
> --- LayoutTests/platform/qt/Skipped (revision 53851)
> +++ LayoutTests/platform/qt/Skipped (working copy)
> @@ -4965,7 +4965,6 @@
> http/tests/xmlhttprequest/workers/shared-worker-methods-async.html
>
> # Missing SocketStreamHandle implementation
> -websocket/tests
I like this part! Yay for layout testing :)
But you should also remove the comment then that says the implementation is
missing ;-)
More information about the webkit-reviews
mailing list