Bug 31583 - [Qt] better test coverage for ErrorPageExtension
Summary: [Qt] better test coverage for ErrorPageExtension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-11-17 05:44 PST by Antonio Gomes
Modified: 2009-11-17 09:59 PST (History)
1 user (show)

See Also:


Attachments
patch 0.1 (4.66 KB, patch)
2009-11-17 05:49 PST, Antonio Gomes
kenneth: review-
Details | Formatted Diff | Diff
patch 0.2 (4.59 KB, patch)
2009-11-17 06:04 PST, Antonio Gomes
kenneth: review-
Details | Formatted Diff | Diff
(committed r51076) patch 0.3 (4.92 KB, patch)
2009-11-17 07:43 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-11-17 05:44:25 PST
load errors in iframes and framesets are currently not covered by qwebpage's autotests ...

patch coming...
Comment 1 Antonio Gomes 2009-11-17 05:49:16 PST
Created attachment 43357 [details]
patch 0.1
Comment 2 Kenneth Rohde Christiansen 2009-11-17 05:52:59 PST
Comment on attachment 43357 [details]
patch 0.1


> +        (tst_QWebPage::errorPageExtension_withIFrames):
> +        (tst_QWebPage::errorPageExtension_withFrameset):

Strange function naming

>  void tst_QWebPage::infiniteLoopJS()
>  {
> +    return;

Doesn't seem right


> -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> +    m_view->setUrl(QUrl("data:text/html,foo"));
> +    QTest::qWait(2000);

Can these waits be avoided?
Comment 3 Antonio Gomes 2009-11-17 06:04:22 PST
Created attachment 43359 [details]
patch 0.2

thx for reviewing.


> +        (tst_QWebPage::errorPageExtension_withIFrames):
> +        (tst_QWebPage::errorPageExtension_withFrameset):
> Strange function naming

fixed. 

>  void tst_QWebPage::infiniteLoopJS()
>  {
> +    return;
> Doesn't seem right

typo, removed.

> -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> +    m_view->setUrl(QUrl("data:text/html,foo"));
> +    QTest::qWait(2000);
> Can these waits be avoided?

removed.
Comment 4 Kenneth Rohde Christiansen 2009-11-17 06:22:28 PST
> >  void tst_QWebPage::infiniteLoopJS()
> >  {
> > +    return;
> > Doesn't seem right
> 
> typo, removed.

If that is a typo, I will need to look very careful at your patches. Please be more careful.

> 
> > -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> > +    m_view->setUrl(QUrl("data:text/html,foo"));
> > +    QTest::qWait(2000);
> > Can these waits be avoided?
> 
> removed.

Why was it there in the first place?
Comment 5 Kenneth Rohde Christiansen 2009-11-17 06:26:29 PST
Comment on attachment 43359 [details]
patch 0.2

Please respond these questions:

> @@ -1629,12 +1631,8 @@ public:
>          const ErrorPageExtensionOption* info = static_cast<const ErrorPageExtensionOption*>(option);
>          ErrorPageExtensionReturn* errorPage = static_cast<ErrorPageExtensionReturn*>(output);
>  
> -        if (info->frame == mainFrame()) {
> -            errorPage->content = "data:text/html,error";
> -            return true;
> -        }
> -
> -        return false;
> +        errorPage->content = "data:text/html,error";
> +        return true;
>      }

This above doesn't seem to have to do with your new change. If not, please explain that in the changelog.

>  };
>  
> @@ -1645,11 +1643,10 @@ void tst_QWebPage::errorPageExtension()
>  
>      QSignalSpy spyLoadFinished(m_view, SIGNAL(loadFinished(bool)));
>  
> -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> +    m_view->setUrl(QUrl("data:text/html,foo"));

Is the frametest/index.html not needed anymore now?

> -    QCOMPARE(page->history()->currentItem().url(), QUrl("qrc:///frametest/index.html"));
> +    QCOMPARE(page->history()->currentItem().url(), QUrl("data:text/html,foo"));
> +
> +    m_view->setPage(0);

Why is this needed. Is it part of the test? Add a comment
Comment 6 Antonio Gomes 2009-11-17 06:45:35 PST
> > typo, removed.
> If that is a typo, I will need to look very careful at your patches. Please be
> more careful.

well, it is really a typo. see 'infiniteLoop()' test (where i added the 'return') takes ~15 seconds running in my box. I added a 'return' there just to speed up my own test execution. so it was a small typo to forget to remove the 'return' and hapelly you catch it and it is fixed, so thank you again.

> > > -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> > > +    m_view->setUrl(QUrl("data:text/html,foo"));
> > > +    QTest::qWait(2000);
> > > Can these waits be avoided?
> > removed.
> Why was it there in the first place?

there were not needed. removed.

> >          const ErrorPageExtensionOption* info = static_cast<const ErrorPageExtensionOption*>(option);
> >          ErrorPageExtensionReturn* errorPage = static_cast<ErrorPageExtensionReturn*>(output);
> >  
> > -        if (info->frame == mainFrame()) {
> > -            errorPage->content = "data:text/html,error";
> > -            return true;
> > -        }
> > -
> > -        return false;
> > +        errorPage->content = "data:text/html,error";
> > +        return true;
> >      }
> 
> This above doesn't seem to have to do with your new change. If not, please
> explain that in the changelog.

It is related, yes. We were firstly handling error pages for the mainFrame only. See "if (info->frame == mainFrame())". Now the "if" got removed, and we are handling it for all frames (which purpose of the patch).

> > @@ -1645,11 +1643,10 @@ void tst_QWebPage::errorPageExtension()
> >  
> >      QSignalSpy spyLoadFinished(m_view, SIGNAL(loadFinished(bool)));
> >  
> > -    page->mainFrame()->load(QUrl("qrc:///frametest/index.html"));
> > +    m_view->setUrl(QUrl("data:text/html,foo"));
> 
> Is the frametest/index.html not needed anymore now?

it is not needed for the test "errorPageExtension" then I replaced it for a simpler URL. It is needed for the newly added "errorPageExtensionForFrameset" test, which specifically tests 'framesets'.

> Why is this needed. Is it part of the test? Add a comment
> > -    QCOMPARE(page->history()->currentItem().url(), QUrl("qrc:///frametest/index.html"));
> > +    QCOMPARE(page->history()->currentItem().url(), QUrl("data:text/html,foo"));

since i changed the url (see my answer to the question above ) it is needed.

> > +    m_view->setPage(0);

it resets the page for the next test.
Comment 7 Kenneth Rohde Christiansen 2009-11-17 07:14:46 PST
Comment on attachment 43359 [details]
patch 0.2

ChangeLog was not updated!
Comment 8 Antonio Gomes 2009-11-17 07:43:30 PST
Created attachment 43364 [details]
(committed r51076) patch 0.3

Update changelog, sorry about that.
Comment 9 Antonio Gomes 2009-11-17 09:58:46 PST
Comment on attachment 43364 [details]
(committed r51076) patch 0.3

thx for reviewing.
Comment 10 Antonio Gomes 2009-11-17 09:59:04 PST
landed r51076