Bug 32953 – [Qt] Qt DRT: respect window.close() and window.closed()

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

[Robert Hogan]

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

[Robert Hogan]

Created attachment 45515 [details]
Patch

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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.

[Robert Hogan]

Created attachment 45596 [details]
Updated to identify and delete window closed

Yes - it is possible. Thanks for catching.

[WebKit Review Bot]

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

[Robert Hogan]

Created attachment 45597 [details]
Updated to identify and delete window closed

Updated per stylebot. Passed for me locally for some reason.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45597 [details] without any errors.

[Jakub Wieczorek]

> +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();

[Robert Hogan]

Created attachment 45661 [details]
Updated to identify and delete window closed

Thanks fawek!

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45661 [details] without any errors.

[Simon Hausmann]

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 !(... == ...) :-)

[Robert Hogan]

Created attachment 45676 [details]
Updated Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45676 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45676 [details]
Updated Patch

Bad indent here:
 93     if (sender() != m_topLoadingFrame->page())
 94       return;

Otherwise looks fine to me.

[Eric Seidel (no email)]

This can either be landed manually, or a new patch posted with indent fix and I'm happy to cq+.

[Eric Seidel (no email)]

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.

[Robert Hogan]

Created attachment 46742 [details]
Patch

Rebased patch

[WebKit Commit Bot]

Comment on attachment 46742 [details]
Patch

Clearing flags on attachment: 46742

Committed r53615: <http://trac.webkit.org/changeset/53615>

[Simon Hausmann]

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

[Robert Hogan]

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?

[Simon Hausmann]

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 :)

[Robert Hogan]

Created attachment 47273 [details]
Updated Patch

[WebKit Commit Bot]

Comment on attachment 47273 [details]
Updated Patch

Clearing flags on attachment: 47273

Committed r53770: <http://trac.webkit.org/changeset/53770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.