Bug 58375 - [Qt] QNetworkReplyHandler refactoring: remove nested event loop.
Summary: [Qt] QNetworkReplyHandler refactoring: remove nested event loop.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Luiz Agostini
URL:
Keywords: Qt
Depends on: 57075 58430
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-12 14:12 PDT by Luiz Agostini
Modified: 2011-04-14 03:55 PDT (History)
4 users (show)

See Also:


Attachments
patch (8.01 KB, patch)
2011-04-12 14:30 PDT, Luiz Agostini
kling: review+
Details | Formatted Diff | Diff
patch (8.68 KB, patch)
2011-04-12 16:04 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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