(will attach the patch tomorrow)
Created attachment 74914 [details] patch attempt 1
Attachment 74914 [details] did not build on qt: Build output: http://queues.webkit.org/results/6315052
Created attachment 74922 [details] patch attempt 2
Comment on attachment 74922 [details] patch attempt 2 View in context: https://bugs.webkit.org/attachment.cgi?id=74922&action=review > WebCore/platform/SharedBuffer.cpp:242 > -#if !PLATFORM(CF) || PLATFORM(QT) > +#if !PLATFORM(CF) && !PLATFORM(QT) this wont break normal build? > WebCore/platform/SharedBuffer.h:45 > #else > class NSData; > #endif > +#endif This added endif here seems wrong > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:238 > + m_request.setAttribute(gMaximumDownloadBufferSizeAttribute, 1024*256); // 256 kB needs spaces around *. Need dot at end of comment > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:370 > + // Check if there is a zerocopy buffer Needs dot at end of comment > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:378 > + // Old school way. Use QNetworkReply as a QIODevice Strange comment, rewrite also add dot at the end. > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:380 > + m_usingZeroCopy = false; maybe call this m_usingZeroCopyFeature > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:485 > + // don't emit the "Document has moved here" type of HTML End with dot, starts with capital > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:503 > + oldSize, bytesReceived - oldSize, bytesReceived - oldSize /*FixMe*/); call it FIXME: plus add a description on WHAT to fix! > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:638 > + // For zero copy Misses dot. > WebCore/platform/network/qt/QNetworkReplyHandler.h:94 > + // for zerocopy. Holds the download data: fix comment > WebCore/platform/network/qt/QNetworkReplyHandler.h:96 > + // for zerocopy it wraps m_byteBlock, otherwise it holds data normally: also > WebCore/platform/qt/QByteBlock.cpp:29 > + QByteBlock* bb = new QByteBlock; write out bb > WebCore/platform/qt/QByteBlock.cpp:46 > + return (const char*)(m_data.data()); I do not think the latter () are needed > WebCore/platform/qt/QByteBlock.cpp:51 > + m_size = s; just write out s > WebCore/platform/qt/QByteBlock.cpp:56 > + m_capacity = c; and c!
Created attachment 75372 [details] v3
Comment on attachment 75372 [details] v3 View in context: https://bugs.webkit.org/attachment.cgi?id=75372&action=review > WebCore/platform/SharedBuffer.h:45 > #endif > +#endif I trust you that this is git showing the diff wrong... if not, you need to fix it > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:237 > + // Then this buffer is provided to us in the QNetworkReply Where is the dot! > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:240 > + > + No need for two newlines here > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:376 > + m_usingZeroCopy = true; Please rename m_usingZeroCopy to m_usingZeroCopyFeature > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:503 > + oldSize, bytesReceived - oldSize, bytesReceived - oldSize); I would keep oldSize on the line above > WebCore/platform/network/qt/ResourceHandleQt.cpp:180 > - return false; > + return true; Shouldnt there be a "if usingZeroCopyFeature" here? > WebCore/platform/qt/QByteBlock.cpp:46 > + return (const char*)(m_data.data()); Remove latter () > WebCore/platform/qt/SharedBufferQt.cpp:50 > + SharedBuffer* sb = new SharedBuffer(); Shouldn't this be RefPtr<SharedBuffer*> sb = new SharedBuffer() ?
Comment on attachment 75372 [details] v3 View in context: https://bugs.webkit.org/attachment.cgi?id=75372&action=review Uploading new patch in a minute:) >> WebCore/platform/SharedBuffer.h:45 >> +#endif > > I trust you that this is git showing the diff wrong... if not, you need to fix it Actually it is not showing it wrong. Just unintuitive. check the whole context. >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:237 >> + // Then this buffer is provided to us in the QNetworkReply > > Where is the dot! argh >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:376 >> + m_usingZeroCopy = true; > > Please rename m_usingZeroCopy to m_usingZeroCopyFeature No, with this patch we are always using the zero copy feature. What we are checking is if the server allowed us to do it by giving a content-length header etc. >> WebCore/platform/network/qt/ResourceHandleQt.cpp:180 >> + return true; > > Shouldnt there be a "if usingZeroCopyFeature" here? No, it's always buffered by QNetworkReplyHandler now. It's just different ways of buffering depending if zerocopy could be used or not. See QNetworkReplyHandler::sendResponseIfNeeded() >> WebCore/platform/qt/SharedBufferQt.cpp:50 >> + SharedBuffer* sb = new SharedBuffer(); > > Shouldn't this be RefPtr<SharedBuffer*> sb = new SharedBuffer() ? I copied/modified that from somewhere else and as far as I understand the refcounting it should be fine..
Created attachment 75383 [details] v4
Comment on attachment 75383 [details] v4 View in context: https://bugs.webkit.org/attachment.cgi?id=75383&action=review > WebCore/WebCore.gypi:3144 > + 'platform/qt/QByteBlock.cpp', > + 'platform/qt/QByteBlock.h', Why is this called QByteBlock? ByteBlockQt seems more in line what we have elsewhere
(In reply to comment #9) > (From update of attachment 75383 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75383&action=review > > > WebCore/WebCore.gypi:3144 > > + 'platform/qt/QByteBlock.cpp', > > + 'platform/qt/QByteBlock.h', > > Why is this called QByteBlock? ByteBlockQt seems more in line what we have elsewhere I am sorry that we always nitpick so much in WebKit. I have to admit that both names have issues. ByteBlockQt indicate that it is some generic interface and this is the Qt specific part and QByteBlock has the danger of clashing with future versions of Qt. :} I think for now you can remove the 'capacity' call as no one is calling it? I also have some questions. So QNetworkAccessManager will internally start changing the QSharedDataPointer<char>? Does this work with realloc? Somehow I would assume to see something like a QList<QByteArray> but I don't. So Qt will still re-alloc internally?
(In reply to comment #10) > I am sorry that we always nitpick so much in WebKit. I have to admit that both names have issues. ByteBlockQt indicate that it is some generic interface and this is the Qt specific part and QByteBlock has the danger of clashing with future versions of Qt. :} Yeah, well, but this one will always have the WebCore namespace :) > I think for now you can remove the 'capacity' call as no one is calling it? I might use it later with the image decoders, let's see. > > I also have some questions. So QNetworkAccessManager will internally start changing the QSharedDataPointer<char>? Does this work with realloc? Somehow I would assume to see something like a QList<QByteArray> but I don't. So Qt will still re-alloc internally? It will not re-allocate. It goes through this codepath only if you allow it to do so (in the QNetworkRequest) and then only if the server supports it (e.g. content-length header is there). In my measurements 70% of the bytes coming with normal websurfing have that condition. The QList<QByteArray> thing (actually not that but similar) is used internally in the other 30% of the cases. Thanks for comments! :)
This is why we should call it WebCore::QtByteBlock, this follows our current way of doing things
Created attachment 76367 [details] v4 (rebased) Here is the rebased patch, feel free to modify it or redo it from scratch before resending it for review. I couldn't test the patch completely since, with Qt 4.7.1, I always got a null pointer from m_reply->attribute(QNetworkRequest::DownloadBufferAttribute). I got the same result with the v4 non-rebased patch so this might be a bug in Qt. You might want to verify the patch with a correct behaving Qt or you'll get a crash if I made any mistake rebasing that code-path.
Comment on attachment 76367 [details] v4 (rebased) Oh come on Jocelyn :-) won't you do that QByteBlock to QtByteBlock renaming?
Comment on attachment 76367 [details] v4 (rebased) View in context: https://bugs.webkit.org/attachment.cgi?id=76367&action=review Could you please rebase again your changes? The source tree was reorganized some times ago. Thanks > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:59 > +#if QT_VERSION >= QT_VERSION_CHECK(4, 7, 0) We do now :).
Created attachment 84725 [details] one more patch attempt
Attachment 84725 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:484: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 84725 [details] did not build on qt: Build output: http://queues.webkit.org/results/8094034
Created attachment 84739 [details] ..and one more, this time with the new files
Comment on attachment 84739 [details] ..and one more, this time with the new files View in context: https://bugs.webkit.org/attachment.cgi?id=84739&action=review > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:231 > + // Then this buffer is provided to us in the QNetworkReply. I don't understand this English > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:382 > + m_usingZeroCopy = true; Wrong identation
Committed r80551: <http://trac.webkit.org/changeset/80551>
http://trac.webkit.org/changeset/80551 might have broken Qt Windows 32-bit Release and Qt Windows 32-bit Debug
(In reply to comment #21) > Committed r80551: <http://trac.webkit.org/changeset/80551> Reopen, because it was rolled out: http://trac.webkit.org/changeset/80560
Not much news from me here, got no time to work on this these days :( If no one else takes it over I'll hope I can do it before Qt 4.8 comes out..
Any volunteers ? :)
I will take a look.