Bug 77541 - SVG pixel tests can't catch regressions anymore
Summary: SVG pixel tests can't catch regressions anymore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Nobody
URL:
Keywords:
Depends on: 77736 78115 78219
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 03:50 PST by Nikolas Zimmermann
Modified: 2012-02-15 02:53 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-02-01 03:50:00 PST
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.
Comment 1 Nikolas Zimmermann 2012-02-03 03:37:23 PST
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.
Comment 2 James Robinson 2012-02-03 11:05:19 PST
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.
Comment 3 Nikolas Zimmermann 2012-02-03 13:32:57 PST
(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.
Comment 4 Simon Fraser (smfr) 2012-02-04 09:49:41 PST
Note that display() is very poorly named. It's really "start tracking repaint rects".
Comment 5 Nikolas Zimmermann 2012-02-05 17:05:16 PST
(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?
Comment 6 Simon Fraser (smfr) 2012-02-06 00:18:05 PST
(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.
Comment 7 Nikolas Zimmermann 2012-02-09 07:57:06 PST
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.
Comment 8 Nikolas Zimmermann 2012-02-15 02:53:35 PST
Closing the master bug, all SVG pixel tests are converted to the new repaint.js scheme, finally.