872619 - Stop using 'load' in many SVG reftests, and use MozReftestInvalidate instead, and flag the fallback setTimeouts

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

I started this patch aiming to flag the setTimeout calls that we have in reftests with a consistent trailing string so that they are easy to filter out of searches when looking for bad setTimeout calls. I've done that, and made them all use the same fallback timeout (4 seconds).

I've also converted a bunch of 'load' event listeners to 'MozReftestInvalidate' listeners. That may fix some random orange on the build machines.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 749943 [details] [diff] [review]
patch

First batch of comments:

>diff --git a/layout/reftests/svg/as-image/svg-image-visited-1.html b/layout/reftests/svg/as-image/svg-image-visited-1.html
>       function snapshot() {
>         document.documentElement.removeAttribute("class");
>       }
>+      function delay_snapshot() {
>+        setTimeout(snapshot, 100); // delay snapshot to allow time for
>+                                   // :visited style inside the image to
>+                                   // resolve (which it hopefully won't)
>+      }
>+      document.addEventListener("MozReftestInvalidate", delay_snapshot, false);
>+      setTimeout(snapshot, 4000); // fallback for running outside reftest

We don't actually need the "fallback for running outside reftest" line at all in this test. The only scripted thing here is the reftest-wait removal, which only matters inside the reftest harness.  So the fallback setTimeout serves no purpose here -- drop that line.

This applies to the next file, svg-image-visited-2.html, too.

>diff --git a/layout/reftests/svg/dynamic-clipPath-02.svg b/layout/reftests/svg/dynamic-clipPath-02.svg
>--- a/layout/reftests/svg/dynamic-clipPath-02.svg
>+++ b/layout/reftests/svg/dynamic-clipPath-02.svg
>@@ -19,17 +19,17 @@
>     function startTest() {
>       document.addEventListener("MozReftestInvalidate", doTest, false);
>       // in case we're not gecko
>-      setTimeout(doTest, 5000);
>+      setTimeout(doTest, 4000); // fallback for running outside reftest

Looks like you forgot to delete the "in case we're not gecko" comment here.

(Probably worth grepping this directory for "not gecko" after applying the patch, to see if there are any others).

>+++ b/layout/reftests/svg/dynamic-use-03.svg
>     document.addEventListener("MozReftestInvalidate", doTest, false);
>-    // in case we're not gecko
>-    setTimeout(doTest, 5000);
>+      tTimeout(doTest, 4000); // fallback for running outside reftest

Looks like you're missing the "se" of setTimeout there.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 749943 [details] [diff] [review]
patch

>diff --git a/layout/reftests/svg/smil/anim-filter-primitive-size-01.svg b/layout/reftests/svg/smil/anim-filter-primitive-size-01.svg
> window.addEventListener("MozReftestInvalidate", run, false);
> 
>-setTimeout(run, 3000); // for non-gecko
>+setTimeout(run, 4000); // fallback for running outside reftest

Optional: Might be worth deleting the blank between the MozReftestInvalidate and the setTimeout, to make this consistent with all your other tweaked tests (and to emphasize that those two lines are related)

Same goes for the next two tests (anim-filter-size-01.svg, anim-view-01.svg )

>diff --git a/layout/reftests/svg/smil/event/event-begin-timeevent-3.svg b/layout/reftests/svg/smil/event/event-begin-timeevent-3.svg
>--- a/layout/reftests/svg/smil/event/event-begin-timeevent-3.svg
>+++ b/layout/reftests/svg/smil/event/event-begin-timeevent-3.svg
>@@ -1,12 +1,11 @@
> <svg xmlns="http://www.w3.org/2000/svg"
>      xmlns:xlink="http://www.w3.org/1999/xlink"
>-     class="reftest-wait"
>-     onload="fastForwardToEvent()">
>+     class="reftest-wait">
[...]
>+    document.addEventListener("MozReftestInvalidate", fastForwardToEvent, false);
>+    setTimeout(fastForwardToEvent, 4000); // fallback for running outside reftest

For SMIL tests that have onload="somethingThatPausesTheTimeline()", we need to be very cautious about swapping onload for MozReftestInvalidate.

The SMIL timeline starts at SVGLoad time (which IIRC is the same as <svg onload>), but MozReftestInvalidate may be delayed by seconds or more, which could violate the tests's assumptions about when the scripted changes happen.

In this particular case, I could imagine something weird happening if MozReftestInvalidate were delayed by more than 5 seconds, since fastForwardToEvent implicitly assumes we aren't at that time yet.

SO: could you hold off on this particular file's changes? I'm open to the possibility that it might be a good idea (possibly with some test tweaks to ensure we don't call finish() twice), but it's more of a potentially behavior-affecting change than the rest of this patch, and I suspect it's more likely to introduce odd test-behavior than the rest of this patch, so it'd be worth landing separately IMHO.

(This is the only instance of this sort of thing that I noticed.)

r=me with the above addressed.

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b9d8277a30a

Thanks, dholbert!

Comment 5

[Jonathan Watt [:jwatt]]

longsonr, heycam: just a heads-up. Can you try and use the same:

setTimeout(doTest, 4000); // fallback for running outside reftest

line for any fallbacks you add to reftests so that it's easy to filter out the legitimate uses of setTimeout?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b9d8277a30a