[webkit-reviews] review granted: [Bug 57049] [Qt] QNetworkReplyHandler refactoring: signal sequence. : [Attachment 87382] patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Luiz Agostini
<luiz at webkit.org>'s request for review:
Bug 57049: [Qt] QNetworkReplyHandler refactoring: signal sequence.
https://bugs.webkit.org/show_bug.cgi?id=57049

Attachment 87382: patch
https://bugs.webkit.org/attachment.cgi?id=87382&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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"?