Summary: | [Qt] Add more tests to cover the behavior of loadFinished() signal | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||
Component: | WebKit Qt | Assignee: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ademar, benjamin, robert, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 61328 | ||||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2011-06-27 15:54:33 PDT
Created attachment 98809 [details]
Patch
This patch was motivated by bug 61328, since we needed to keep some functionality that wasn't previously tested. Comment on attachment 98809 [details]
Patch
LGTM.
Robert, Benjamin: could either of you take a look at this one? 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. Created attachment 101336 [details]
Patch
(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. (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. 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. Created attachment 101866 [details]
Patch
(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. Comment on attachment 101866 [details] Patch Clearing flags on attachment: 101866 Committed r91669: <http://trac.webkit.org/changeset/91669> All reviewed patches have been landed. Closing bug. Revision r91669 cherry-picked into qtwebkit-2.2 with commit 8a95d2b <http://gitorious.org/webkit/qtwebkit/commit/8a95d2b> |