Closed Bug 1471814 Opened 6 years ago Closed 6 years ago

Add more fine-grained prefs to the Web Animations API

Categories

(Core :: DOM: Animation, enhancement, P3)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Bug 1471814 - Add a preference for Web Animations timelines;
59 bytes, text/x-review-board-request
bzbarsky
: review+
hiro
: review+
Details
Bug 1471814 - Add a preference for implicit keyframes
59 bytes, text/x-review-board-request
bzbarsky
: review+
hiro
: review+
Details
Bug 1471814 - Add a preference for animation composite modes;
59 bytes, text/x-review-board-request
bzbarsky
: review+
hiro
: review+
Details
Bug 1471814 - Ignore the composite member on Keyframe objects when the compositing pref is not set;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1471814 - Add a preference for {Document,Element}.getAnimations();
59 bytes, text/x-review-board-request
bzbarsky
: review+
hiro
: review+
Details 
My plan here is to:

* Add dom.animations-api.timelines.enabled to control:

  Animation.timeline
  DocumentTimeline
  AnimationTimeline

  i.e. enabling dom.animations-api.core.enabled does NOT enable timelines.

  Preffed on in Nightly only.

* Add dom.animations-api.compositing.enabled to control:

  iterationComposite
  composite

  Likewise, enabling dom.animations-api.core.enabled does NOT enable compositing.

  Preffed on in Nightly only.

* Add dom.animations-api.get-animations.enabled to control:

  Element.getAnimations()
  Document.getAnimations()
  CSSPseudoElement

  Likewise, enabling dom.animations-api.core.enabled does NOT enable getAnimations.

  Preffed on in Nightly only.

* Send intent to ship where we turn on dom.animations-api.core.enabled in release.

* After dom.animations-api.core.enabled has hit the release channel, drop the
  dom.animations-api.element-animate.enabled pref.
Priority: -- → P3
I started looking into the composite modes pref and I noticed we're accidentally shipping composite modes. -.-;

Test case:

  https://codepen.io/anon/pen/xJxwga

We take care to ignore 'composite' and 'iterationComposite' when passed-in via keyframe options but not when set on the keyframes themselves.

I'm currently wondering if we want a separate pref for implicit keyframes. It's annoying to have to maintain the code for turning that off but it's probably safest to have a pref for it since we plan to ship that.

I also noticed that we can probably remove a bit of templating in KeyframeEffect.cpp now that we only have one type of KeyframeEffect---that can be separate bug however.
Depends on: 1475162
Try with all the prefs on (as per Nightly):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f219786712f3c78ac202bbf288e5058e3c5b8fd0

Try with all the prefs off (to test we are actually turning on what we need per test):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=247f0a1f19662c59aaa1cf59e700c391f95185b2

Some failures from the second run are expected (e.g. test_interfaces.js).

Also, for crashtests if we fail to set the necessary prefs they won't fail, they'll simply not test what they are intended to. For that reason I've checked them pretty carefully. Also, all these prefs are on in Nightly/early beta so even if I've made a mistake with the prefs for certain crashtests, at least we'll have Nightly/early beta coverage.

Once those try jobs look good (I've almost certainly made a mistake somewhere), I'll put these patches up for review and then send out an intent to implement next week for turning dom.animations-api.core.enabled and dom.animations-api.implicit-keyframes.enabled on by default.

(And having written all that I now notice that I forgot to put CSSPseudoElement behind the getAnimations() pref. So I guess these patches still need more work.)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment on attachment 8992151 [details]
Bug 1471814 - Add a preference for Web Animations timelines;

https://reviewboard.mozilla.org/r/257052/#review263880

I think we should file a bug for supporting preference settings in [DEFAULT] section in crashtest.list or refrest.list as well as we can do in mochitest.ini.
Attachment #8992151 - Flags: review?(hikezoe) → review+
Comment on attachment 8992152 [details]
Bug 1471814 - Add a preference for implicit keyframes

https://reviewboard.mozilla.org/r/257054/#review263882

::: layout/style/crashtests/crashtests.list:179
(Diff revision 1)
>  HTTP load 1320423-1.html
>  load 1321357-1.html
>  load 1328535-1.html
>  load 1331272.html
>  HTTP load 1333001-1.html
> -pref(dom.animations-api.core.enabled,true) load 1340344.html
> +pref(dom.animations-api.core.enabled,true) pref(dom.animations-api.implicit-keyframes.enabled,true) load 1340344.html

I don't think this test needs dom.animations-api.core.enabled, there is just a;

 document.documentElement.animate([{ "color": "hsla(6e147grad,16%,183.379675555%,0.0210463770007)" }])

::: layout/style/crashtests/crashtests.list:229
(Diff revision 1)
>  load 1390726.html
>  load 1393791.html
>  load 1384232.html
>  load 1395725.html
>  load 1396041.html
> -load 1397363-1.html
> +pref(dom.animations-api.implicit-keyframes.enabled,true) load 1397363-1.html

Huh?  This test has been working without any problems?  This test uses iterationComposite: "accumulate" so I think we need dom.animations-api.core.enabled, I don't know why it's not there.
Attachment #8992152 - Flags: review?(hikezoe) → review+
Comment on attachment 8992152 [details]
Bug 1471814 - Add a preference for implicit keyframes

https://reviewboard.mozilla.org/r/257054/#review263882

> Huh?  This test has been working without any problems?  This test uses iterationComposite: "accumulate" so I think we need dom.animations-api.core.enabled, I don't know why it's not there.

OK, now I see the reason while reading the commit message in the next patch. :)
Comment on attachment 8992153 [details]
Bug 1471814 - Add a preference for animation composite modes;

https://reviewboard.mozilla.org/r/257056/#review263886

::: dom/animation/test/mozilla/test_disable_animations_api_compositing.html:11
(Diff revision 1)
>  <script>
>  'use strict';
>  setup({explicit_done: true});
>  SpecialPowers.pushPrefEnv(
> -  { "set": [["dom.animations-api.core.enabled", false]]},
> +  { "set": [["dom.animations-api.compositing.enabled", false]]},
>    function() {

I didn't notice in the previous patches though, we might prefer arrow functions in these days.  It's up to you though.
Attachment #8992153 - Flags: review?(hikezoe) → review+
Comment on attachment 8992154 [details]
Bug 1471814 - Ignore the composite member on Keyframe objects when the compositing pref is not set;

https://reviewboard.mozilla.org/r/257058/#review263890
Attachment #8992154 - Flags: review?(hikezoe) → review+
Comment on attachment 8992155 [details]
Bug 1471814 - Add a preference for {Document,Element}.getAnimations();

https://reviewboard.mozilla.org/r/257060/#review263892

::: layout/style/crashtests/crashtests.list:162
(Diff revision 1)
>  # layout.css.prefixes.webkit pref
>  pref(layout.css.prefixes.webkit,false) load 1265611-1.html
>  load 1270795.html
>  load 1275026.html
>  load 1278463-1.html
> -pref(dom.animations-api.core.enabled,true) load 1277908-1.html # bug 1323652
> +pref(dom.animations-api.getAnimations.enabled,true) load 1277908-1.html # bug 1323652

I think the comment (# bug 1323652) is no longer valid.
Attachment #8992155 - Flags: review?(hikezoe) → review+
Comment on attachment 8992151 [details]
Bug 1471814 - Add a preference for Web Animations timelines;

https://reviewboard.mozilla.org/r/257052/#review264154

::: commit-message-b7945:7
(Diff revision 1)
> +
> +We don't intend to ship this in the near future until the integration with
> +AnimationWorklet is clear (although we might ship a read-only version).
> +
> +That said, we use this feature extensively internally (e.g. in DevTools etc.) so
> +this we enable this feature for Chrome callers.

s/this //

::: dom/base/nsContentUtils.cpp:706
(Diff revision 1)
>                                 "ui.use_activity_cursor", false);
>  
>    Preferences::AddBoolVarCache(&sAnimationsAPICoreEnabled,
>                                 "dom.animations-api.core.enabled", false);
>  
> +  Preferences::AddBoolVarCache(&sAnimationsAPITimelinesEnabled,

Please use staticprefs here; we should not be adding new bool var caches.
Attachment #8992151 - Flags: review?(bzbarsky) → review+
Comment on attachment 8992151 [details]
Bug 1471814 - Add a preference for Web Animations timelines;

https://reviewboard.mozilla.org/r/257052/#review264160

::: commit-message-b7945:7
(Diff revision 1)
> +
> +We don't intend to ship this in the near future until the integration with
> +AnimationWorklet is clear (although we might ship a read-only version).
> +
> +That said, we use this feature extensively internally (e.g. in DevTools etc.) so
> +this we enable this feature for Chrome callers.

s/Chrome callers/system callers/
Comment on attachment 8992152 [details]
Bug 1471814 - Add a preference for implicit keyframes

https://reviewboard.mozilla.org/r/257054/#review264158

::: commit-message-50c34:3
(Diff revision 1)
> +Bug 1471814 - Add a preference for implicit keyframes; r?hiro r?bz
> +
> +This preference controls whether or not authors are allowed to specify

s/or not //

::: commit-message-50c34:10
(Diff revision 1)
> +
> +We intend to ship this soon but this preference acts as a safeguard in case we
> +discover we need to disable it.
> +
> +This feature is very convenient and commonly used so this patch ensures it is
> +always enabled for Chrome content.

s/Chrome content/system code/

::: dom/base/nsContentUtils.h:2353
(Diff revision 1)
>    {
>      return sAnimationsAPICoreEnabled;
>    }
>  
>    /**
> +   * Returns true if the animations without explicit 0%/100% keyframes should

s/the animations/animations/

::: dom/base/nsContentUtils.cpp:707
(Diff revision 1)
>                                 "ui.use_activity_cursor", false);
>  
>    Preferences::AddBoolVarCache(&sAnimationsAPICoreEnabled,
>                                 "dom.animations-api.core.enabled", false);
>  
> +  Preferences::AddBoolVarCache(&sAnimationsAPIImplicitKeyframesEnabled,

Again, staticprefs.
Attachment #8992152 - Flags: review?(bzbarsky) → review+
Comment on attachment 8992155 [details]
Bug 1471814 - Add a preference for {Document,Element}.getAnimations();

https://reviewboard.mozilla.org/r/257060/#review264162

::: dom/base/nsContentUtils.cpp:713
(Diff revision 1)
>                                 false);
>  
>    Preferences::AddBoolVarCache(&sAnimationsAPICoreEnabled,
>                                 "dom.animations-api.core.enabled", false);
>  
> +  Preferences::AddBoolVarCache(&sAnimationsAPIGetAnimationsEnabled,

staticprefs.
Attachment #8992155 - Flags: review?(bzbarsky) → review+
Comment on attachment 8992153 [details]
Bug 1471814 - Add a preference for animation composite modes;

https://reviewboard.mozilla.org/r/257056/#review264164

::: commit-message-8226d:4
(Diff revision 1)
> +Bug 1471814 - Add a preference for animation composite modes; r?hiro, r?bz
> +
> +This feature should not be shipped until the various definitions of addition for
> +each additive property is properly specified.

s/is/are/

::: commit-message-8226d:8
(Diff revision 1)
> +This feature should not be shipped until the various definitions of addition for
> +each additive property is properly specified.
> +
> +Unlike other patches in this series, compositing is not frequently used
> +internally (e.g. by DevTools etc.) so there is no need to enable this by default
> +for Chrome code.

s/Chrome/system/

::: dom/base/nsContentUtils.cpp:705
(Diff revision 1)
>                                 "privacy.donottrackheader.enabled", false);
>  
>    Preferences::AddBoolVarCache(&sUseActivityCursor,
>                                 "ui.use_activity_cursor", false);
>  
> +  Preferences::AddBoolVarCache(&sAnimationsAPICompositingEnabled,

staticprefs
Attachment #8992153 - Flags: review?(bzbarsky) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8992152 [details]
> Bug 1471814 - Add a preference for implicit keyframes
> 
> https://reviewboard.mozilla.org/r/257054/#review263882
> 
> ::: layout/style/crashtests/crashtests.list:179
> (Diff revision 1)
> >  HTTP load 1320423-1.html
> >  load 1321357-1.html
> >  load 1328535-1.html
> >  load 1331272.html
> >  HTTP load 1333001-1.html
> > -pref(dom.animations-api.core.enabled,true) load 1340344.html
> > +pref(dom.animations-api.core.enabled,true) pref(dom.animations-api.implicit-keyframes.enabled,true) load 1340344.html
> 
> I don't think this test needs dom.animations-api.core.enabled, there is just
> a;
> 
>  document.documentElement.animate([{ "color":
> "hsla(6e147grad,16%,183.379675555%,0.0210463770007)" }])

I thought so too, but on the next line it waits on anim.ready so we need 'core' too.

> ::: layout/style/crashtests/crashtests.list:229
> (Diff revision 1)
> >  load 1390726.html
> >  load 1393791.html
> >  load 1384232.html
> >  load 1395725.html
> >  load 1396041.html
> > -load 1397363-1.html
> > +pref(dom.animations-api.implicit-keyframes.enabled,true) load 1397363-1.html
> 
> Huh?  This test has been working without any problems?  This test uses
> iterationComposite: "accumulate" so I think we need
> dom.animations-api.core.enabled, I don't know why it's not there.

It works properly because it's a crashtest. It just means it's not actually testing what it's supposed to in late-beta / release builds.

The patch that adds the compositing pref adds the annotation. At that point we shouldn't need the 'core' annotation anymore.
Blocks: 1476158
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b9959f1bc80
Add a preference for Web Animations timelines; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/bb1c7e6f5d7a
Add a preference for implicit keyframes; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/f302f2d706fb
Add a preference for animation composite modes; r=bz,hiro
https://hg.mozilla.org/integration/autoland/rev/bcc8cc984ddf
Ignore the composite member on Keyframe objects when the compositing pref is not set; r=hiro
https://hg.mozilla.org/integration/autoland/rev/86eaab5999fb
Add a preference for {Document,Element}.getAnimations(); r=bz,hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12023 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment on attachment 8992153 [details]
Bug 1471814 - Add a preference for animation composite modes;

https://reviewboard.mozilla.org/r/257056/#review264540

::: commit-message-8226d:12
(Diff revision 4)
> +internally (e.g. by DevTools etc.) so there is no need to enable this by default
> +for system code.
> +
> +Also, it turns out we have inadvertently been shipping part of this feature for
> +some time now. The next patch in this series will add tests for that case and
> +disable that part of the feature (a suitable intent to unship will follow). This

Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/4QgtCF1-Q74
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: