On 17 Jan 2014, at 14:10, Gladhorn Frederik <[email protected]> wrote:
> Hi Kurt, > > I'm quite impressed with the state of the websockets module. Thanks! > I tried reviewing the code, but I have to admit that I just started reading > the RFC a bit, so here are some first random notes. > > When websockets becomes part of a Qt module you can consider using > QObjectPrivate which makes the d pointer implementation slightly cleaner and > removes the need for duplicate d pointers (one from qobject one for the > class's own implementation. Okay, will fix that. > > In the public interface I'd generally declare signals with const refs, alone > for consistency. Done. Your patch has been reviewed and merged (thanks for that). > > While not really harmful there is generally no point in explicitly > initializing empty strings, I'd personally remove the QString() calls from > the class initialzers just to remove a few lines. > > qsslserver newEncryptedConnection is never emitted, I didn't look into what > it means though. Hmmm, I’ll have a look at that one. > > qwebsockethandshakeresponse: m_acceptedExtension doesn't seem used, instead > there is a string list passed through, I suppose it can simply be removed, it > also has a getter that will always return an empty string. Will check this also. > > > I was reading the RFC a bit, in section 9.1 there are some notes about the > extensions: > > const QList<QString> matchingExtensions = > > supportedExtensions.toSet().intersect(request.extensions().toSet()).toList(); > > does no longer guarantee the order of extentions - according to sec 9.1. this > is important. Right. Will fix this. > > The same seems to go for matchingProtocols - the first one is picked, since > QSet doesn't guarantee any order it is basically chosen by random. > I don't know how significant that is but I assume the client's first choice > (?) should be taken. Hmm, the list is sorted in descending order, and the server picks the latest protocol version that it supports. That seems fine to me, but I’ll double-check the RFC. Cheers, Kurt > > Greetings, > Frederik > > From: [email protected] > [[email protected]] on behalf of > Kurt Pattyn [[email protected]] > Sent: Thursday, January 16, 2014 12:51 PM > To: [email protected] > Cc: Heikkinen Jani; '[email protected]' ([email protected]); > [email protected]; [email protected] > Subject: Re: [Development] Qt 5.3 Feature freeze is coming quite soon... > > I would like to propose the QtWebSockets module as a new feature for Qt 5.3 > (see https://qt.gitorious.org/qtplayground/websockets/source/master) > > There are a number of requests in Jira: > https://bugreports.qt-project.org/issues/?jql=labels%20%3D%20websockets > asking to include web socket functionality in Qt, so maybe it is a good time > to include it now. > Besides that, EnginIo could make use of this module as well (currently it > uses an own implementation of web sockets). > > QtWebSockets is fully RFC6455 compliant (see > http://tools.ietf.org/html/rfc6455) and is successfully tested against the > latest Autobahn TestSuite. Performance is OK as well (test report is not > included because of too big). > > There is a C++ interface as well as a QML module. > The C++ API consists of the following classes: > QWebSocket > QWebSocketServer > QWebSocketProtocol > > The QML API consists of the following component: > WebSocket (included in the Qt.WebSockets 1.0 module). > > I see 2 options: either add the functionality to the QtNetwork module, or add > it as a Qt add-on. > Maybe adding it to QtNetwork would be a ‘natural’ place: QWebSocket would > then live besides QTcpSocket, QWebSocketServer would live besides QTcpServer. > > The module compiles on all reference platforms, has automated unit tests and > manual unit tests (against Autobahn TestSuite), is fully documented, and has > a number of examples. > The module is included in the CI system of Qt (see > http://testresults.qt-project.org/ci/WebSockets_master_Integration/). > > > Please raise your votes. > > Cheers, > > Kurt > > > > On 16 Jan 2014, at 09:28, Heikkinen Jani <[email protected]> wrote: > >> Hi all, >> >> I want to remind you all that Qt 5.3 feature freeze is coming pretty soon. >> Feature freeze for Qt 5.3 is 14th Feb 2014 so there is only 4 weeks left for >> implementing new features. >> >> Qt 5.3 schedule can be found here: http://qt-project.org/wiki/Qt-5.3-release >> . There is also link to the Qt 5.3 new features page >> (http://qt-project.org/wiki/New-Features-in-Qt-5.3). You can start >> collecting list of new features there… >> >> Note: Let’s keep the feature freeze date! If your feature isn’t ready at >> that point let’s then move it to Qt 5.4 release instead of taking in it in >> Qt 5.3 and fighting with it whole release time… >> This is also action point for each maintainer: Make sure your component is >> ready for feature freeze at that date. >> >> Just a reminder: >> In the feature freeze all new functionality must >> >> - Compile on all reference platforms (If a module/feature is only for one >> platform, make sure qmake/make does nothing on the other platforms) >> - Have tests. Automated tests should cover as much as possible of the new >> functionality. If certain areas are not covered by automated tests, there >> must be clarification how testing will be done for those >> - Have documentation. No undocumented public API. Basic docs have to be >> there, only polishing should still be required after the freeze >> - Have examples. Have some examples showing how to use the API. Examples >> need to be linked to from documentation. >> >> In addition, new modules need to >> >> - Follow the branching scheme. dev/stable/release should be there. A new >> module can be ok to only have dev, with stable being created at branching >> time. >> - Have a CI system. New modules that are going to be part of Qt releases >> need to have a CI system set up >> >> Br, >> Jani >> _______________________________________________ >> Development mailing list >> [email protected] >> http://lists.qt-project.org/mailman/listinfo/development
_______________________________________________ Development mailing list [email protected] http://lists.qt-project.org/mailman/listinfo/development
