Bug 31583 – [Qt] better test coverage for ErrorPageExtension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

load errors in iframes and framesets are currently not covered by qwebpage's autotests ...

patch coming...

[Antonio Gomes]

Created attachment 43357 [details]
patch 0.1

[Kenneth Rohde Christiansen]

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?

[Antonio Gomes]

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.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Antonio Gomes]

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

[Kenneth Rohde Christiansen]

Comment on attachment 43359 [details]
patch 0.2

ChangeLog was not updated!

[Antonio Gomes]

Created attachment 43364 [details]
(committed r51076) patch 0.3

Update changelog, sorry about that.

[Antonio Gomes]

Comment on attachment 43364 [details]
(committed r51076) patch 0.3

thx for reviewing.

[Antonio Gomes]

landed r51076