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

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

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?

(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

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

(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?

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.

(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 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 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!

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

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e21db2c8dbca Make spacing, iteration composite and effect composite work if the caller is chrome. r=hiro

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

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

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