Bug 36702 - [Qt] NPP_SetWindow seems to not be called when TestNetscapePlugin is moved
Summary: [Qt] NPP_SetWindow seems to not be called when TestNetscapePlugin is moved
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on: 40730 53420
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-27 07:18 PDT by Robert Hogan
Modified: 2011-01-31 05:55 PST (History)
4 users (show)

See Also:


Attachments
Test Case (1.45 KB, text/plain)
2010-03-27 07:18 PDT, Robert Hogan
no flags Details
Patch (3.88 KB, patch)
2010-06-14 14:40 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (5.22 KB, patch)
2010-06-19 13:02 PDT, Robert Hogan
hausmann: 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-03-27 07:18:54 PDT
Created attachment 51828 [details]
Test Case

In the attached test, replacing

            window.addEventListener('load', doTest, false);

with

            window.addEventListener('load', function() {
                window.setTimeout(doTest, 0);

will cause the test to fail.
Comment 1 Alexey Proskuryakov 2010-03-27 11:51:25 PDT
The test expectation is "should not crash". Are you saying that it crashes if doTest is deferred?

Note that WebKit Qt component is for bugs in the WebKit Qt API layer.
Comment 2 Robert Hogan 2010-03-27 12:04:35 PDT
(In reply to comment #1)
> The test expectation is "should not crash". Are you saying that it crashes if
> doTest is deferred?
> 
> Note that WebKit Qt component is for bugs in the WebKit Qt API layer.

No, it times out in Qt. Note that you need to have the patch at https://bugs.webkit.org/show_bug.cgi?id=36675 applied to encounter the bug.
Comment 3 Alexey Proskuryakov 2010-03-27 14:10:59 PDT
This seems to imply that NPP_SetWindow is not called on plug-in if it's moved after initial layout. I think that's correct behavior for windowed plug-ins (but I'm not sure whether TestNetscapePlugin is windowed or windowless, particularly on Qt).
Comment 4 Balazs Kelemen 2010-06-03 05:49:39 PDT
Please clarify the situation:
  * is it crashing or just not working correctly?
  * is this the correct behavior as ap suggested or not?
Comment 5 Robert Hogan 2010-06-03 12:11:57 PDT
(In reply to comment #4)
> Please clarify the situation:
>   * is it crashing or just not working correctly?
>   * is this the correct behavior as ap suggested or not?

The two tests: LayoutTests/plugins/update-widgets-crash.html and LayoutTests/plugins/reentrant-update-widget-positions.html are the same except for that the former has:
           window.addEventListener('load', doTest, false);

instead of

            window.addEventListener('load', function() {
                window.setTimeout(doTest, 0);

LayoutTests/plugins/update-widgets-crash.html passes while LayoutTests/plugins/reentrant-update-widget-positions.html fails.

It fails by timing out.
Comment 6 Robert Hogan 2010-06-14 14:40:53 PDT
Created attachment 58702 [details]
Patch
Comment 7 WebKit Commit Bot 2010-06-17 00:33:21 PDT
Comment on attachment 58702 [details]
Patch

Clearing flags on attachment: 58702

Committed r61311: <http://trac.webkit.org/changeset/61311>
Comment 8 WebKit Commit Bot 2010-06-17 00:33:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Csaba Osztrogonác 2010-06-17 02:02:31 PDT
It was rolled out by http://trac.webkit.org/changeset/61315,
because it make 2 tests fail:
http://build.webkit.org/results/Qt%20Linux%20Release/r61314%20%2813453%29/results.html
Comment 10 Robert Hogan 2010-06-17 10:35:38 PDT
(In reply to comment #9)
> It was rolled out by http://trac.webkit.org/changeset/61315,
> because it make 2 tests fail:
> http://build.webkit.org/results/Qt%20Linux%20Release/r61314%20%2813453%29/results.html

That was sloppy, sorry. I'll look into this now.
Comment 11 Robert Hogan 2010-06-17 10:50:07 PDT
(In reply to comment #9)
> It was rolled out by http://trac.webkit.org/changeset/61315,
> because it make 2 tests fail:
> http://build.webkit.org/results/Qt%20Linux%20Release/r61314%20%2813453%29/results.html

Qt expected results for resize-from-plugin.html were wrong anyway by the looks of it, the new results are slightly less wrong but still problematic. 

The mac expected result is:

x: 18, y: 52, width: 200, height: 200, clipRect: (18, 52, 200, 142)

Qt result was (without patch):

x: 18, y: 54, width: 100, height: 100, clipRect: (0, 0, 100, 100)

And with patch:

x: 18, y: 54, width: 100, height: 100, clipRect: (0, 0, 200, 142)

So closer, but still no cigar.

The other test looks like a minor layout difference due to the extra resizing now taking place in DRT. So results just need to be updated when I land the corrected patch.
Comment 12 Robert Hogan 2010-06-19 13:02:47 PDT
Created attachment 59192 [details]
Patch
Comment 13 Robert Hogan 2010-06-20 05:54:47 PDT
Committed r61505: <http://trac.webkit.org/changeset/61505>