Bug 58375 – [Qt] QNetworkReplyHandler refactoring: remove nested event loop.

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

[Luiz Agostini]

A nested event loop is not needed for synchronous loads any more.

[Kenneth Rohde Christiansen]

So where is the patch :-) ;-)

[Luiz Agostini]

Created attachment 89273 [details]
patch

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Andreas Kling]

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  <[e-mail]>
>  

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.

[Luiz Agostini]

(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);

[Luiz Agostini]

Created attachment 89296 [details]
patch

[Andreas Kling]

Comment on attachment 89273 [details]
patch

After discussing, I prefer this approach to keeping things clean. :3

[Andreas Kling]

(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.

[Csaba Osztrogonác]

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

[Luiz Agostini]

(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. :)

[Luiz Agostini]

	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

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

(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)

[Csaba Osztrogonác]

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