load errors in iframes and framesets are currently not covered by qwebpage's autotests ... patch coming...
Created attachment 43357 [details] patch 0.1
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?
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.
> > 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 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
> > 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 on attachment 43359 [details] patch 0.2 ChangeLog was not updated!
Created attachment 43364 [details] (committed r51076) patch 0.3 Update changelog, sorry about that.
Comment on attachment 43364 [details] (committed r51076) patch 0.3 thx for reviewing.
landed r51076