[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