Closed Bug 1264101 Opened 5 years ago Closed 5 years ago
Various animation inspector tests are going to permafail when Gecko 48 merges to Beta (Reference
Error: Keyframe Effect is not defined)
MozReview Request: Bug 1264101 - force dom.animations-api.core.enabled pref to true before animation inspector tests;r=pbro
58 bytes, text/x-review-board-request
Whoops, s/Aurora/Beta. Not quite as urgent as originally hyped to be :)
Severity: critical → major
Summary: Various animation inspector tests are going to permafail when Gecko 48 merges to Aurora (ReferenceError: KeyframeEffect is not defined) → Various animation inspector tests are going to permafail when Gecko 48 merges to Beta (ReferenceError: KeyframeEffect is not defined)
Similar to bug 1261651, the WebAnimations API isn't enabled fully in various releases. Pinging Brian so he can explain what's the way forward here.
Flags: needinfo?(pbrosset) → needinfo?(bbirtles)
For Chrome tests you don't need to do anything. For content tests you need to make sure the dom.animations-api.core.enabled pref is set. If you're using a test framework that doesn't let you set prefs before running the test, you might need to set the pref in an outer doc, then load the test in an iframe to make sure the necessary globals are available. e.g. https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_animations_pausing.html
Thanks Brian. I was under the impression that the pref was only required to use el.animate, and I specifically used the various animation constructors to avoid using el.animate in those tests. Anyway, I'll fix this.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(In reply to Patrick Brosset [:pbro] from comment #4) > Thanks Brian. I was under the impression that the pref was only required to > use el.animate, and I specifically used the various animation constructors > to avoid using el.animate in those tests. > Anyway, I'll fix this. No, it's just that we explicitly disabled el.animate on Fx 47 due to compat issues that are fixed in Fx 48. That said, we are going to introduce a separate pref for el.animate (dom.animations-api.element-animate.enabled) in bug 1245000 but DevTools shouldn't be affected by that. As long as you set dom.animations-api.core.enabled for all content tests you get the whole API.
As discussed, taking the bug.
Assignee: pbrosset → jdescottes
WebAnimations API is not enabled by default in all release channels yet. It is behing the pref dom.animations-api.core.enabled. Turn the preference on before any test of the animationinspector, and clear it after the test. Review commit: https://reviewboard.mozilla.org/r/46711/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46711/
Attachment #8741730 - Flags: review?(pbrosset)
Attachment #8741730 - Flags: review?(pbrosset) → review+
Comment on attachment 8741730 [details] MozReview Request: Bug 1264101 - force dom.animations-api.core.enabled pref to true before animation inspector tests;r=pbro https://reviewboard.mozilla.org/r/46711/#review43315 Thanks a lot!
Is there any reason you used setBoolPref instead of pushPrefEnv? My understanding is that the latter is desirable these days.
Comment on attachment 8741730 [details] MozReview Request: Bug 1264101 - force dom.animations-api.core.enabled pref to true before animation inspector tests;r=pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46711/diff/1-2/
I actually didn't know about pushPrefEnv! I guess I must have missed an email thread about it. Julian, I'm going on PTO later today and as a result I disabled reviews on my bugzilla account. If you wanted to ping me for review for this one, consider this comment a R+.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9) > Is there any reason you used setBoolPref instead of pushPrefEnv? My > understanding is that the latter is desirable these days. No particular reason! I've seen both and didn't know pushPrefEnv was the preferred option. So thanks for the info. I modified the implementation based on this. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=359430035ddc
pushPrefEnv is less-prone to racy pref setting and cleans up after itself at the end of the test, so it's a bit simpler to use and more reliable :)
You need to log in before you can comment on or make changes to this bug.