Bug 58375

So where is the patch :-) ;-)

Created attachment 89273 [details] patch

Attachment 89273 [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/ResourceHandleQt.cpp:56: The parameter name "response" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:56: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 89273 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=89273&action=review > Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:66 > + ResourceResponse& m_response; > + ResourceError& m_error; > + Vector<char>& m_data; so we own these? who were setting them before? Im wondering, as I dont see that code removed here

Comment on attachment 89273 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=89273&action=review Nice cleanup, just one thing- > Source/WebCore/ChangeLog:37 > > 2011-04-12 Luiz Agostini <luiz.agostini@openbossa.org> > This hunk doesn't start at the top of the ChangeLog file :) > Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:192 > + // starting in deferred mode gives d->m_job the chance of being set before sending the request. > + d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true); > + d->m_job->setLoadingDeferred(false); This would be much less mysterious if QNetworkReplyHandler had a start() method that we would explicitly call from the outside.

(In reply to comment #5) > (From update of attachment 89273 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89273&action=review > > > Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:192 > > + // starting in deferred mode gives d->m_job the chance of being set before sending the request. > > + d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true); > > + d->m_job->setLoadingDeferred(false); > > This would be much less mysterious if QNetworkReplyHandler had a start() method that we would explicitly call from the outside. This new method would be such that it would be called only once, just like the constructor. :) For this to work I would be putting back flags to this class, what I would like to avoid. Would it be better? // starting in deferred mode gives d->m_job the chance of being set before sending the request. QNetworkReplyHandler* replyHandler = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true); d->m_job = replyHandler; d->m_job->setLoadingDeferred(false);

Created attachment 89296 [details] patch

Comment on attachment 89273 [details] patch After discussing, I prefer this approach to keeping things clean. :3

(In reply to comment #4) > (From update of attachment 89273 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89273&action=review > > > Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:66 > > + ResourceResponse& m_response; > > + ResourceError& m_error; > > + Vector<char>& m_data; > > so we own these? who were setting them before? Im wondering, as I dont see that code removed here Look at ResourceHandle::loadResourceSynchronously(), this way the ownership is simply transferred to the out-arguments without making temporary copies in WebCoreSynchronousLoader.

I had to rollout http://trac.webkit.org/changeset/83659 too, because it was conflicted with buggy http://trac.webkit.org/changeset/83634. See https://bugs.webkit.org/show_bug.cgi?id=57075

(In reply to comment #9) > (In reply to comment #4) > > (From update of attachment 89273 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89273&action=review > > > > > Source/WebCore/platform/network/qt/ResourceHandleQt.cpp:66 > > > + ResourceResponse& m_response; > > > + ResourceError& m_error; > > > + Vector<char>& m_data; > > > > so we own these? who were setting them before? Im wondering, as I dont see that code removed here > > Look at ResourceHandle::loadResourceSynchronously(), this way the ownership is simply transferred to the out-arguments without making temporary copies in WebCoreSynchronousLoader. Sorry Kenneth, I did not see your question in time. :)

M Source/WebCore/ChangeLog M Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp M Source/WebCore/platform/network/qt/QNetworkReplyHandler.h M Source/WebCore/platform/network/qt/ResourceHandleQt.cpp Committed r83695

(In reply to comment #12) > M Source/WebCore/ChangeLog > M Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp > M Source/WebCore/platform/network/qt/QNetworkReplyHandler.h > M Source/WebCore/platform/network/qt/ResourceHandleQt.cpp > Committed r83695 It broke http tests with WK2. :( See https://bugs.webkit.org/show_bug.cgi?id=58430 for details.

(In reply to comment #13) > (In reply to comment #12) > > M Source/WebCore/ChangeLog > > M Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp > > M Source/WebCore/platform/network/qt/QNetworkReplyHandler.h > > M Source/WebCore/platform/network/qt/ResourceHandleQt.cpp > > Committed r83695 > > It broke http tests with WK2. :( > > See https://bugs.webkit.org/show_bug.cgi?id=58430 for details. It was rolled out by http://trac.webkit.org/changeset/83720. (rs=kling)

Landed in http://trac.webkit.org/changeset/83788