474669 - Improve svg invalidation reftests

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: Sample updated reftest

Can use MozReftestInvalidate now that we have it.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    setTimeout(finishTest, 200);

This is good, but when we do this, let's make this timeout really huge, like 5000ms, to make sure on non-Gecko platforms on slow hardware the test is actually tested reliably.

Are you planning to fix all the tests?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, this has the same problem as the other patch --- MozReftestInvalidate should trigger move_foreignObject, which should immediately remove reftest-wait.

Basically, you need to wait for MozReftestInvalidate to fire before you make the change that triggers invalidation. But after you've made the change, you can immediately remove reftest-wait and the reftest harness is responsible for making sure it takes the snapshot at the right time.

Comment 3

[Robert Longson [:longsonr]]

Attachment: like so?

Not sure about doing all of them. I just wanted to get one right to start.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 358064 [details] [diff] [review]
like so?

Looks good.

But could you do me a favour and check two things?
1) that if you deliberately break this test, say by reverting the fix that we added this test for, the test actually fails reliably (say you run it 5 times and it fails every time)
2) on trunk, the test succeeds reliably (run it 5 times and it passes every time)

We should only need to do that for this test. I've already checked this on other code but I would like to double-check :-)

Comment 5

[Robert Longson [:longsonr]]

Attachment: complete patch

Comment 6

[Robert Longson [:longsonr]]

(In reply to comment #4)
> But could you do me a favour and check two things?
> 1) that if you deliberately break this test, say by reverting the fix that we
> added this test for, the test actually fails reliably (say you run it 5 times
> and it fails every time)

Can't quite revert the patch as too much time has passeed but I took out the UpdateGraphic calls from nsSVGForeignObjectFrame::AttributeChanged which would mean that a change in the "x" attribute would be ignored and sure enough the test failed.

> 2) on trunk, the test succeeds reliably (run it 5 times and it passes every
> time)

Done with all tests in the final patch.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great, thanks!

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/8425b9c675bd

Thanks!