Bug 33333 - [Qt] pass plugins/open-and-close-window-with-plugin.html on Qt
Summary: [Qt] pass plugins/open-and-close-window-with-plugin.html on Qt
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: Qt
Depends on:
Blocks:
 
Reported: 2010-01-07 11:24 PST by Robert Hogan
Modified: 2010-06-18 14:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.10 KB, patch)
2010-01-07 11:28 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (3.24 KB, patch)
2010-06-13 14:19 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2010-06-17 14:26 PDT, Robert Hogan
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-01-07 11:24:40 PST
This test cannot pass on platforms other than Mac because the NP plugin
is a child of the page rather than the webview on those platforms.

The test is intended to cater for cases where a window is closed but
-[webview close] is not called by the plugin - Mac experienced a
regression where you could close the window and the plugin would continue
displaying because the plugin did not close the webview. When a window is closed
on non-Mac platforms the page is deleted and the plugin destroyed, so
the message logged by the plugin in the test reaches a deleted object.
In the case of Qt, because the object is (being) deleted the log message
from the plugin arrives in the QWebPage's implementation of
javascriptConsoleMessage() (which does nothing by default) rather than
in that of DRT's derived class WebPage (which writes to stdout). It's
pure chance that the memory has not been overwritten at
that point so it would not be correct to alter QWebpage to
cater for it. (Note that this behaviour only occurs in the DRT anyway
because the test plugin has logDestroy property).
Comment 1 Robert Hogan 2010-01-07 11:28:11 PST
Created attachment 46069 [details]
Patch
Comment 2 WebKit Review Bot 2010-01-07 11:37:46 PST
style-queue ran check-webkit-style on attachment 46069 [details] without any errors.
Comment 3 Maciej Stachowiak 2010-01-22 02:17:42 PST
Comment on attachment 46069 [details]
Patch

Are you sure it can't pass on platforms other than Mac? I don't think it's been a problem on anything but Qt. I think the right fix may be to put this test in the Qt skipped list. r- to consider that option (or just reflag if you are sure this is the best approach).
Comment 4 Robert Hogan 2010-06-13 13:25:32 PDT
(In reply to comment #3)
> (From update of attachment 46069 [details])
> Are you sure it can't pass on platforms other than Mac? I don't think it's been a problem on anything but Qt. I think the right fix may be to put this test in the Qt skipped list. r- to consider that option (or just reflag if you are sure this is the best approach).

It's skipped by everyone except chromium:

LayoutTests/platform/mac-snowleopard/Skipped:plugins/open-and-close-window-with-plugin.html
LayoutTests/platform/qt/Skipped:plugins/open-and-close-window-with-plugin.html
LayoutTests/platform/gtk/Skipped:plugins/open-and-close-window-with-plugin.html
LayoutTests/platform/win/Skipped:plugins/open-and-close-window-with-plugin.html
Comment 5 Robert Hogan 2010-06-13 14:19:25 PDT
Created attachment 58603 [details]
Patch
Comment 6 Robert Hogan 2010-06-13 14:21:04 PDT
(In reply to comment #5)
> Created an attachment (id=58603) [details]
> Patch

Maybe it's ok/expected that QWebPage can receive the call to javasriptConsoleMessage() once DRT's WebPage is deleted. I'm not sure, so putting this up as a solution for discussion.
Comment 7 Simon Hausmann 2010-06-15 07:22:05 PDT
Comment on attachment 58603 [details]
Patch

scary :) r=me
Comment 8 Robert Hogan 2010-06-17 13:39:39 PDT
Committed r61346: <http://trac.webkit.org/changeset/61346>
Comment 9 Robert Hogan 2010-06-17 13:59:36 PDT
Reverted r61346 for reason:

Broke inspector tests on Qt

Committed r61348: <http://trac.webkit.org/changeset/61348>
Comment 10 Robert Hogan 2010-06-17 14:26:10 PDT
Created attachment 59034 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2010-06-17 16:00:35 PDT
Comment on attachment 59034 [details]
Patch

I dont get it. The WebPage inherits from QWebPage, and you say that the WebPage part has been destroyed but not the QWebPage one? Wouldn't it be better to fix the destruction?
Comment 12 Kenneth Rohde Christiansen 2010-06-17 16:03:21 PDT
(In reply to comment #11)
> (From update of attachment 59034 [details])
> I dont get it. The WebPage inherits from QWebPage, and you say that the WebPage part has been destroyed but not the QWebPage one? Wouldn't it be better to fix the destruction?

Apparently now that I am reading your original comment
Comment 13 Robert Hogan 2010-06-18 14:01:41 PDT
Committed r61433: <http://trac.webkit.org/changeset/61433>