Bug 77736

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.

Created attachment 125327 [details] Patch v1

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

Created attachment 125340 [details] Patch v2 Ok, webkit-patch doesn't handle large files, trying git diff --binary per Ossys suggestion.

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

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

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

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

Created attachment 125529 [details] Patch v3

Created attachment 125530 [details] Patch v4

Created attachment 125536 [details] Patch v5

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.

Created attachment 125544 [details] Patch v6

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

Did you read https://bugs.webkit.org/show_bug.cgi?id=69459#c18 ?

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

Created attachment 125601 [details] Patch v7

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. :)

Committed r106918: <http://trac.webkit.org/changeset/106918>

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]

This induces some flakiness, see bug 78021 - at least for DRT/Mac.

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.

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

Closing this bug, the remaining individual bugs, have to be fixed now.