Bug 77736 - Rewrite SVG tests to make extensive use of display() in repaint tests
Summary: Rewrite SVG tests to make extensive use of display() in repaint tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 77183 77995 78026
Blocks: 69459 77541 78021 78084 78115
  Show dependency treegraph
 
Reported: 2012-02-03 06:58 PST by Nikolas Zimmermann
Modified: 2012-02-09 03:48 PST (History)
8 users (show)

See Also:


Attachments
Patch v1 (deleted)
2012-02-03 07:53 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (deleted)
2012-02-03 08:39 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (deleted)
2012-02-05 09:06 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v4 (deleted)
2012-02-05 09:23 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v5 (deleted)
2012-02-05 14:53 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v6 (deleted)
2012-02-05 16:58 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v7 (deleted)
2012-02-06 02:29 PST, Nikolas Zimmermann
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-02-03 06:58:24 PST
This bug covers all tests in svg/ except for the dynamic-updates/ and custom/ folders -- 140 tests remaining to examine.
Comment 1 Nikolas Zimmermann 2012-02-03 07:44:23 PST
CC'ing Simon & James. I'd be glad if any of you could check the patch, and see whether the approach is sane now.

I basically grepped for all tests containing waitUntilDone() and/or setTimeout calls, and fixed all that don't use dumpAsText() to repaint correctly, by forcing it using display() calls and layout triggers before.

I left out svg/dynamic-updates, and svg/custom as they are quite large, and should be done in a separated bug. There are "only" 140 tests left that need fixing, but I'd like to hear if it's all okay this way.
Comment 2 Nikolas Zimmermann 2012-02-03 07:53:18 PST
Created attachment 125327 [details]
Patch v1
Comment 3 Nikolas Zimmermann 2012-02-03 08:06:30 PST
*grml* I can't upload this using webkit-patch upload, as the patch is too large (it fails w/o an error, pretending it worked).

As a git-newbie, I don't know how to correctly format the patch so it applies.
Comment 4 Nikolas Zimmermann 2012-02-03 08:39:10 PST
Created attachment 125340 [details]
Patch v2

Ok, webkit-patch doesn't handle large files, trying git diff --binary per Ossys suggestion.
Comment 5 Dirk Schulze 2012-02-03 10:07:58 PST
Comment on attachment 125340 [details]
Patch v2

Just as a comment. It might depend on the test, but often you just want to know the result at the end. Therefore I don't see a reason to run the display differences on all tests. This would also make it harder to create Ref Tests in the future. And I really think that we should migrate to Ref Tests as much as possible in order to reduce pixel results (often for every single platform).
Comment 6 James Robinson 2012-02-03 11:07:50 PST
(In reply to comment #5)
> (From update of attachment 125340 [details])
> Just as a comment. It might depend on the test, but often you just want to know the result at the end. Therefore I don't see a reason to run the display differences on all tests. This would also make it harder to create Ref Tests in the future. And I really think that we should migrate to Ref Tests as much as possible in order to reduce pixel results (often for every single platform).

Agree.  Are these tests of type (1) or (2) by the classification in https://bugs.webkit.org/show_bug.cgi?id=77541#c2 ?
Comment 7 Nikolas Zimmermann 2012-02-03 13:34:50 PST
(In reply to comment #6)
> Agree.  Are these tests of type (1) or (2) by the classification in https://bugs.webkit.org/show_bug.cgi?id=77541#c2 ?
All type (2).
Comment 8 James Robinson 2012-02-03 13:41:25 PST
For type (2) I think this is the right approach.  Summarizing some suggestions from IRC:

1.) use LayoutTests/fast/repaint/resources/repaint.js harness, it provides nice fallback when loading the test in a browser and takes care of things like flushing styles so you don't have to do so in every test

2.) run the test logic in window.onload, not from a setTimeout() set in an inline script since the latter might run before the document has been fully parsed. it's an unlikely race to lose but can still cause flake, especially if any of these are ever run as http tests
Comment 9 Nikolas Zimmermann 2012-02-05 09:06:05 PST
Created attachment 125529 [details]
Patch v3
Comment 10 Nikolas Zimmermann 2012-02-05 09:23:20 PST
Created attachment 125530 [details]
Patch v4
Comment 11 Nikolas Zimmermann 2012-02-05 14:53:52 PST
Created attachment 125536 [details]
Patch v5
Comment 12 Nikolas Zimmermann 2012-02-05 16:53:27 PST
Still fighting with a particular layout test, I hope the bots like this patch now.
I've added runSVGRepaintTest() to the repaint.js harness to encapsulate the different handling related to the SVG load event timing. Once we fix the SVG load time, to fire later (as HTML load event) the need for the setTimeout() will vanish - for now centralize this hack, so we can turn it off easily later.
Comment 13 Nikolas Zimmermann 2012-02-05 16:58:10 PST
Created attachment 125544 [details]
Patch v6
Comment 14 WebKit Review Bot 2012-02-05 19:09:50 PST
Comment on attachment 125544 [details]
Patch v6

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

New failing tests:
svg/zoom/text/zoom-coords-viewattr-01-b.svg
Comment 15 WebKit Review Bot 2012-02-05 19:47:23 PST
Comment on attachment 125544 [details]
Patch v6

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

New failing tests:
svg/zoom/text/zoom-coords-viewattr-01-b.svg
Comment 16 Dirk Schulze 2012-02-05 21:23:04 PST
Did you read https://bugs.webkit.org/show_bug.cgi?id=69459#c18 ?
Comment 17 Nikolas Zimmermann 2012-02-06 02:14:36 PST
(In reply to comment #12)
> Still fighting with a particular layout test, I hope the bots like this patch now.
> I've added runSVGRepaintTest() to the repaint.js harness to encapsulate the different handling related to the SVG load event timing. Once we fix the SVG load time, to fire later (as HTML load event) the need for the setTimeout() will vanish - for now centralize this hack, so we can turn it off easily later.

There were line-ending problems with svg/filters/invalidate-child-layout.svg - I fixed them in trunk, hopefully the patch applies now. Will also add a better ChangeLog, and hopefully fix the last chromium expectation problem.
Comment 18 Nikolas Zimmermann 2012-02-06 02:29:55 PST
Created attachment 125601 [details]
Patch v7
Comment 19 Andreas Kling 2012-02-07 02:42:08 PST
Comment on attachment 125601 [details]
Patch v7

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

Looks pretty damn good. r=me!

> LayoutTests/ChangeLog:8
> +        Convert all tests in svg/ (except svg/custom & svg/dynamic-updates) that excersive repainting to use the

Typo, exercise.

> LayoutTests/svg/as-background-image/animated-svg-as-background.html:9
> +    // The animation durates 100ms. Wait 200ms for the repaint.

Not sure 'durate' is an English word. Same comment for all occurrences. :)
Comment 20 Nikolas Zimmermann 2012-02-07 03:20:03 PST
Committed r106918: <http://trac.webkit.org/changeset/106918>
Comment 21 Csaba Osztrogonác 2012-02-07 08:33:50 PST
It made svg/zoom/page/zoom-foreignObject.svg crash with Qt5-WK1.
svg/zoom/page/zoom-foreignObject.svg works fine if I run only this test,
but crahes if I run all tests or svg/zoom/page tests.

1   0x8067c7b /home/oszi/WebKit/WebKitBuild/Release/bin/DumpRenderTree() [0x8067c7b]
2   0xf7742400 [0xf7742400]
3   0xf6531ab6 /home/oszi/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4(WebCore::ThreadTimers::sharedTimerFiredInternal()+0xa6) [0xf6531ab6]
4   0xf6531b95 /home/oszi/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4(WebCore::ThreadTimers::sharedTimerFired()+0x45) [0xf6531b95]
5   0xf671def6 /home/oszi/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4(WebCore::SharedTimerQt::timerEvent(QTimerEvent*)+0x46) [0xf671def6]
6   0xf3ba53d4 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QObject::event(QEvent*)+0x84) [0xf3ba53d4]
7   0xf44967ac /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtWidgets.so.5(QApplicationPrivate::notify_helper(QObject*, QEvent*)+0xac) [0xf44967ac]
8   0xf449d4fe /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtWidgets.so.5(QApplication::notify(QObject*, QEvent*)+0x15e) [0xf449d4fe]
9   0xf3b883db /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QCoreApplication::notifyInternal(QObject*, QEvent*)+0x7b) [0xf3b883db]
10  0xf3bc9ae8 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QTimerInfoList::activateTimers()+0x3b8) [0xf3bc9ae8]
11  0xf3bca58a /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(+0x20358a) [0xf3bca58a]
12  0xf4fa8305 /lib/libglib-2.0.so.0(g_main_context_dispatch+0x1d5) [0xf4fa8305]
13  0xf4fabfe8 /lib/libglib-2.0.so.0(+0x3efe8) [0xf4fabfe8]
14  0xf4fac1c8 /lib/libglib-2.0.so.0(g_main_context_iteration+0x68) [0xf4fac1c8]
15  0xf3bca275 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)+0x65) [0xf3bca275]
16  0xf3b86e61 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)+0x31) [0xf3b86e61]
17  0xf3b87352 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>)+0x132) [0xf3b87352]
18  0xf3b8d5bf /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtCore.so.5(QCoreApplication::exec()+0xaf) [0xf3b8d5bf]
19  0xf3e03f47 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtGui.so.5(QGuiApplication::exec()+0x17) [0xf3e03f47]
20  0xf4495d27 /usr/local/Trolltech/Qt5/Qt-5.0.0-r21/lib/libQtWidgets.so.5(QApplication::exec()+0x27) [0xf4495d27]
21  0x8068a91 /home/oszi/WebKit/WebKitBuild/Release/bin/DumpRenderTree() [0x8068a91]
22  0xf36e0c96 /lib/libc.so.6(__libc_start_main+0xe6) [0xf36e0c96]
Comment 22 Nikolas Zimmermann 2012-02-07 13:00:16 PST
This induces some flakiness, see bug 78021 - at least for DRT/Mac.
Comment 23 Nikolas Zimmermann 2012-02-08 02:25:31 PST
Comment on attachment 125601 [details]
Patch v7

Bug 78084 is resolved, outermost <svg> SVGLoad event fires as soon as HTML load event. Removed the special runSVGRepaintTest() it's no longer needed.
Comment 24 Nikolas Zimmermann 2012-02-08 03:50:38 PST
(In reply to comment #21)
> It made svg/zoom/page/zoom-foreignObject.svg crash with Qt5-WK1.
> svg/zoom/page/zoom-foreignObject.svg works fine if I run only this test,
> but crahes if I run all tests or svg/zoom/page tests.
This regression is fixed as well, bug 77995 is just landing.
Comment 25 Nikolas Zimmermann 2012-02-09 03:48:21 PST
Closing this bug, the remaining individual bugs, have to be fixed now.