Bug 57075 – [Qt] QNetworkReplyHandler refactoring: signal queue

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

[Luiz Agostini]

Now the idea is to make all signals that come from the QNetworkReply to pass through a queue and to stop that queue when loading is deferred.

This way almost all the deferred logic can be removed from QNetworkReplyHandler class and encapsulated in its own class.

[Luiz Agostini]

Created attachment 86877 [details]
patch

I will not set the review flag because this patch depends on the patch in bug 57049 and all bots would be red.

But please review it.

[Jocelyn Turcotte]

- Please explain why Lock is needed in QNetworkReplyWrapper::receiveMetaData, in a comment as well would be great.
- m_queue->setDeferSignals(m_queue->deferSignals()) could cause surprises if more logic is added to setDeferSignals. Deferring bugs easily slip through tests and some explicit flush() method call would be safer.
- Unless I'm missing something, you shouldn't need the inline keyword at the declaration of receiveReadyRead

[Luiz Agostini]

Created attachment 89232 [details]
patch

[WebKit Review Bot]

Attachment 89232 [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/QNetworkReplyHandler.h:42:  The parameter name "handler" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:62:  The parameter name "queue" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 89232 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89232&action=review

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:77
> +    void readyRead();

didReceiveReadyRead()?

[Luiz Agostini]

Created attachment 89234 [details]
patch

style fix

[Andreas Kling]

Comment on attachment 89234 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89234&action=review

r=me. This is a great step towards making this spaghetti understandable without spending 2 hours reading it. :)

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:77
> +    void readyRead();

didReceiveReadyRead() or something like that would sound more slot-like.

[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 r83634

[Csaba Osztrogonác]

(In reply to comment #8)
>     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 r83634

It was rolled out by http://trac.webkit.org/changeset/83673,
because it made two layout tests time out:

http://build.webkit.org/results/Qt%20Linux%20Release/r83635%20%2831280%29/results.html

[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 r83691

[Alexis Menard (darktears)]

I don't see why It should be part of 2.1.x. It's a huge refactoring with some other patches and has no purpose to be in 2.1.x.