Qt DRT needs to maintain a correct count of open windows for windowCount(). It also needs to delete windows that have been closed by window.close(). This fixes the following tests: plugins/destroy-during-npp-new.html fast/dom/Document/early-document-access.html fast/dom/Window/window-early-properties.html fast/events/open-window-from-another-frame.html fast/events/popup-blocking-click-in-iframe.html
Created attachment 45515 [details] Patch
Attachment 45515 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/ChangeLog:19: Line contains tab character. [whitespace/tab] [5] Total errors found: 1
Comment on attachment 45515 [details] Patch There is really no way to know here? 4 void DumpRenderTree::windowCloseRequested() 755 { 756 // We don't know which window was closed, so we have to assume it was the one 757 // most recently opened. 758 if (windows.count()) { 759 QObject* container = windows.takeLast(); 760 container->deleteLater(); 761 } 762 } The tabs will prevent this from being landed as is.
Created attachment 45596 [details] Updated to identify and delete window closed Yes - it is possible. Thanks for catching.
Attachment 45596 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:760: Declaration has space between type name and * in QWebPage *page [whitespace/declaration] [3] Total errors found: 1
Created attachment 45597 [details] Updated to identify and delete window closed Updated per stylebot. Passed for me locally for some reason.
style-queue ran check-webkit-style on attachment 45597 [details] without any errors.
> +void DumpRenderTree::windowCloseRequested() > +{ > + QWebPage* page = qobject_cast<QWebPage*>(sender()); > + for (int i = 0; i < windows.size(); ++i) { > + QObject* container = windows.at(i); > + QWebPage* childPage = static_cast<QWebPage*>(container->findChild<WebPage *>()); > + if (childPage == page) { > + windows.takeAt(i); > + container->deleteLater(); > + break; > + } > + } > +} I believe QWebPage::parent() would give you the correct container, no? Then you can simply do: QWebPage* page = qobject_cast<QWebPage*>(sender()); QObject* container = page->parent(); windows.removeAll(container); container->deleteLater();
Created attachment 45661 [details] Updated to identify and delete window closed Thanks fawek!
style-queue ran check-webkit-style on attachment 45661 [details] without any errors.
Comment on attachment 45661 [details] Updated to identify and delete window closed > + // It is possible that we get called by windows created from the main page that have finished > + // loading, so we don't ASSERT here. At the moment we do not gather results from such windows, > + // but may need to in future. > + if (!(sender() == m_topLoadingFrame->page())) > + return; The change looks very good to me, but please change the above to use != instead of !(... == ...) :-)
Created attachment 45676 [details] Updated Patch
style-queue ran check-webkit-style on attachment 45676 [details] without any errors.
Comment on attachment 45676 [details] Updated Patch Bad indent here: 93 if (sender() != m_topLoadingFrame->page()) 94 return; Otherwise looks fine to me.
This can either be landed manually, or a new patch posted with indent fix and I'm happy to cq+.
I tried to land this manually, but svn-apply failed. Failed to run "['/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel']" exit_code: 255 Last 500 characters of output: | 5 --- WebKitTools/ChangeLog | 28 ++++++++++++++++++++ WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp | 10 +++++++ WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.h | 1 + .../DumpRenderTree/qt/LayoutTestControllerQt.cpp | 6 +++- 6 files changed, 66 insertions(+), 6 deletions(-) ------------------------------------------------------------------- Died at /Users/eseidel/Projects/WebKit/WebKitTools/Scripts/svn-apply line 336, <> line 179.
Created attachment 46742 [details] Patch Rebased patch
Comment on attachment 46742 [details] Patch Clearing flags on attachment: 46742 Committed r53615: <http://trac.webkit.org/changeset/53615>
Rolled out in http://trac.webkit.org/changeset/53616 as it caused two crashes on the Qt bot: http://build.webkit.org/results/Qt%20Linux%20Release/r53615%20(6308)/results.html
Created attachment 47148 [details] Patch to fix crashes Very sorry about that - I can only recreate one of the crashes locally (the first) and have fixed it with the attached. The second still has the debug output on the buildbot and I can see it crashed for the same reason, so this should fix both. It's not entirely clear to me why m_topLoadingFrame is null when loadFinished fires so the fix is just a workaround. Also - patch has no changelog since original one is still there - assume that's OK?
Comment on attachment 47148 [details] Patch to fix crashes Patch looks good to me, but please provide it as complete patch with ChangeLog, then we can land it through the bot :)
Created attachment 47273 [details] Updated Patch
Comment on attachment 47273 [details] Updated Patch Clearing flags on attachment: 47273 Committed r53770: <http://trac.webkit.org/changeset/53770>
All reviewed patches have been landed. Closing bug.