Bug 63490 - [Qt] Add more tests to cover the behavior of loadFinished() signal
Summary: [Qt] Add more tests to cover the behavior of loadFinished() signal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 61328
  Show dependency treegraph
 
Reported: 2011-06-27 15:54 PDT by Caio Marcelo de Oliveira Filho
Modified: 2011-07-29 08:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.73 KB, patch)
2011-06-27 16:04 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (5.76 KB, patch)
2011-07-19 09:55 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2011-07-25 07:30 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-06-27 15:54:33 PDT
[Qt] Add more tests to cover the behavior of loadFinished() signal
Comment 1 Caio Marcelo de Oliveira Filho 2011-06-27 16:04:24 PDT
Created attachment 98809 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2011-06-27 16:05:42 PDT
This patch was motivated by bug 61328, since we needed to keep some functionality that wasn't previously tested.
Comment 3 Alexis Menard (darktears) 2011-06-28 11:05:38 PDT
Comment on attachment 98809 [details]
Patch

LGTM.
Comment 4 Caio Marcelo de Oliveira Filho 2011-07-06 07:25:01 PDT
Robert, Benjamin: could either of you take a look at this one?
Comment 5 Benjamin Poulain 2011-07-07 15:38:00 PDT
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.
Comment 6 Caio Marcelo de Oliveira Filho 2011-07-19 09:55:00 PDT
Created attachment 101336 [details]
Patch
Comment 7 Caio Marcelo de Oliveira Filho 2011-07-19 09:56:26 PDT
(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.
Comment 8 Caio Marcelo de Oliveira Filho 2011-07-19 11:19:24 PDT
(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 9 Benjamin Poulain 2011-07-25 05:18:21 PDT
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.
Comment 10 Caio Marcelo de Oliveira Filho 2011-07-25 07:30:04 PDT
Created attachment 101866 [details]
Patch
Comment 11 Caio Marcelo de Oliveira Filho 2011-07-25 07:34:25 PDT
(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 12 WebKit Review Bot 2011-07-25 08:51:31 PDT
Comment on attachment 101866 [details]
Patch

Clearing flags on attachment: 101866

Committed r91669: <http://trac.webkit.org/changeset/91669>
Comment 13 WebKit Review Bot 2011-07-25 08:51:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Ademar Reis 2011-07-29 08:41:46 PDT
Revision r91669 cherry-picked into qtwebkit-2.2 with commit 8a95d2b <http://gitorious.org/webkit/qtwebkit/commit/8a95d2b>