1293106 - Ignore 'spacing' if dom.animations-api.core.enabled is false

Status: RESOLVED FIXED

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

Description

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

We have implemented spacing but there are still a few outstanding pieces like bug 1272549, bug 1286150, and bug 1286151. We should avoid shipping this in a half-complete state by turning of spacing modes until the feature is complete.

Boris, do you mind taking this?

Comment 1

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #0)
> We have implemented spacing but there are still a few outstanding pieces
> like bug 1272549, bug 1286150, and bug 1286151. We should avoid shipping
> this in a half-complete state by turning of spacing modes until the feature
> is complete.
> 
> Boris, do you mind taking this?

OK. I can take this, but could you please give me some hints about this bug. I didn't resolve bugs like this before, so I'm not sure how many things I have to do. Thanks.

Comment 2

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

You probably want to do something like use Preferences::AddBoolVarCache (you can see lots of examples if you search the code for this). You can use the existing dom.animations-api.core.enabled pref. Then you will need to check that flag and skip processing the 'spacing' flag.

Comment 3

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #2)
> You probably want to do something like use Preferences::AddBoolVarCache (you
> can see lots of examples if you search the code for this). You can use the
> existing dom.animations-api.core.enabled pref. Then you will need to check
> that flag and skip processing the 'spacing' flag.

Thanks, I may add a helper function like this [1], so we can use the cached flag to enable/disable 'spacing'.

[1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/dom/animation/AnimationUtils.cpp#50-63

Comment 4

[Boris Chiou [:boris]]

BTW, should I write a test case for this?

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1293106 - Ignore 'spacing' if dom.animations-api.core.enabled is false.

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

Comment 6

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

Comment on attachment 8779694 [details]
Bug 1293106 - Ignore 'spacing' if dom.animations-api.core.enabled is false.

https://reviewboard.mozilla.org/r/70644/#review68632

This looks fine to me with the changes below. Are there any tests that might fail as a result of this once it hits beta? For example, are there any spacing tests that don't toggle this preference?

::: dom/animation/AnimationUtils.h:64
(Diff revision 1)
> +  /**
> +   * Checks if spacing feature is enabled.
> +   */
> +  static bool
> +  IsSpacingEnabled();

Perhaps we should just call this something that matches the pref since I think we'll use this to turn off other features like iterationComposite until they're specced properly.

Perhaps IsCoreAPIEnabled ?

  /**
   * Returns true if the preference to enable the core Web Animations API is
   * true.
   */

::: dom/animation/AnimationUtils.cpp:68
(Diff revision 1)
>  }
>  
> +/* static */ bool
> +AnimationUtils::IsSpacingEnabled()
> +{
> +  static bool sSpacingEnabled;

We should update these local variables to reflect the change to the function name.

::: dom/animation/KeyframeEffectParams.cpp:115
(Diff revision 1)
> +  // Ignore spacing if IsSpacingEnabled() returns false. We will remove this
> +  // after landing bug 1272549, bug 1286150, and bug 1286151.
> +  if (!AnimationUtils::IsSpacingEnabled()) {

// Ignore spacing if the core API is not enabled since it is not yet ready to
  // ship.


We won't necessarily ship it after those bugs are fixed since we probably need to also make sure it is specced properly--i.e. the distance formulae for each of the animation types is specced. We can probably do that while it rides the trains, but in any case, let's just leave out the release criteria out for now.

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8779694 [details]
Bug 1293106 - Ignore 'spacing' if dom.animations-api.core.enabled is false.

https://reviewboard.mozilla.org/r/70644/#review68632

> Perhaps we should just call this something that matches the pref since I think we'll use this to turn off other features like iterationComposite until they're specced properly.
> 
> Perhaps IsCoreAPIEnabled ?
> 
>   /**
>    * Returns true if the preference to enable the core Web Animations API is
>    * true.
>    */

OK. I see. Let's use IsCoreAPIEnabled() as the function name.

> We should update these local variables to reflect the change to the function name.

OK. I'd like to use "sCoreAPIEnabled".

> // Ignore spacing if the core API is not enabled since it is not yet ready to
>   // ship.
> 
> 
> We won't necessarily ship it after those bugs are fixed since we probably need to also make sure it is specced properly--i.e. the distance formulae for each of the animation types is specced. We can probably do that while it rides the trains, but in any case, let's just leave out the release criteria out for now.

OK. I will remove these bug numbers.

Comment 8

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #6)
> This looks fine to me with the changes below. Are there any tests that might
> fail as a result of this once it hits beta? For example, are there any
> spacing tests that don't toggle this preference?

All the tests added in Bug 1244590 are in testing/web-platform/tests/web-animaions/, and dom.animations-api-core.enabled is true in that directory, so I think our spacing tests all toggle this preference.

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8779694 [details]
Bug 1293106 - Ignore 'spacing' if dom.animations-api.core.enabled is false.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/055efa24a883
Ignore 'spacing' if dom.animations-api.core.enabled is false. r=birtles

Comment 11

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

Thanks for doing this. We'll need to uplift this to Aurora (perhaps after giving it a day or so to bake on m-c).

Comment 12

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #11)
> Thanks for doing this. We'll need to uplift this to Aurora (perhaps after
> giving it a day or so to bake on m-c).

OK. After we land this on m-c, I will uplift this to Aurora. Thanks.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/055efa24a883

Comment 14

[Boris Chiou [:boris]]

Attachment: Uplift to aurora. r=birtles

This patch is ready to uplift to aurora. Carry birtles' r+ from attachment 8779694 [details].

MozReview-Commit-ID: K7hbCjLP6vB

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8781813 [details] [diff] [review]
Uplift to aurora. r=birtles

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

Approval Request Comment
[Feature/regressing bug #]: Bug 1244590
[User impact if declined]: We should avoid shipping this feature in a half-complete state. if declined, users may use this but it doesn't work in some cases.
[Describe test coverage new/current, TreeHerder]: No specific tests for this bug. All tests for this feature had been added by Bug 1244590 and this patch doesn't affect those tests.
[Risks and why]: No. We just added a pref on a incomplete feature.
[String/UUID change made/needed]: None.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Fixed in 51 based on comment 13.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8781813 [details] [diff] [review]
Uplift to aurora. r=birtles

Makes sense to put a feature that is not ready behind a off-by-default pref, Aurora50+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84220f4fafeb