Convert 2 svg repaint tests to not use setTimeout
Created attachment 127062 [details] Patch
Similar to the changes in bug 78516. Chromium may need some new baselines, but I'm gardening so I can do that.
(In reply to comment #2) > Chromium may need some new baselines, but I'm gardening so I can do that. Actually, I tried it locally and it seems like this should pass Chromium with no new baselines.
(In reply to comment #3) > (In reply to comment #2) > > Chromium may need some new baselines, but I'm gardening so I can do that. > > Actually, I tried it locally and it seems like this should pass Chromium with no new baselines. Can you hold back this patch? I have a massive patch waiting for review, that switches all svg/custom tests to use the repaint.js harness.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Chromium may need some new baselines, but I'm gardening so I can do that. > > > > Actually, I tried it locally and it seems like this should pass Chromium with no new baselines. > Can you hold back this patch? I have a massive patch waiting for review, that switches all svg/custom tests to use the repaint.js harness. Ooops, the svg/custom changes are already in, sorry. Can you explain why they shouldn't use setTimeout? Any flakiness?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > Chromium may need some new baselines, but I'm gardening so I can do that. > > > > > > Actually, I tried it locally and it seems like this should pass Chromium with no new baselines. > > Can you hold back this patch? I have a massive patch waiting for review, that switches all svg/custom tests to use the repaint.js harness. > > Ooops, the svg/custom changes are already in, sorry. Can you explain why they shouldn't use setTimeout? Any flakiness? Sorry for not explaining. Yes, the Chromium bots were getting different results in debug and release because in debug, we would do an extra layout and paint. Removing the setTimeouts doesn't allow those extra layouts or paints to happen.
Comment on attachment 127062 [details] Patch Ah ok, thanks for the explanation. I'd r+ this, but following should be resolved first: I have some worries regarding the svg/repaint/image-with-clip-path.svg changes. Can you try to comment out the repaint() in RenderSVGImage::imageChanged in rendering/svg/RenderSVGImage.cpp, and then rerun all pixel tests. Check if the image-with-clip-path.svg is now broken, as expected - if that's the case I'm fine with your change. I remember the timeouts were added at some point to be sure we detect this repaint(). Not 100% sure, so you'd need to try first.
(In reply to comment #7) > (From update of attachment 127062 [details]) > Ah ok, thanks for the explanation. I'd r+ this, but following should be resolved first: > I have some worries regarding the svg/repaint/image-with-clip-path.svg changes. > Can you try to comment out the repaint() in RenderSVGImage::imageChanged in rendering/svg/RenderSVGImage.cpp, and then rerun all pixel tests. Check if the image-with-clip-path.svg is now broken, as expected - if that's the case I'm fine with your change. > > I remember the timeouts were added at some point to be sure we detect this repaint(). Not 100% sure, so you'd need to try first. I commented out the call to repaint() then ran the image-with-clip-path.svg test with my changes. It passed. I then reverted my changes to image-with-clip-path.svg (repaint() is still commented out) and ran the test again, and it passed again. Does something different happen on Apple Mac? I tested on Chromium Linux.
(In reply to comment #8) > I then reverted my changes to image-with-clip-path.svg (repaint() is still commented out) and ran the test again, and it passed again. > > Does something different happen on Apple Mac? I tested on Chromium Linux. Hrm, this is unfortunate. Is repainting throttled on Chromium?
(In reply to comment #9) > (In reply to comment #8) > > I then reverted my changes to image-with-clip-path.svg (repaint() is still commented out) and ran the test again, and it passed again. > > > > Does something different happen on Apple Mac? I tested on Chromium Linux. > Hrm, this is unfortunate. Is repainting throttled on Chromium? Yes, we coalesce repaints and generate the union of smaller repaint regions. I think it's like a 40ms window or something.
(In reply to comment #10) > Yes, we coalesce repaints and generate the union of smaller repaint regions. I think it's like a 40ms window or something. The test makes the assumption that this doesn't happen - we expect repaints to happen in between the setTimeout(, 0) timeouts. Hmpf, it's probably time to rename layoutTestController.display() to startTrackingRepaintRectsAndDisplay() and to make display() really only display the web view, so that we can force redraws for all platforms at specific points, to make tests like this reliable everywhere. What do you think?
(In reply to comment #11) > (In reply to comment #10) > > Yes, we coalesce repaints and generate the union of smaller repaint regions. I think it's like a 40ms window or something. > The test makes the assumption that this doesn't happen - we expect repaints to happen in between the setTimeout(, 0) timeouts. > Hmpf, it's probably time to rename layoutTestController.display() to startTrackingRepaintRectsAndDisplay() and to make display() really only display the web view, so that we can force redraws for all platforms at specific points, to make tests like this reliable everywhere. > > What do you think? I see. Maybe we could also add a method layoutTestController.layoutAndRepaint() and call that explicitly rather than using setTimeout? Renaming display() also sounds useful, but changing it's behavior sounds tricky (I'm not opposed, it just sounds like a lot of work).
(In reply to comment #12) > I see. Maybe we could also add a method layoutTestController.layoutAndRepaint() and call that explicitly rather than using setTimeout? That would avoid the "document.body.offsetTop" hack as well in repaint.js. If we'd use "layoutAndRepaint()" there instead of "document.body.offsetTop; layoutTestController.display()". Sounds good to me. > Renaming display() also sounds useful, but changing it's behavior sounds tricky (I'm not opposed, it just sounds like a lot of work). Well it's a global find . -type f | xargs perl -pi -e s/layoutTestController.display()/layoutTestController.startTrackingRepaintRectAndDisplay()/ :-) Let's hear Simons opinion and other rendering/testing webkittens.