Bug 57049 – [Qt] QNetworkReplyHandler refactoring: signal sequence.

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

[Luiz Agostini]

This is the first step in QNetworkReplyHandler. The main objective here is to create simple invariants: 

 1 - that the signals metadatachanged, readyRead and finished will come in this order.
 2 - that signals metadatachanged and finished will come for sure.

Having these invariants further simplifications will be possible and will come in future patches.

[Luiz Agostini]

Created attachment 86823 [details]
patch

[Kenneth Rohde Christiansen]

Kling, Jocelyn can you have a look at this?

[Benjamin Poulain]

Could you give a bit of context? Which bugs is this fixing?

About the new enforced invariants:
 1 - that the signals metadatachanged, readyRead and finished will come in this order.
 2 - that signals metadatachanged and finished will come for sure.

In which case are they not met with the current code?

[Luiz Agostini]

(In reply to comment #3)
> Could you give a bit of context? Which bugs is this fixing?
> 
> About the new enforced invariants:
>  1 - that the signals metadatachanged, readyRead and finished will come in this order.
>  2 - that signals metadatachanged and finished will come for sure.
> 
> In which case are they not met with the current code?

The current code expects the signals to come in any sequence.

Slots forwardData() and finish() call sendResponseIfNeeded() and a flag named m_responseSent is kept to indicate if sendResponseIfNeeded() is been called for the second time.

All those slots (forwardData, finish and sendResponseIfNeeded) are connected to a QNetworkReply object.

[Luiz Agostini]

This is a refactoring that will make further changes. I tried to provide patches that are easy to understand each one alone. But the whole refactoring is much larger.

Basically the strategy is to:

 1 - create a wrapper that will enforce the invariants that I need and will be the place to add further logic as mime sniffing. (bug 57049)
 2 - separate the 'deferred' logic from the actual content processing logic. (bug 57075)
 3 - separate the logic for redirection from the logic for receive meta data. (bug 57083)
 4 - cleanup. (bug 57092)
 5 - create a separated class to be used in synchronous requests.

[Jocelyn Turcotte]

Concerns:
- I'm not sure why you need to disconnect and reconnect the reply at each signal received. This could have performance impacts, especially for readyRead.
- In QNetworkReplyWrapper::receiveMetaData, each time you will receive readyRead from the reply, you will do the mime type dance and emit metadataChanged. Doing it once the first time should be enough.
- The QNetworkReplyWrapper class name doesn't define much it's responsibility, though I can't find a better name myself so...

This whole refactoring is awesome but it moves more tiny networking bug fixes than it may look. I think it would be better not to get this in QtWebKit 2.2 unless of course there is a clear gain and we put additional efforts to test this code.

[Luiz Agostini]

(In reply to comment #6)
> Concerns:
> - I'm not sure why you need to disconnect and reconnect the reply at each signal received. This could have performance impacts, especially for readyRead.

At construction time all signals from QNetworkReply are connected to receiveMetaData and they are disconnected as soon as the first one come. The slot receiveMetaData is not used form this point on. If needed, the signals from QNetworkReply will be connected to other slots. Specifically, readyRead signal form QNetworkReply will only trigger QNetworkReplyWrapper's own readyRead signal.

 214    connect(m_reply, SIGNAL(readyRead()), this, SIGNAL(readyRead()));
 215    connect(m_reply, SIGNAL(finished()), this, SLOT(receiveFinished()));

> - In QNetworkReplyWrapper::receiveMetaData, each time you will receive readyRead from the reply, you will do the mime type dance and emit metadataChanged. Doing it once the first time should be enough.

The slot receiveMetaData is only be used to receive the first signal from QNetwrokReply. Signal metadataChanged is emitted only once. The mime type dance will only happen exactly once.

> - The QNetworkReplyWrapper class name doesn't define much it's responsibility, though I can't find a better name myself so...
> 
> This whole refactoring is awesome but it moves more tiny networking bug fixes than it may look. I think it would be better not to get this in QtWebKit 2.2 unless of course there is a clear gain and we put additional efforts to test this code.

Ok, the whole refactoring may be too much for this time but this first patch was designed to be clearly sane and to prepare the way to mime sniffing integration. Please then consider integrating this patch alone for this first time.

[Jocelyn Turcotte]

(In reply to comment #7)
> At construction time all signals from QNetworkReply are connected to receiveMetaData and they are disconnected as soon as the first one come. The slot receiveMetaData is not used form this point on. If needed, the signals from QNetworkReply will be connected to other slots. Specifically, readyRead signal form QNetworkReply will only trigger QNetworkReplyWrapper's own readyRead signal.
> 
>  214    connect(m_reply, SIGNAL(readyRead()), this, SIGNAL(readyRead()));
>  215    connect(m_reply, SIGNAL(finished()), this, SLOT(receiveFinished()));
> 
> > - In QNetworkReplyWrapper::receiveMetaData, each time you will receive readyRead from the reply, you will do the mime type dance and emit metadataChanged. Doing it once the first time should be enough.
> 
> The slot receiveMetaData is only be used to receive the first signal from QNetwrokReply. Signal metadataChanged is emitted only once. The mime type dance will only happen exactly once.

Ahh, I missed that part, it makes more sense.
Please add a comment somewhere to highlight this.

> 
> Ok, the whole refactoring may be too much for this time but this first patch was designed to be clearly sane and to prepare the way to mime sniffing integration. Please then consider integrating this patch alone for this first time.

I have nothing against having it in trunk, as long as we are aware of the risk. In worst conditions we can revert it in the future branch.

[Luiz Agostini]

Created attachment 87382 [details]
patch

adding the comments requested by Jocelyn.

[Kenneth Rohde Christiansen]

Comment on attachment 87382 [details]
patch

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

This is pretty readable and understandable

> Source/WebCore/ChangeLog:11
> +            2 - that signals metadatachanged and finished will will be called exactly once.

two will's there

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:176
> +    disconnect(reply, 0, this, 0);

Isn't there a disconnectAll or so?

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:189
> +    // this slot is only used to receive the first signal from the QNetworkReply object.

comments should start with capital - basically they should be real sentences

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:207
> +        // if not finished, connect to the slots that will be used from this point on.

same here

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:209
> +        connect(m_reply, SIGNAL(finished()), this, SLOT(receiveFinished()));

receive_d_Finished?

more webkit like, didReceiveFinished

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:226
> +    // disconnecting will make sure that nothing will happen after emiting finished signal.

Disco...*

after emitting (two t's) THE finished signal

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:48
> +    QUrl redirectUrl() const { return m_redirectUrl; }

I think redirectionUrl would be more correct. or redirectionTargetUrl()

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.h:50
> +    QString officialMimeType() const { return m_officialMimeType; }

I don't like the official part of the name? maybe "advertisedMimeType"?

[Kenneth Rohde Christiansen]

Have you tried measuring the performance impact of this change? I think it might be a good think to do, even though we think it won't affect it.

[Luiz Agostini]

(In reply to comment #11)
> Have you tried measuring the performance impact of this change? I think it might be a good think to do, even though we think it won't affect it.

I have made some only simple tests with QtTestBrowser in robot mode using a master build as reference. I did not see any differences.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82478 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
canvas/philip/tests/2d.text.draw.align.center.html
fast/blockflow/broken-ideographic-font.html

[Luiz Agostini]

	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp
	M	Source/WebCore/platform/network/qt/QNetworkReplyHandler.h
Committed r82478