Bug 46259 - [Qt] QNetworkReplyHandler forces buffered output for FormData with files
Summary: [Qt] QNetworkReplyHandler forces buffered output for FormData with files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-22 05:43 PDT by Sriram Neelakandan
Modified: 2011-04-29 07:22 PDT (History)
5 users (show)

See Also:


Attachments
intial proposal (3.17 KB, patch)
2010-09-22 05:43 PDT, Sriram Neelakandan
akling: review-
Details | Formatted Diff | Diff
Addressed style and typo errors of previous patch (4.48 KB, patch)
2010-10-01 03:14 PDT, Sriram Neelakandan
no flags Details | Formatted Diff | Diff
Fix style and EWS errors (4.66 KB, patch)
2010-10-01 03:52 PDT, Sriram Neelakandan
akling: review-
Details | Formatted Diff | Diff
Removed m_totalSize and Debug statement (4.48 KB, patch)
2010-10-01 05:29 PDT, Sriram Neelakandan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sriram Neelakandan 2010-09-22 05:43:45 PDT
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
Comment 1 WebKit Review Bot 2010-09-28 21:54:46 PDT
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.
Comment 2 Andreas Kling 2010-09-29 01:11:54 PDT
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"
Comment 3 Sriram Neelakandan 2010-10-01 03:14:29 PDT
Created attachment 69442 [details]
Addressed style and typo errors of previous patch

thanks for the review comments. Hope this one gets in
Comment 4 WebKit Review Bot 2010-10-01 03:16:45 PDT
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.
Comment 5 Early Warning System Bot 2010-10-01 03:22:49 PDT
Attachment 69442 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4159037
Comment 6 Sriram Neelakandan 2010-10-01 03:52:32 PDT
Created attachment 69445 [details]
Fix style and EWS errors

Hope this passes style and EWS
Comment 7 Andreas Kling 2010-10-01 04:50:35 PDT
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;"
Comment 8 Sriram Neelakandan 2010-10-01 05:29:00 PDT
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.
Comment 9 Andreas Kling 2010-10-01 06:06:31 PDT
Comment on attachment 69454 [details]
Removed m_totalSize and Debug statement

Very nice, r=me
Comment 10 WebKit Commit Bot 2010-10-04 21:16:29 PDT
Comment on attachment 69454 [details]
Removed m_totalSize and Debug statement

Clearing flags on attachment: 69454

Committed r69064: <http://trac.webkit.org/changeset/69064>
Comment 11 WebKit Commit Bot 2010-10-04 21:16:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Ademar Reis 2011-04-29 07:22:41 PDT
Revision r69064 cherry-picked into qtwebkit-2.1.x with commit 2bb9aaf <http://gitorious.org/webkit/qtwebkit/commit/2bb9aaf>