Bug 46259 – [Qt] QNetworkReplyHandler forces buffered output for FormData with files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sriram Neelakandan]

Created attachment 68360 [details]
intial proposal 

Webkit's QNRHandler.cpp does not set up
Content-Length and  QNetworkRequest::DoNotBufferUploadDataAttribute
and eventually forces QNetworkReplyImpl to buffer up the data;

This is not fine for uploading large files, since systems will run out of buffer memory

Attached is a patch for fixing the same

The patch is not formatted yet, approach needs to be decided yet
https://lists.webkit.org/pipermail/webkit-qt/2010-September/000870.html

[WebKit Review Bot]

Attachment 68360 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 68360 [details]
intial proposal 

View in context: https://bugs.webkit.org/attachment.cgi?id=68360&action=review

r- for compilation-breaking typo and missing ChangeLog entry.
Please see http://trac.webkit.org/wiki/QtWebKitContrib for information on how to make patches for WebKit, specifically the "Submitting a patch" section.

> QNetworkReplyHandler.h.new:100
> +    qint64 getFormDataSize() { return m_totalSize; }

This method should be const.

> QNetworkReplyHandler.cpp.new:70
> +    m_totalSize=computeSize();

Coding style, should be "m_totalSize = computeSize();"

> QNetworkReplyHandler.cpp.new:81
> +    for (int i=0; i<m_formElements.size(); i++)

Coding style, should be "for (int i = 0; i < m_formElements.size(); ++i)"

> QNetworkReplyHandler.cpp.new:84
> +        if (element.m_type == FormDataElement::data) {

Coding style, shouldn't have {

> QNetworkReplyHandler.cpp.new:85
> +            m_dataSize+=element.m_data.size();

Coding style, should be "m_dataSize += element.m_data.size();"

> QNetworkReplyHandler.cpp.new:88
> +            m_fileSize+=fi.size();

Coding style, should be "m_fileSize += fi.size();"

> QNetworkReplyHandler.cpp.new:94
> +    return (m_dataSize + m_fileSize);

Unnecessary parentheses.

> QNetworkReplyHandler.cpp.new:482
> +            //We may be uploading files so prevent QNR from buffering data

Coding style, please leave a space after //

> QNetworkReplyHandler.cpp.new:483
> +            m_request.setHeader(QNetworkRequest::ContentLengthHeader, posdDevice->getFormDataSize());

Typo, "posdDevice" should be "postDevice"

> QNetworkReplyHandler.cpp.new:494
> +            //We may be uploading files so prevent QNR from buffering data

Coding style, please leave a space after //

> QNetworkReplyHandler.cpp.new:495
> +            m_request.setHeader(QNetworkRequest::ContentLengthHeader, posdDevice->getFormDataSize());

Typo, "posdDevice" should be "postDevice"

[Sriram Neelakandan]

Created attachment 69442 [details]
Addressed style and typo errors of previous patch

thanks for the review comments. Hope this one gets in

[WebKit Review Bot]

Attachment 69442 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/qt/QNetworkReplyHandler.cpp:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69442 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4159037

[Sriram Neelakandan]

Created attachment 69445 [details]
Fix style and EWS errors

Hope this passes style and EWS

[Andreas Kling]

Comment on attachment 69445 [details]
Fix style and EWS errors

View in context: https://bugs.webkit.org/attachment.cgi?id=69445&action=review

Close, but not quite there yet.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:88
> +        if (element.m_type == FormDataElement::data) 
> +            m_dataSize += element.m_data.size();
> +        else {
> +            QFileInfo fi(element.m_filename);
> +            m_fileSize += fi.size();
> +        }

If building with ENABLE(BLOB), this fails to take FormDataElement::encodedBlob into account. (see FormData.h)

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:92
> +#ifdef QT_NETWORK_DEBUG
> +    qDebug() << __PRETTY_FUNCTION__ << "FileSize=" << m_fileSize << "DataSize=" << m_dataSize;
> +#endif

Please remove this.
This kind of output should only be added for things that are non-trivial to do using a debugger.

> WebCore/platform/network/qt/QNetworkReplyHandler.h:100
> +    qint64 getFormDataSize() const { return m_totalSize; }

You don't need the m_totalSize member, just "return m_dataSize + m_fileSize;"

[Sriram Neelakandan]

Created attachment 69454 [details]
Removed m_totalSize and Debug statement

Did not address BLOB support. formdataiodevice does not support BLOB yet.
Will add another bug for the same.

[Andreas Kling]

Comment on attachment 69454 [details]
Removed m_totalSize and Debug statement

Very nice, r=me

[WebKit Commit Bot]

Comment on attachment 69454 [details]
Removed m_totalSize and Debug statement

Clearing flags on attachment: 69454

Committed r69064: <http://trac.webkit.org/changeset/69064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r69064 cherry-picked into qtwebkit-2.1.x with commit 2bb9aaf <http://gitorious.org/webkit/qtwebkit/commit/2bb9aaf>