[webkit-reviews] review granted: [Bug 63490] [Qt] Add more tests to cover the behavior of loadFinished() signal : [Attachment 101336] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Caio Marcelo de Oliveira
Filho <cmarcelo at webkit.org>'s request for review:
Bug 63490: [Qt] Add more tests to cover the behavior of loadFinished() signal
https://bugs.webkit.org/show_bug.cgi?id=63490

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101336&action=review


This looks correct. I did some nitpicking for the sake of it, feel free to cq+
this version if you wish :)

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2325
> +	   } else if (request.url() ==
QUrl("http://this.will/return-http-404-error-without-contents.html")) {

It would be nice if
"http://this.will/return-http-404-error-without-contents.html" was a constant
QUrl defined for the whole file instead of copying the string.

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2326
> +	       setError(QNetworkReply::ContentNotFoundError, "Not found");

I think the tr() around "Not found" is interesting iff "Not found" is a
standard string inside Qt.