Summary: | [Qt] Qt DRT: respect window.close() and window.closed() | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, hausmann, jwieczorek, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Robert Hogan
2009-12-26 12:17:00 PST
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. |