Bug 47059

Created attachment 69593 [details] Patch

Created attachment 69680 [details] Patch

Created attachment 69826 [details] Patch

Comment on attachment 69826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review Quick r- for somewhat superficial reasons. Peter Hartmann (p--hartmann on IRC) has been working on multipart support for QNAM, I've asked him to have a look at this. > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432 > + int s = contentType.lastIndexOf("boundary=") + 9; "s" is not a good variable name. > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446 > + int bs = data->indexOf(boundary); "bs" is not a good variable name. > WebCore/platform/network/qt/QNetworkReplyHandler.h:72 > + void removeMultipartHeaders(QByteArray* data); > + void processMultipartResponse(QByteArray* data, ResourceHandleClient* client); None of the argument names (data, data and client) are needed here. > WebCore/platform/network/qt/QNetworkReplyHandler.h:73 > + bool isMultipartResponse(); This method should be const.

I think this change can still be useful while waiting for Peter's QNAM work to land in Qt. When it does, we will still need to special-case multipart responses in QNetworkReplyHandler because of the WebCore quirks noted in the comments. Certainly, bug 47060 will definitely need support in QNAM.

View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review The multipart code that we plan for Qt 4.8 is for sending multipart messages only, see http://bugreports.qt.nokia.com/browse/QTBUG-6222 . > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:424 > + if (contentType.contains("multipart/x-mixed-replace")) shouldn't we check for contentType.startsWith("multipart/") ? >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432 >> + int s = contentType.lastIndexOf("boundary=") + 9; > > "s" is not a good variable name. the boundary could (and in fact, should) be quoted, recommended in RFC 2046 section 5.1.1; I think the patch should handle that use case as well >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446 >> + int bs = data->indexOf(boundary); > > "bs" is not a good variable name. maybe combine the two above lines to while ((boundaryIndex = data->indexOf(boundary)) != -1) { the indexOf() method could be costly for big parts > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:450 > + data->replace(part, ""); how about keeping data constant and only use the char pointer and indexing? data might be a really big byte array, so detaching should be avoided. Or how about wrapping it into a QBuffer? Or could it somehow be avoided to construct the QByteArray in the first place, and operate on some char pointer instead?

(In reply to comment #6) > View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review > > The multipart code that we plan for Qt 4.8 is for sending multipart messages only, see http://bugreports.qt.nokia.com/browse/QTBUG-6222 . > > > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:424 > > + if (contentType.contains("multipart/x-mixed-replace")) > > shouldn't we check for contentType.startsWith("multipart/") ? > Not sure - I think we're only supporting the x-mixed-replace MIME-type here. This is what WebCore itself looks for. > >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432 > >> + int s = contentType.lastIndexOf("boundary=") + 9; > > > > "s" is not a good variable name. > > the boundary could (and in fact, should) be quoted, recommended in RFC 2046 section 5.1.1; I think the patch should handle that use case as well > Yes - thanks for the reference. There's quite a few cases to handle there! > >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446 > >> + int bs = data->indexOf(boundary); > > > > "bs" is not a good variable name. > > maybe combine the two above lines to > > while ((boundaryIndex = data->indexOf(boundary)) != -1) { > > the indexOf() method could be costly for big parts > > > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:450 > > + data->replace(part, ""); > > how about keeping data constant and only use the char pointer and indexing? data might be a really big byte array, so detaching should be avoided. Or how about wrapping it into a QBuffer? Or could it somehow be avoided to construct the QByteArray in the first place, and operate on some char pointer instead? Yes, that's much better. Works well.

Created attachment 70282 [details] Patch

Created attachment 70358 [details] Patch

I'd say it might be the wrong layer to support this here and the support should be in QNetworkAccessManager. However there is a lack of people that can handle the stuff there, so I guess this is fine for now. Which servers send multipart images and why? I have not heard of that before. I think there should be comments in QNetworkReplyHandler::forwardData() and QNetworkReplyHandler::sendResponseIfNeeded() explaining why the function/data flow is going like this. m_boundariesRead should be called m_multipartBoundariesRead IMHO

(In reply to comment #10) > I'd say it might be the wrong layer to support this here and the support should be in QNetworkAccessManager. However there is a lack of people that can handle the stuff there, so I guess this is fine for now. Yeah, I'm of the same opinion now. Parsing data for HTTP headers just feels wrong in QNetworkReplyHandler. QNAM should feed it the individual sections of the multipart message with the appropriate header for each. > > Which servers send multipart images and why? I have not heard of that before. > http://en.wikipedia.org/wiki/Server_push#HTTP_server_push It's a pretty old proposal/standard and ignored by IE. I think supporting this beyond streaming images, for which it was originally intended, may be a case of diminishing returns. If the patch is too intrusive it may not be worth supporting at all. I don't know who uses it in the wild. > I think there should be comments in QNetworkReplyHandler::forwardData() and QNetworkReplyHandler::sendResponseIfNeeded() explaining why the function/data flow is going like this. > > m_boundariesRead should be called m_multipartBoundariesRead IMHO Will do. Before I work on this further maybe we should decide how much of this (if any) we want to support? A simple implementation that supports a multipart/x-mixed-replace response containing nothing but image sections requires the least overhead and will be achieved by this patch. An implementation that supports a multipart response containing sections with different mime-types requires quite a bit more code.

(In reply to comment #11) > If the patch is too intrusive it may not be worth supporting at all. > > I don't know who uses it in the wild. > Before I work on this further maybe we should decide how much of this (if any) we want to support? > > A simple implementation that supports a multipart/x-mixed-replace response containing nothing but image sections requires the least overhead and will be achieved by this patch. MJPEG streams are implemented as a multipart/x-mixed-replace response with a new frame in each part. Supporting this case would be very useful - and most browsers do. Notably, many IP cameras use MJPEG streams.

So, do we want this? Should I clean it up for final review?

Comment on attachment 70358 [details] Patch Clearing review for now, going to come back to this.

=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.