[Qt] Add more tests to cover the behavior of loadFinished() signal
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>