1219872 - Animation inspector tests are permafailing after merge to beta

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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?

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Phil Ringnalda (:philor)]

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2458

Comment 4

[Brian Grinstead [:bgrins]]

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.

Comment 5

[Brian Grinstead [:bgrins]]

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.

Comment 6

[Phil Ringnalda (:philor)]

That's not your test, that's a web-platform-tests tests.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: animation-permaorange.patch

This should fix it, we will see what try says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=928c524fa83e

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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).

Comment 9

[Brian Grinstead [:bgrins]]

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

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

Does this look right?

Comment 10

[Brian Grinstead [:bgrins]]

(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

Comment 11

[Brian Grinstead [:bgrins]]

(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

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.)

Comment 13

[Brian Grinstead [:bgrins]]

(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

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: animation-permaorange-for-aurora-and-central.patch

patch for aurora / central (paths changed in 44)

Comment 15

[Brian Grinstead [:bgrins]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c59c4d8fd67d

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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?

Comment 18

[Brian Grinstead [:bgrins]]

(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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c59c4d8fd67d

Comment 20

[Patrick Brosset <:pbro>]

Thanks for figuring that out Brian and Brian (while I was on PTO).

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c59c4d8fd67d

Comment 22

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion