Bug 58375

Summary: [Qt] QNetworkReplyHandler refactoring: remove nested event loop.
Product: WebKit Reporter: Luiz Agostini <luiz>
Component: New BugsAssignee: Luiz Agostini <luiz>
Status: RESOLVED FIXED    
Severity: Normal CC: kenneth, kling, ossy, webkit.review.bot
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 57075, 58430    
Bug Blocks:    
Attachments:
Description Flags
patch
kling: review+
patch none

Description Luiz Agostini 2011-04-12 14:12:01 PDT
A nested event loop is not needed for synchronous loads any more.
Comment 1 Kenneth Rohde Christiansen 2011-04-12 14:17:30 PDT
So where is the patch :-) ;-)
Comment 2 Luiz Agostini 2011-04-12 14:30:47 PDT
Created attachment 89273 [details]
patch
Comment 3 WebKit Review Bot 2011-04-12 14:33:56 PDT
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 4 Kenneth Rohde Christiansen 2011-04-12 14:40:42 PDT
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 5 Andreas Kling 2011-04-12 14:41:34 PDT
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.
Comment 6 Luiz Agostini 2011-04-12 15:49:15 PDT
(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);
Comment 7 Luiz Agostini 2011-04-12 16:04:28 PDT
Created attachment 89296 [details]
patch
Comment 8 Andreas Kling 2011-04-12 16:11:48 PDT
Comment on attachment 89273 [details]
patch

After discussing, I prefer this approach to keeping things clean. :3
Comment 9 Andreas Kling 2011-04-12 16:20:07 PDT
(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.
Comment 10 Csaba Osztrogonác 2011-04-12 17:29:21 PDT
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
Comment 11 Luiz Agostini 2011-04-12 21:39:30 PDT
(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. :)
Comment 12 Luiz Agostini 2011-04-12 22:03:28 PDT
	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
Comment 13 Csaba Osztrogonác 2011-04-13 04:28:30 PDT
(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.
Comment 14 Csaba Osztrogonác 2011-04-13 04:43:47 PDT
(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)
Comment 15 Csaba Osztrogonác 2011-04-14 03:55:54 PDT
Landed in http://trac.webkit.org/changeset/83788