Bug 32953 - [Qt] Qt DRT: respect window.close() and window.closed()
Summary: [Qt] Qt DRT: respect window.close() and window.closed()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-26 12:17 PST by Robert Hogan
Modified: 2010-01-23 08:54 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.20 KB, patch)
2009-12-26 12:19 PST, Robert Hogan
eric: review-
Details | Formatted Diff | Diff
Updated to identify and delete window closed (7.49 KB, patch)
2009-12-29 05:54 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated to identify and delete window closed (7.49 KB, patch)
2009-12-29 06:00 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated to identify and delete window closed (deleted)
2009-12-30 06:38 PST, Robert Hogan
hausmann: review-
Details | Formatted Diff | Diff
Updated Patch (7.27 KB, patch)
2009-12-30 12:22 PST, Robert Hogan
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2010-01-16 09:07 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch to fix crashes (5.11 KB, patch)
2010-01-21 14:50 PST, Robert Hogan
hausmann: review-
Details | Formatted Diff | Diff
Updated Patch (7.59 KB, patch)
2010-01-23 07:41 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2009-12-26 12:17:00 PST
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
Comment 1 Robert Hogan 2009-12-26 12:19:22 PST
Created attachment 45515 [details]
Patch
Comment 2 WebKit Review Bot 2009-12-26 12:24:58 PST
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 3 Eric Seidel (no email) 2009-12-29 00:16:46 PST
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.
Comment 4 Robert Hogan 2009-12-29 05:54:43 PST
Created attachment 45596 [details]
Updated to identify and delete window closed

Yes - it is possible. Thanks for catching.
Comment 5 WebKit Review Bot 2009-12-29 05:55:55 PST
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
Comment 6 Robert Hogan 2009-12-29 06:00:28 PST
Created attachment 45597 [details]
Updated to identify and delete window closed

Updated per stylebot. Passed for me locally for some reason.
Comment 7 WebKit Review Bot 2009-12-29 06:01:33 PST
style-queue ran check-webkit-style on attachment 45597 [details] without any errors.
Comment 8 Jakub Wieczorek 2009-12-30 06:03:35 PST
> +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();
Comment 9 Robert Hogan 2009-12-30 06:38:38 PST
Created attachment 45661 [details]
Updated to identify and delete window closed

Thanks fawek!
Comment 10 WebKit Review Bot 2009-12-30 06:45:37 PST
style-queue ran check-webkit-style on attachment 45661 [details] without any errors.
Comment 11 Simon Hausmann 2009-12-30 06:46:07 PST
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 !(... == ...) :-)
Comment 12 Robert Hogan 2009-12-30 12:22:53 PST
Created attachment 45676 [details]
Updated Patch
Comment 13 WebKit Review Bot 2009-12-30 12:23:23 PST
style-queue ran check-webkit-style on attachment 45676 [details] without any errors.
Comment 14 Eric Seidel (no email) 2010-01-05 13:28:01 PST
Comment on attachment 45676 [details]
Updated Patch

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

Otherwise looks fine to me.
Comment 15 Eric Seidel (no email) 2010-01-05 13:28:37 PST
This can either be landed manually, or a new patch posted with indent fix and I'm happy to cq+.
Comment 16 Eric Seidel (no email) 2010-01-14 13:07:04 PST
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.
Comment 17 Robert Hogan 2010-01-16 09:07:29 PST
Created attachment 46742 [details]
Patch

Rebased patch
Comment 18 WebKit Commit Bot 2010-01-21 01:12:32 PST
Comment on attachment 46742 [details]
Patch

Clearing flags on attachment: 46742

Committed r53615: <http://trac.webkit.org/changeset/53615>
Comment 19 Simon Hausmann 2010-01-21 01:28:34 PST
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
Comment 20 Robert Hogan 2010-01-21 14:50:20 PST
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 21 Simon Hausmann 2010-01-22 08:01:01 PST
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 :)
Comment 22 Robert Hogan 2010-01-23 07:41:13 PST
Created attachment 47273 [details]
Updated Patch
Comment 23 WebKit Commit Bot 2010-01-23 08:54:41 PST
Comment on attachment 47273 [details]
Updated Patch

Clearing flags on attachment: 47273

Committed r53770: <http://trac.webkit.org/changeset/53770>
Comment 24 WebKit Commit Bot 2010-01-23 08:54:51 PST
All reviewed patches have been landed.  Closing bug.