Bug 62385 - [GTK] [Qt] Eliminate duplicate TestNetscapePlugin implementation
Summary: [GTK] [Qt] Eliminate duplicate TestNetscapePlugin implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
: 61783 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-09 10:53 PDT by Martin Robinson
Modified: 2011-06-22 19:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (24.59 KB, patch)
2011-06-09 11:10 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Try to fix the cr-linux build (24.73 KB, patch)
2011-06-09 11:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.23 MB, application/zip)
2011-06-09 11:42 PDT, WebKit Review Bot
no flags Details
Patch fixing Qt issues (25.64 KB, patch)
2011-06-10 11:50 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.38 MB, application/zip)
2011-06-10 12:55 PDT, WebKit Review Bot
no flags Details
Patch also removing unecessary cr-linux result (27.35 KB, patch)
2011-06-10 15:29 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.42 MB, application/zip)
2011-06-10 15:49 PDT, WebKit Review Bot
no flags Details
Patch which just updates the cr-linux expected results instead of removing them (27.43 KB, patch)
2011-06-10 15:56 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-06-09 10:53:22 PDT
GTK+ and Qt use Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp, which is almost identical to Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp. Having two implementation of the same file makes it difficult to keep them in sync.
Comment 1 Martin Robinson 2011-06-09 11:10:21 PDT
Created attachment 96606 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-09 11:15:36 PDT
Comment on attachment 96606 [details]
Patch

Attachment 96606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8811944
Comment 3 Martin Robinson 2011-06-09 11:26:48 PDT
Created attachment 96608 [details]
Try to fix the cr-linux build
Comment 4 WebKit Review Bot 2011-06-09 11:42:16 PDT
Comment on attachment 96608 [details]
Try to fix the cr-linux build

Attachment 96608 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8788945

New failing tests:
plugins/mouse-events.html
Comment 5 WebKit Review Bot 2011-06-09 11:42:21 PDT
Created attachment 96612 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Andreas Kling 2011-06-09 11:45:47 PDT
Comment on attachment 96608 [details]
Try to fix the cr-linux build

View in context: https://bugs.webkit.org/attachment.cgi?id=96608&action=review

Great idea!
r=me

> Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:-671
> -        if (obj->eventLogging)
> -            pluginLog(instance, "adjustCursorEvent");

This will break plugins/mouse-events.html on chromium-linux.
You can simply remove the chromium-specific expectation for this test.
Comment 7 Martin Robinson 2011-06-09 12:31:02 PDT
Committed r88471: <http://trac.webkit.org/changeset/88471>
Comment 8 Csaba Osztrogonác 2011-06-09 19:01:02 PDT
It was rolled out by r88471, because it broke all plugin tests on Qt.

Guys, you should have run layout tests before landing!
Comment 9 Martin Robinson 2011-06-09 19:22:29 PDT
(In reply to comment #8)
> It was rolled out by r88471, because it broke all plugin tests on Qt.
> Guys, you should have run layout tests before landing!

I did run the layout tests, but on GTK+. They passed there. I don't see any obvious reason why they would not pass on Qt as well. I'll investigate further, but I'll need to build for Qt so it will take a littl while.
Comment 10 Csaba Osztrogonác 2011-06-09 19:26:09 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > It was rolled out by r88471, because it broke all plugin tests on Qt.
> > Guys, you should have run layout tests before landing!
> 
> I did run the layout tests, but on GTK+. They passed there. I don't see any obvious reason why they would not pass on Qt as well. I'll investigate further, but I'll need to build for Qt so it will take a littl while.

OK, but the Qt reviewer should have done it. :-S And should do it next time ...
Comment 11 Andreas Kling 2011-06-10 00:53:25 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > It was rolled out by r88471, because it broke all plugin tests on Qt.
> > > Guys, you should have run layout tests before landing!
> > 
> > I did run the layout tests, but on GTK+. They passed there. I don't see any obvious reason why they would not pass on Qt as well. I'll investigate further, but I'll need to build for Qt so it will take a littl while.
> 
> OK, but the Qt reviewer should have done it. :-S And should do it next time ...

Given the nature of this bug, I assumed the tests had already been run on GTK+ and Qt. In retrospect, I should have asked about it. Anyways, that doesn't matter now.

But please relax already- occasional bot breakage is not the end of the world- roll out the change and move on. There is no need to get passive-aggressive every time someone causes a test failure.

@Martin: If you need testing your patch on Qt, feel free to poke us on #webkit (or better yet #qtwebkit ;) and we'll hook you up.
Comment 12 Antonio Gomes 2011-06-10 11:05:19 PDT
> But please relax already- occasional bot breakage is not the end of the world- roll out the change and move on. There is no need to get passive-aggressive every time someone causes a test failure.

I make Kling's words mine!
Comment 13 Martin Robinson 2011-06-10 11:50:15 PDT
Created attachment 96768 [details]
Patch fixing Qt issues
Comment 14 Martin Robinson 2011-06-10 11:51:52 PDT
(In reply to comment #13)
> Created an attachment (id=96768) [details]
> Patch fixing Qt issues

I've uploaded a new patch fixing the issues with Qt. I verified that all plugin tests pass in both release and debug buildes. I've made the following changes to the previous patch:

- Added the XP_UNIX define for Qt Linux builds.
- main.cpp no longer forces the plugin into windowless mode unconditionally. It worries me that this did not cause issues on GTK+, but things are looking good now for Qt.
Comment 15 WebKit Review Bot 2011-06-10 12:54:56 PDT
Comment on attachment 96768 [details]
Patch fixing Qt issues

Attachment 96768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8831022

New failing tests:
plugins/mouse-events.html
Comment 16 WebKit Review Bot 2011-06-10 12:55:02 PDT
Created attachment 96773 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 17 Martin Robinson 2011-06-10 15:29:49 PDT
Created attachment 96805 [details]
Patch also removing unecessary cr-linux result
Comment 18 Martin Robinson 2011-06-10 15:30:19 PDT
I forgot to remove the now unecessary cr-linux expectation! I've removed it and re-uploaded the patch.
Comment 19 WebKit Review Bot 2011-06-10 15:48:58 PDT
Comment on attachment 96805 [details]
Patch also removing unecessary cr-linux result

Attachment 96805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8830149

New failing tests:
plugins/mouse-events.html
Comment 20 WebKit Review Bot 2011-06-10 15:49:05 PDT
Created attachment 96812 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 21 Martin Robinson 2011-06-10 15:53:22 PDT
Hrm. The cr-linux test is still failing. Could it be that this test just needs a rebaseline?
Comment 22 Adam Barth 2011-06-10 15:54:18 PDT
(In reply to comment #21)
> Hrm. The cr-linux test is still failing. Could it be that this test just needs a rebaseline?

The results are in the zip that the bot attached.  If they're correct, you can add them to your patch.
Comment 23 Martin Robinson 2011-06-10 15:56:42 PDT
Created attachment 96813 [details]
Patch which just updates the cr-linux expected results instead of removing them
Comment 24 Martin Robinson 2011-06-10 15:57:25 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > Hrm. The cr-linux test is still failing. Could it be that this test just needs a rebaseline?
> 
> The results are in the zip that the bot attached.  If they're correct, you can add them to your patch.

Thanks, Adam. I've attached a new patch with the updated results.
Comment 25 Eric Seidel (no email) 2011-06-13 15:08:48 PDT
Comment on attachment 96813 [details]
Patch which just updates the cr-linux expected results instead of removing them

Sounds good to me.
Comment 26 WebKit Review Bot 2011-06-13 15:25:45 PDT
Comment on attachment 96813 [details]
Patch which just updates the cr-linux expected results instead of removing them

Clearing flags on attachment: 96813

Committed r88712: <http://trac.webkit.org/changeset/88712>
Comment 27 WebKit Review Bot 2011-06-13 15:25:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 noel gordon 2011-06-22 19:45:45 PDT
*** Bug 61783 has been marked as a duplicate of this bug. ***