Improve svg invalidation reftests

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 358016 [details] [diff] [review]
Sample updated reftest

Can use MozReftestInvalidate now that we have it.
Attachment #358016 - Flags: review?(roc)
+    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?
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.
(Assignee)

Comment 3

9 years ago
Created attachment 358064 [details] [diff] [review]
like so?

Not sure about doing all of them. I just wanted to get one right to start.
Attachment #358016 - Attachment is obsolete: true
Attachment #358064 - Flags: review?(roc)
Attachment #358016 - Flags: review?(roc)
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 :-)
(Assignee)

Comment 5

9 years ago
Created attachment 358176 [details] [diff] [review]
complete patch
Assignee: nobody → longsonr
Attachment #358064 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
(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.
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/8425b9c675bd

Thanks!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.