Bug 62385 – [GTK] [Qt] Eliminate duplicate TestNetscapePlugin implementation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 96606 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96606 [details]
Patch

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

[Martin Robinson]

Created attachment 96608 [details]
Try to fix the cr-linux build

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Andreas Kling]

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.

[Martin Robinson]

Committed r88471: <http://trac.webkit.org/changeset/88471>

[Csaba Osztrogonác]

It was rolled out by r88471, because it broke all plugin tests on Qt.

Guys, you should have run layout tests before landing!

[Martin Robinson]

(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.

[Csaba Osztrogonác]

(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 ...

[Andreas Kling]

(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.

[Antonio Gomes]

> 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!

[Martin Robinson]

Created attachment 96768 [details]
Patch fixing Qt issues

[Martin Robinson]

(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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Martin Robinson]

Created attachment 96805 [details]
Patch also removing unecessary cr-linux result

[Martin Robinson]

I forgot to remove the now unecessary cr-linux expectation! I've removed it and re-uploaded the patch.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Martin Robinson]

Hrm. The cr-linux test is still failing. Could it be that this test just needs a rebaseline?

[Adam Barth]

(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.

[Martin Robinson]

Created attachment 96813 [details]
Patch which just updates the cr-linux expected results instead of removing them

[Martin Robinson]

(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.

[Eric Seidel (no email)]

Comment on attachment 96813 [details]
Patch which just updates the cr-linux expected results instead of removing them

Sounds good to me.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

*** Bug 61783 has been marked as a duplicate of this bug. ***