1304805 - test_animation_observers.html | [set_spacing:subtree] records after animation is changed - number of records - got +0, expected 1

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

If mozilla-central gets run as beta in a simulation https://wiki.mozilla.org/Sheriffing/Uplift-Sim/Trunk-Beta , the following error occurs:

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

17:04:53     INFO -  1172 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_effect_with_previous_animation:subtree] records after animation effects are changed - record[1].removedAnimations length
17:04:53     INFO -  1173 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_effect_with_previous_animation:subtree] records after animation effects are changed - record[1].removedAnimations contains expected Animation
17:04:53     INFO -  1174 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is added - number of records
17:04:53     INFO -  1175 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is added - record[0].addedAnimations length
17:04:53     INFO -  1176 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is added - record[0].addedAnimations contains expected Animation
17:04:53     INFO -  1177 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is added - record[0].changedAnimations length
17:04:53     INFO -  1178 INFO TEST-PASS | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is added - record[0].removedAnimations length
17:04:53     INFO -  1179 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_observers.html | [set_spacing:subtree] records after animation is changed - number of records - got +0, expected 1
17:04:53     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:270:5
17:04:53     INFO -  is@chrome://mochitests/content/chrome/dom/animation/test/chrome/test_animation_observers.html:102:3
17:04:53     INFO -  assert_records@chrome://mochitests/content/chrome/dom/animation/test/chrome/test_animation_observers.html:163:3
17:04:53     INFO -  @chrome://mochitests/content/chrome/dom/animation/test/chrome/test_animation_observers.html:1938:3
17:04:53     INFO -  step@chrome://mochitests/content/chrome/dom/animation/test/chrome/test_animation_observers.html:61:16

The test/checkins from bug 1274944 likely assume that web animations are always enabled, but they are only on Aurora and Nightly https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2695

Comment 1

[Boris Chiou [:boris]]

OK. We should enable "dom.animations-api.core.enabled" in test_animation_observers.html.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I am wondering why there is no exception when we call anim.effect if the pref is disabled.
Also there are lots of test cases which touch effect.timing properties, how do those tests pass?

Comment 3

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I am wondering why there is no exception when we call anim.effect if the
> pref is disabled.
> Also there are lots of test cases which touch effect.timing properties, how
> do those tests pass?

I guess this test is called by chrome, and nsDocument::IsWebAnimationsEnabled() returns true if nsContentUtils::IsCallerChrome() is true [1], so anim.effect is ok. However, we only check "dom.animations-api.core.enabled" before paring spacing, so set_spacing is failed. Therefore, there are two ways to fix this bug:
1) Enable "dom.animations-api.core.enabled" in test_animation_observers.html.
2) Let AnimationUtils::IsCoreAPIEnabled() returns true if nsContentUtils::IsCallerChrome() is also true.

I prefer the 1st solution.

[1] http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/dom/base/nsDocument.cpp#3010-3017

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > I am wondering why there is no exception when we call anim.effect if the
> > pref is disabled.
> > Also there are lots of test cases which touch effect.timing properties, how
> > do those tests pass?
> 
> I guess this test is called by chrome, and
> nsDocument::IsWebAnimationsEnabled() returns true if
> nsContentUtils::IsCallerChrome() is true [1], so anim.effect is ok. However,
> we only check "dom.animations-api.core.enabled" before paring spacing, so
> set_spacing is failed.

Oh! I did not notice that.

> 1) Enable "dom.animations-api.core.enabled" in test_animation_observers.html.
> 2) Let AnimationUtils::IsCoreAPIEnabled() returns true if
> nsContentUtils::IsCallerChrome() is also true.
> 
> I prefer the 1st solution.

If we just enable it in test_animation_observers.html, will we do similar mistakes in other tests again?  I am more likely to do the mistake...

Comment 5

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> (In reply to Boris Chiou [:boris] from comment #3)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > I am wondering why there is no exception when we call anim.effect if the
> > > pref is disabled.
> > > Also there are lots of test cases which touch effect.timing properties, how
> > > do those tests pass?
> > 
> > I guess this test is called by chrome, and
> > nsDocument::IsWebAnimationsEnabled() returns true if
> > nsContentUtils::IsCallerChrome() is true [1], so anim.effect is ok. However,
> > we only check "dom.animations-api.core.enabled" before paring spacing, so
> > set_spacing is failed.
> 
> Oh! I did not notice that.
> 
> > 1) Enable "dom.animations-api.core.enabled" in test_animation_observers.html.
> > 2) Let AnimationUtils::IsCoreAPIEnabled() returns true if
> > nsContentUtils::IsCallerChrome() is also true.
> > 
> > I prefer the 1st solution.
> 
> If we just enable it in test_animation_observers.html, will we do similar
> mistakes in other tests again?  I am more likely to do the mistake...

Me, too, so we have to be carefully. This depends on whether we should enable spacing and iterationComposite for chrome, or only for this test. Hi heycam, do you have any suggestion for this?

Comment 6

[Cameron McCormack (:heycam)]

I think it would be OK to make IsCoreAPIEnabled() call IsCallerChrome(), just like nsDocument::Is(ElementAnimate|WebAnimations)Enabled do.  Maybe it can be renamed IsCoreAPIEnabledForCaller() or something like that, so it's clearer that it takes into account whether we're in chrome.

I'm not sure whether there's anything better we can do, and luckily we have the sheriffs performing try runs to catch these issues before the merge.  So I think it's OK just to fix this issue, and be mindful of them in the future.

Comment 7

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) from comment #6)
> I think it would be OK to make IsCoreAPIEnabled() call IsCallerChrome(),
> just like nsDocument::Is(ElementAnimate|WebAnimations)Enabled do.  Maybe it
> can be renamed IsCoreAPIEnabledForCaller() or something like that, so it's
> clearer that it takes into account whether we're in chrome.
> 
> I'm not sure whether there's anything better we can do, and luckily we have
> the sheriffs performing try runs to catch these issues before the merge.  So
> I think it's OK just to fix this issue, and be mindful of them in the future.

Thanks, Cameron. Let's call IsCallerChrome() in IsCoreAPIEnabledForCaller().

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1304805 - Make spacing, iteration composite and effect composite work if the caller is chrome.

Review commit: https://reviewboard.mozilla.org/r/80684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80684/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8794071 [details]
Bug 1304805 - Make spacing, iteration composite and effect composite work if the caller is chrome.

https://reviewboard.mozilla.org/r/80684/#review79340

Nice!

::: dom/animation/AnimationUtils.cpp:13
(Diff revision 1)
>  
>  #include "nsDebug.h"
>  #include "nsIAtom.h"
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
> +#include "nsContentUtils.h"

nit: I think this should be prior to nsDebug.h. Right?  Also please add a comment '// nsContentUtils::IsCallerChrome'.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8794071 [details]
Bug 1304805 - Make spacing, iteration composite and effect composite work if the caller is chrome.

https://reviewboard.mozilla.org/r/80684/#review79340

> nit: I think this should be prior to nsDebug.h. Right?  Also please add a comment '// nsContentUtils::IsCallerChrome'.

Yes. You're right. Thanks!

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8794071 [details]
Bug 1304805 - Make spacing, iteration composite and effect composite work if the caller is chrome.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80684/diff/1-2/

Comment 12

[Boris Chiou [:boris]]

After landing this into m-c, I will uplift it to aurora and beta.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e21db2c8dbca
Make spacing, iteration composite and effect composite work if the caller is chrome. r=hiro

Comment 14

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #12)
> After landing this into m-c, I will uplift it to aurora and beta.

Sorry, only need to uplift this to aurora.

Comment 15

[Iris Hsiao [:ihsiao]]

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

Comment 16

[Boris Chiou [:boris]]

Attachment: Make spacing, iteration composite and effect composite work if the caller is

chrome. (carry hiro's r+, uplift to aurora)

This patch is ready to uplift to aurora. Carry hiro's r+ from attachment 8794071 [details].

MozReview-Commit-ID: 30BOUA8QPh8

Comment 17

[Boris Chiou [:boris]]

Attachment: Uplift to aurora (carry hiro's r+)

This patch is ready to uplift to aurora. Carry hiro's r+ from attachment 8794071 [details].

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8795646 [details] [diff] [review]
Uplift to aurora (carry hiro's r+)

Approval Request Comment
[Feature/regressing bug #]: Bug 1274944
[User impact if declined]: We have to make sure all the tests of Web Animations API work in both release build and nightly build.
[Describe test coverage new/current, TreeHerder]: I didn't add any specific tests for this bug. Instead, this patch is trying to make all the tests in test_animation_observers.html happy.
[Risks and why]: No risk. Just make sure spacing works if it is called from chrome tests.
[String/UUID change made/needed]: No.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8795646 [details] [diff] [review]
Uplift to aurora (carry hiro's r+)

Fix an issue related to Web Animations API. Take it in 51 aurora.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e12aae05567