Bug 36702

Summary: [Qt] NPP_SetWindow seems to not be called when TestNetscapePlugin is moved
Product: WebKit Reporter: Robert Hogan <robert>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, kbalazs, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 40730, 53420    
Bug Blocks:    
Attachments:
Description Flags
Test Case
none
Patch
none
Patch hausmann: review+

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>