Bug 57075

Summary:

[Qt] QNetworkReplyHandler refactoring: signal queue

Product:

WebKit

Reporter:

Luiz Agostini <luiz>

Component:

New Bugs

Assignee:

Luiz Agostini <luiz>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cmarcelo, hausmann, jturcotte, kenneth, kling, menard, ossy, webkit.review.bot

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

57049

Bug Blocks:

57083, 58375

Attachments:

Description	Flags
patch	none
patch	none
patch	none

Luiz Agostini

Reported 2011-03-24 19:59:58 PDT

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.

Attachments
patch (15.82 KB, patch) 2011-03-24 21:31 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
patch (17.79 KB, patch) 2011-04-12 11:31 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
patch (17.77 KB, patch) 2011-04-12 11:46 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Luiz Agostini

Comment 1 2011-03-24 21:31:00 PDT

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

Comment 2 2011-03-28 07:37:26 PDT

- 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

Comment 3 2011-04-12 11:31:44 PDT

Created attachment 89232 [details] patch

WebKit Review Bot

Comment 4 2011-04-12 11:34:20 PDT

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 5 2011-04-12 11:41:40 PDT

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

Comment 6 2011-04-12 11:46:18 PDT

Created attachment 89234 [details] patch style fix

Andreas Kling

Comment 7 2011-04-12 11:53:53 PDT

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

Comment 8 2011-04-12 14:25:02 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 r83634

Csaba Osztrogonác

Comment 9 2011-04-12 17:26:31 PDT

(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

Comment 10 2011-04-12 21:08:06 PDT

Alexis Menard (darktears)

Comment 11 2011-04-27 09:28:06 PDT

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.

Note You need to log in before you can comment on or make changes to this bug.