Closed Bug 1219872 Opened 4 years ago Closed 4 years ago

Animation inspector tests are permafailing after merge to beta

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

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

RESOLVED FIXED
Firefox 45
Tracking Status
firefox42 --- unaffected
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

Details

Attachments

(2 files)

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
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)
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)
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)
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.
(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 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)
(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
(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)
(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+
patch for aurora / central (paths changed in 44)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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)
(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)
https://hg.mozilla.org/mozilla-central/rev/c59c4d8fd67d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Thanks for figuring that out Brian and Brian (while I was on PTO).
Flags: needinfo?(pbrosset)
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.