Closed Bug 1264101 Opened 8 years ago Closed 8 years ago

Various animation inspector tests are going to permafail when Gecko 48 merges to Beta (ReferenceError: KeyframeEffect is not defined)

Categories

(DevTools :: Inspector: Animations, defect, P1)

defect

Tracking

(firefox46 unaffected, firefox47+ fixed, firefox48+ fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 + fixed

People

(Reporter: RyanVM, Assigned: jdescottes)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1261651 +++

[Tracking Requested - why for this release]: Devtools permafail when Gecko 48 merges to Aurora.

Because the fun never stops around here :-)

https://treeherder.mozilla.org/logviewer.html#?job_id=19232205&repo=try

00:03:15     INFO -  14 INFO TEST-START | devtools/client/animationinspector/test/browser_animation_click_selects_animation.js
00:03:15     INFO -  TEST-INFO | started process screentopng
00:03:17     INFO -  TEST-INFO | screentopng: exit 0
00:03:17     INFO -  15 INFO checking window state
00:03:17     INFO -  16 INFO Entering test bound
00:03:17     INFO -  17 INFO Adding a new tab with URL: http://example.com/browser/devtools/client/animationinspector/test/doc_simple_animation.html
00:03:17     INFO -  18 INFO Waiting for event: 'load' on [object XULElement].
00:03:17     INFO -  19 INFO TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_click_selects_animation.js | uncaught exception - ReferenceError: KeyframeEffect is not defined at @http://example.com/browser/devtools/client/animationinspector/test/doc_simple_animation.html:121:9
00:03:17     INFO -  Stack trace:
00:03:17     INFO -  chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1563
00:03:17     INFO -  JavaScript error: http://example.com/browser/devtools/client/animationinspector/test/doc_simple_animation.html, line 121: ReferenceError: KeyframeEffect is not defined
00:03:17     INFO -  20 INFO Console message: [JavaScript Error: "ReferenceError: KeyframeEffect is not defined" {file: "http://example.com/browser/devtools/client/animationinspector/test/doc_simple_animation.html" line: 121}]
00:03:17     INFO -  @http://example.com/browser/devtools/client/animationinspector/test/doc_simple_animation.html:121:9

etc.
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(jdescottes)
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
Flags: needinfo?(jdescottes)
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 :)
https://hg.mozilla.org/mozilla-central/rev/898a9b29cadb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.