Animation inspector tests are permafailing after merge to beta

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Animation Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox42 unaffected, firefox43 fixed, firefox44 fixed, firefox45 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=dfd7ec48d4fe

Errors like: INFO TEST-UNEXPECTED-FAIL | browser/devtools/animationinspector/test/browser_animation_timeline_takes_rate_into_account.js | uncaught exception - TypeError: el.getAnimations is not a function at http://example.com/browser/browser/devtools/animationinspector/test/doc_modify_playbackRate.html:26
(Assignee)

Comment 1

2 years ago
This makes it appear that platform features may not be available.  Patrick, is there an easy 'off switch' for the animation inspector that we can flip until such functions are available?
Flags: needinfo?(pbrosset)
(Assignee)

Comment 2

2 years ago
Looks like there is a check to this.target.form.animationsActor before adding the sidebar here, so that could maybe be extended with a pref check to prevent breakage in the toolbox: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#356.

Brian, is there a pref / build config we can check to see if animation platform features are enabled for a build?  My suspicion is that the panel is broken because certain features aren't available in release (beta) builds.
Flags: needinfo?(bbirtles)
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2458
(Assignee)

Comment 4

2 years ago
The weird thing is that the animation inspector seems to work for me in a local beta build at http://bgrins.github.io/devtools-demos/inspector/animation-timing.html.  But getAnimations() is used in the animation actor so I don't understand why it works.

Test definitely fails locally though - we should skip it on RELEASE_BUILD with a note that we will want to re-enable when Web Animations API is enabled.

I also wonder if we should hide the animations panel altogether or account for a case where the animations API is disabled, but I'll leave that up to Patrick.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 5

2 years ago
Oh, looks like it was fixed by setting the animations API pref just for this test: https://hg.mozilla.org/releases/mozilla-beta/rev/d11e8e2e9425.  I guess we can leave this bug open so we can decide what to do with the panel in that case.
That's not your test, that's a web-platform-tests tests.
(Assignee)

Comment 7

2 years ago
Created attachment 8680884 [details] [diff] [review]
animation-permaorange.patch

This should fix it, we will see what try says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=928c524fa83e
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Brian, is there a pref / build config we can check to see if animation
> platform features are enabled for a build?  My suspicion is that the panel
> is broken because certain features aren't available in release (beta) builds.

If you're running chrome code, then all the Web Animations API features should be available regardless of the settings of any prefs. If they're not, that's a bug we need to fix.

For tests running without chrome privileges, the pref is dom.animations-api.core.enabled (currently set to false on release builds).
(Assignee)

Comment 9

2 years ago
Comment on attachment 8680884 [details] [diff] [review]
animation-permaorange.patch

Review of attachment 8680884 [details] [diff] [review]:
-----------------------------------------------------------------

Does this look right?
Attachment #8680884 - Flags: review?(bbirtles)
(Assignee)

Comment 10

2 years ago
(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Brian, is there a pref / build config we can check to see if animation
> > platform features are enabled for a build?  My suspicion is that the panel
> > is broken because certain features aren't available in release (beta) builds.
> 
> If you're running chrome code, then all the Web Animations API features
> should be available regardless of the settings of any prefs. If they're not,
> that's a bug we need to fix.
> 
> For tests running without chrome privileges, the pref is
> dom.animations-api.core.enabled (currently set to false on release builds).

Yeah in this case it looks like the test is running on http://example.com/ so no chrome privileges
(Assignee)

Comment 11

2 years ago
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Created attachment 8680884 [details] [diff] [review]
> animation-permaorange.patch
> 
> This should fix it, we will see what try says:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=928c524fa83e

Looks like this fixed it on beta
(In reply to Brian Grinstead [:bgrins] from comment #10)
> Yeah in this case it looks like the test is running on http://example.com/
> so no chrome privileges

How does that work? i.e. Where/how in the test is that page loaded?

The reason I'm asking is because if we're adding interfaces to the global object, we need to do it before loading the page. (That's why we have this awkward setup in dom/animation/tests where we have a test_* file that sets the pref, then loads the actual file_* test in a separate window.)
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 13

2 years ago
(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > Yeah in this case it looks like the test is running on http://example.com/
> > so no chrome privileges
> 
> How does that work? i.e. Where/how in the test is that page loaded?
> 
> The reason I'm asking is because if we're adding interfaces to the global
> object, we need to do it before loading the page. (That's why we have this
> awkward setup in dom/animation/tests where we have a test_* file that sets
> the pref, then loads the actual file_* test in a separate window.)

The tab is added on the line immediately after the call to pushPrefEnv in attachment 8680884 [details] [diff] [review]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/test/browser_animation_timeline_takes_rate_into_account.js#15

So we set the pref first, then add the tab
Flags: needinfo?(bgrinstead)
Attachment #8680884 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8680943 [details] [diff] [review]
animation-permaorange-for-aurora-and-central.patch

patch for aurora / central (paths changed in 44)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
status-firefox42: --- → unaffected
status-firefox43: --- → affected
status-firefox44: --- → affected
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/065abb91e10a

Comment 16

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c59c4d8fd67d
https://hg.mozilla.org/integration/fx-team/rev/c59c4d8fd67d
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1b46edc0615

Does this patch fix things (can I set the statuses to 'fixed') or is further work needed?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 18

2 years ago
(In reply to Wes Kocher (:KWierso) from comment #17)
> https://hg.mozilla.org/integration/fx-team/rev/c59c4d8fd67d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a1b46edc0615
> 
> Does this patch fix things (can I set the statuses to 'fixed') or is further
> work needed?

I believe it fixes things as per Comment 8 and since the rest of the animation inspector tests are passing
Flags: needinfo?(bgrinstead)

Updated

2 years ago
status-firefox43: affected → fixed
status-firefox44: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/c59c4d8fd67d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Thanks for figuring that out Brian and Brian (while I was on PTO).
Flags: needinfo?(pbrosset)

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c59c4d8fd67d
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: --- → ---
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
You need to log in before you can comment on or make changes to this bug.