77736 2012-02-03 06:58:24 -0800 Rewrite SVG tests to make extensive use of display() in repaint tests 2012-02-09 03:48:21 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 77183 77995 78026 69459 77541 78021 78084 78115 1 zimmermann zimmermann dglazkov jamesr krit ossy simon.fraser tonyg webkit.review.bot zimmermann oldest_to_newest 548683 0 zimmermann 2012-02-03 06:58:24 -0800 This bug covers all tests in svg/ except for the dynamic-updates/ and custom/ folders -- 140 tests remaining to examine. 548709 1 zimmermann 2012-02-03 07:44:23 -0800 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. 548712 2 125327 zimmermann 2012-02-03 07:53:18 -0800 Created attachment 125327 Patch v1 548721 3 zimmermann 2012-02-03 08:06:30 -0800 *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. 548753 4 125340 zimmermann 2012-02-03 08:39:10 -0800 Created attachment 125340 Patch v2 Ok, webkit-patch doesn't handle large files, trying git diff --binary per Ossys suggestion. 548815 5 125340 krit 2012-02-03 10:07:58 -0800 Comment on attachment 125340 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). 548869 6 jamesr 2012-02-03 11:07:50 -0800 (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 ? 549019 7 zimmermann 2012-02-03 13:34:50 -0800 (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). 549027 8 jamesr 2012-02-03 13:41:25 -0800 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 549593 9 125529 zimmermann 2012-02-05 09:06:05 -0800 Created attachment 125529 Patch v3 549594 10 125530 zimmermann 2012-02-05 09:23:20 -0800 Created attachment 125530 Patch v4 549620 11 125536 zimmermann 2012-02-05 14:53:52 -0800 Created attachment 125536 Patch v5 549638 12 zimmermann 2012-02-05 16:53:27 -0800 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. 549640 13 125544 zimmermann 2012-02-05 16:58:10 -0800 Created attachment 125544 Patch v6 549675 14 125544 webkit.review.bot 2012-02-05 19:09:50 -0800 Comment on attachment 125544 Patch v6 Attachment 125544 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 549681 15 125544 webkit.review.bot 2012-02-05 19:47:23 -0800 Comment on attachment 125544 Patch v6 Attachment 125544 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 549712 16 krit 2012-02-05 21:23:04 -0800 Did you read https://bugs.webkit.org/show_bug.cgi?id=69459#c18 ? 549849 17 zimmermann 2012-02-06 02:14:36 -0800 (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. 549857 18 125601 zimmermann 2012-02-06 02:29:55 -0800 Created attachment 125601 Patch v7 550756 19 125601 kling 2012-02-07 02:42:08 -0800 Comment on attachment 125601 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. :) 550773 20 zimmermann 2012-02-07 03:20:03 -0800 Committed r106918: <http://trac.webkit.org/changeset/106918> 551014 21 ossy 2012-02-07 08:33:50 -0800 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] 551265 22 zimmermann 2012-02-07 13:00:16 -0800 This induces some flakiness, see bug 78021 - at least for DRT/Mac. 551929 23 125601 zimmermann 2012-02-08 02:25:31 -0800 Comment on attachment 125601 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. 551984 24 zimmermann 2012-02-08 03:50:38 -0800 (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. 553031 25 zimmermann 2012-02-09 03:48:21 -0800 Closing this bug, the remaining individual bugs, have to be fixed now. 125327 2012-02-03 07:53:18 -0800 2012-02-06 02:22:32 -0800 Patch v1 FixFailures.diff text/plain 0 zimmermann 125340 2012-02-03 08:39:10 -0800 2012-02-05 09:04:52 -0800 Patch v2 FixFailures.diff text/plain 0 zimmermann 125529 2012-02-05 09:06:05 -0800 2012-02-06 02:22:38 -0800 Patch v3 bug-77736-20120205180520.patch text/plain 0 zimmermann 125530 2012-02-05 09:23:20 -0800 2012-02-05 14:52:06 -0800 Patch v4 bug-77736-20120205182236.patch text/plain 0 zimmermann 125536 2012-02-05 14:53:52 -0800 2012-02-05 16:56:12 -0800 Patch v5 bug-77736-20120205235307.patch text/plain 0 zimmermann 125544 2012-02-05 16:58:10 -0800 2012-02-06 02:27:48 -0800 Patch v6 bug-77736-20120206015726.patch text/plain 0 zimmermann 125601 2012-02-06 02:29:55 -0800 2012-02-08 02:25:31 -0800 Patch v7 bug-77736-20120206112905.patch text/plain 0 zimmermann