Bug 63490 – [Qt] Add more tests to cover the behavior of loadFinished() signal

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

[Qt] Add more tests to cover the behavior of loadFinished() signal

[Caio Marcelo de Oliveira Filho]

Created attachment 98809 [details]
Patch

[Caio Marcelo de Oliveira Filho]

This patch was motivated by bug 61328, since we needed to keep some functionality that wasn't previously tested.

[Alexis Menard (darktears)]

Comment on attachment 98809 [details]
Patch

LGTM.

[Caio Marcelo de Oliveira Filho]

Robert, Benjamin: could either of you take a look at this one?

[Benjamin Poulain]

Comment on attachment 98809 [details]
Patch

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

The patch looks correct but I am not sure ErrorPage isn't leaked.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2579
> +    ErrorPage* page = new ErrorPage;

Why is this page allocated on the stack? Isn't it leaked?
QWebView::setPage() does not take ownership.

[Caio Marcelo de Oliveira Filho]

Created attachment 101336 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #5)
> (From update of attachment 98809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98809&action=review
> 
> The patch looks correct but I am not sure ErrorPage isn't leaked.
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2579
> > +    ErrorPage* page = new ErrorPage;
> 
> Why is this page allocated on the stack? Isn't it leaked?
> QWebView::setPage() does not take ownership.

Yes, it was leaking. Thanks for the catch. Uploaded a new version with that issue fixed.

The other tests led me to think that QWebView took the ownership. I'll make a patch to fix them too.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
> The other tests led me to think that QWebView took the ownership. I'll make a patch to fix them too.

This is in bug 64814.

[Benjamin Poulain]

Comment on attachment 101336 [details]
Patch

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 101866 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #9)
> > 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.

I uploaded a new patch with this implemented. I added the URL as a static const member for FakeReply class.


> > 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.

I'm not familiar with translation system in Qt, but after digging in the Qt source for a while, I don't think there's a built-in translation for "Not found". Since it isn't something any user will see, I think it's fine to not use tr() here.

[WebKit Review Bot]

Comment on attachment 101866 [details]
Patch

Clearing flags on attachment: 101866

Committed r91669: <http://trac.webkit.org/changeset/91669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r91669 cherry-picked into qtwebkit-2.2 with commit 8a95d2b <http://gitorious.org/webkit/qtwebkit/commit/8a95d2b>