77541 2012-02-01 03:50:00 -0800 SVG pixel tests can't catch regressions anymore 2012-02-15 02:53:35 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) All All RESOLVED FIXED P1 Critical --- 77736 78115 78219 1 zimmermann webkit-unassigned eric.carlson jamesr simon.fraser zimmermann oldest_to_newest 546986 0 zimmermann 2012-02-01 03:50:00 -0800 From the recent webkit-dev discussion: the initial painting time changed for testing with DRT. Comment out eg. the repaint() in RenderSVGImage::imageChanged(), and see that all pixel tests still pass with tolerance 0 on a vanilla Lion iMac (like I own). Running eg svg/repaint/image-href-change.svg in Safari, it's clearly not repainting anymore - you have to zoom in/out to see the href change. See the thread at https://lists.webkit.org/pipermail/webkit-dev/2012-January/019265.html. 548616 1 zimmermann 2012-02-03 03:37:23 -0800 I went back until Oct 15, where its still broken. Can't go back much further, as Safari depends on a symbol thats not present in older WebKit2 libraries. Anyhow, I've compared DRT, the WebKit2-testrunner to Safari, and can only reproduce the "old behavior" with crude hacks forcing repaints inside DRT. Still this is not reliable. I've decided to stop wasting my time and instead fix the SVG tests. I'll grep for all occurrences of setTimeout and/or layoutTestController.waitUntilDone(), and will have to fix all testcases that don't use dumpAsText, to correctly enforce repainting through layoutTestController.display() and forcing a layout before, by eg. document.rootElement.offsetLeft. As I initially said, I share James opinion that the tests have to be fixed, but I was confident it was a recent change that changed the timing. What recently changed on my side is that I got a new fast machine now. It turns out the repainting trouble I ran into happens _much_ less frequently on my older MBP from 2006 that I used as main machine until December. Now with my shiny iMac, it's easy to see that there are obvious races between dumping & painting, that I won't be able to fix using any setTimeout tricks. /me goes fixing the SVG tests once for all. 548864 2 jamesr 2012-02-03 11:05:19 -0800 Could you describe what exactly you are trying to test with these? The pixel tests I'm familiar with in WebKit do one of two things: 1.) Test that we render a given document correctly. This is done by setting up the document and then letting the test end. The -expected.png is just the final rendering of the page 2.) Test that after rendering a given document A, we correctly repaint + render the document B which is A with some mutations applied. This is done by setting up document A, calling layoutTestController.display(), then mutating the document to state B and letting the test end. 549014 3 zimmermann 2012-02-03 13:32:57 -0800 (In reply to comment #2) > Could you describe what exactly you are trying to test with these? The pixel tests I'm familiar with in WebKit do one of two things: > > 1.) Test that we render a given document correctly. This is done by setting up the document and then letting the test end. The -expected.png is just the final rendering of the page > > 2.) Test that after rendering a given document A, we correctly repaint + render the document B which is A with some mutations applied. This is done by setting up document A, calling layoutTestController.display(), then mutating the document to state B and letting the test end. It's all about (2). I'm fixing the first set of svg tests in bug 77736. We tried to fake (2) by using setTimeout(runTest, 0) hoping that painting would happen in-between. I changed that to call layoutTestController.display() to guarantee we're painting. 549439 4 simon.fraser 2012-02-04 09:49:41 -0800 Note that display() is very poorly named. It's really "start tracking repaint rects". 549642 5 zimmermann 2012-02-05 17:05:16 -0800 (In reply to comment #4) > Note that display() is very poorly named. It's really "start tracking repaint rects". Yes, but it also explicitly calls [WebView display] for Mac DRT, thus forcing a paint, right? 549784 6 simon.fraser 2012-02-06 00:18:05 -0800 (In reply to comment #5) > (In reply to comment #4) > > Note that display() is very poorly named. It's really "start tracking repaint rects". > Yes, but it also explicitly calls [WebView display] for Mac DRT, thus forcing a paint, right? Yes. It also causes DRT/WTR to paint a gray overlay with holes for the repaint rects. 553173 7 zimmermann 2012-02-09 07:57:06 -0800 Phew, what a boring week, bug 78219 has the last patch, fixing svg/dynamic-updates -- once that landed, all tests that don't use dumpAsText() are converted to the repaint.js harness, giving us reliable pixel results. The SVGLoad event timing is also fixed in trunk - the vanilla repaint.js can be used together with SVG files like <svg onload="runRepaintTest()" w/o problems. Once bug 78219, we can close this bug. 557091 8 zimmermann 2012-02-15 02:53:35 -0800 Closing the master bug, all SVG pixel tests are converted to the new repaint.js scheme, finally.