1078122 - Provide better encapsulation of AnimationPlayer

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles)]

Splitting this bug off from bug 1073336 to address the refactoring mentioned there.

In summary we want to:
* Better encapsulate the members of AnimationPlayer
* Move queuing of animation events into CSSAnimationPlayer

Eventually we want to move queuing of transition events into CSSTransitionPlayer. We don't have CSSTransitionPlayer and, after an initial attempt, it looks like we'll need to merge more of nsAnimationManager and nsTransitionManager first.

Also, we want encapsulate mStartTime in AnimationPlayer yet because first we'll tackle that when we make start time settable in general (bug 1073379).

For rationale of this refactoring see the last part of bug 1073336 comment 0.

Comment 1

[Brian Birtles (:birtles)]

Attachment: part 1 - Move checks for animation throttling to AnimationPlayer

This patch moves code from AnimationPlayerCollection to AnimationPlayer.

However, there is one subtle change in logic involved. Previously, we would test
if the player had finished by getting the computed time of its source content
and checking if it was in the after phase or not. In this patch, however, we
simply check the play state to see if it is finished or not.

These two approaches differ in the case where an animation is paused. The
animation phase approach will indicate the player has finished, by the play
state approach will indicate the player has paused (since the "paused" state
trumps the "finished" state). This, however, should not produce any observable
effect because when an animation is paused mIsRunningOnCompositor will be false
(we don't put paused animations on the compositor).

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 2 - Encapsulate mIsRunningOnCompositor in AnimationPlayer

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 3 - Move animation value building down to the Animation objects

This patch extracts the logic for calculating animation styles from
AnimationPlayerCollection and puts the bulk of it into the Animation objects.
Some of the initial logic surrounding the animation player state (e.g. is it
paused or not, etc.) is put into AnimationPlayer.

In future we may shift this logic even further down to the AnimationEffect
objects but currently we don't create such objects unless necessary.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 4 - Make the mSource and mTimeline members of AnimationPlayer, protected

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 5 - Move CSSAnimationPlayer to nsAnimationManager

This patch takes the CSSAnimationPlayer object, currently defined in
dom/animation/AnimationPlayer.{cpp,h}, and moves it to
layout/style/nsAnimationManager.{cpp,h} where the rest of the CSS
Animations-specific code lives.

At the same time it extends the scope of the mozilla namespace block in
nsAnimationManager.h to include also the AnimationEventInfo and EventArray types
since these class which don't have an ns* prefix probably should be in the
mozilla namespace anyway.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 6 - Store the previous finished state

AnimationPlayer::CanThrottle determines if an animation player has just finished
by inspecting the value of mLastNotification. This is problematic for two
reasons:

1. mLastNotification is intended to be used for events (as the XXX comment
   notes)
2. mLastNotification is specific to CSS Animations and should be moved to
   CSSAnimationPlayer.

To address this, this patch adds an extra member mPreviousFinishedState. The Web
Animations spec already defines animation players as having such a member:

  http://w3c.github.io/web-animations/#previous-finished-state

We set it to true when we calculate the style for an animation that has
finished. This differs slightly from the code it is replacing as explained
below.

In the case of CSS Animations we perform the following sequence of steps on each
sample.

  1. EnsureStyleRuleFor (calls CanThrottle, and maybe ComposeStyle)
  2. GetEventsForCurrentTime

In the existing code, we update mLastNotification in (2) which happens on every
sample, even throttled samples.

In this patch, however, we update mPreviousFinishedState in (1) during the
ComposeStyle step which only happens for unthrottled samples. So, as of this
patch, in CanThrottle, we ask "have we newly entered the finished state since
the last *unthrottled* sample?", whereas previously we simply looked for
a change since the last sample, throttled or not. However, if the answer to the
question is "yes", then we'll run an unthrottled sample and update
mPreviousFinishedState so these should be functionally equivalent.

Another subtle difference is that this patch looks at the player's finished
state rather than the animation phase of its source content, and these will
produce different results in the case where the player is paused. However, since
paused animations are not run on the compositor, this should not matter.

In the case of CSS Transitions, AnimationPlayer::CanThrottle() is not currently
used and so mPreviousFinishedState is irrelevant.

Ultimately, both the existing and the new code is somewhat fragile but hopefully
this will be addressed by:
* Replacing mPreviousFinishedState with inspecting whether the finished promise
  is settled (bug 1074630),
* Merging more of the code in nsAnimationManager and nsTransitionManager and
  applying a unified approach to sampling that better accommodates these
  considerations.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 7 - Add method to Animation to get the target element and pseudo

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 8 - Move queuing of CSS animation events to CSSAnimationPlayer

This patch moves the code for queuing CSS animation events from
nsAnimationManager to CSSAnimationPlayer. In doing so, it also moves the
mLastNotification member and associated enum values.

Comment 9

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=18e28e00d335

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 1 - Move checks for animation throttling to AnimationPlayer

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 2 - Encapsulate mIsRunningOnCompositor in AnimationPlayer

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 3 - Move animation value building down to the Animation objects

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 4 - Make the mSource and mTimeline members of AnimationPlayer, protected

Comment 14

[Brian Birtles (:birtles)]

Attachment: part 5 - Move CSSAnimationPlayer to nsAnimationManager

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 6 - Store the previous finished state

Comment 16

[Brian Birtles (:birtles)]

Attachment: part 7 - Add method to Animation to get the target element and pseudo

Comment 17

[Brian Birtles (:birtles)]

Attachment: part 8 - Use the new GetTarget overload in AnimationPlayer

Comment 18

[Brian Birtles (:birtles)]

Attachment: part 9 - Move queuing of CSS animation events to CSSAnimationPlayer

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8502972 [details] [diff] [review]
part 1 - Move checks for animation throttling to AnimationPlayer

Quoting your extended commit message:
>These two approaches differ in the case where an animation is paused. The
>animation phase approach will indicate the player has finished, by the play
>state approach will indicate the player has paused (since the "paused" state

Two things:
 - First quoted line: I think you might really mean "in the case where an animation is paused **after it has finished**"?  (It sounds like that's what you're talking about, at least - not normal partway-through-animation pausing.)
 - Second quoted line: s/by the play/but the play/, I think?

>+++ b/dom/animation/AnimationPlayer.cpp
>+bool
>+AnimationPlayer::CanThrottle() const
>+{
[...]
>+  // If the animation is ending we can't throttle because we need to get the
>+  // correct end-of-animation behavior (the styles of the animation disappear,
>+  // or the fill mode behaviour).
>+  return PlayState() != AnimationPlayState::Finished ||
>+         mSource->LastNotification() == Animation::LAST_NOTIFICATION_END;

This joint condition is a little confusing to interpret & reason about, for me at least.

Perhaps it could be restructured into two separate return statements?

e.g. something like:

  if (PlayState() != AnimationPlayState::Finished) {
    // Unfinished animations can be throttled.
    return true;
  }

  // Our animation is ::Finished -- but if we haven't yet sent an
  // "end" notification, then we still need a main-thread style flush
  // and can't throttle.
  return mSource->LastNotification() == Animation::LAST_NOTIFICATION_END;

(My sample comments probably have some room for improvement; obviously, feel free to tweak/extend them.)

r=me with the commit-message typo fixed; up to you whether you think this logic-restructuring makes things clearer (though I think it does)

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8502973 [details] [diff] [review]
part 2 - Encapsulate mIsRunningOnCompositor in AnimationPlayer

r=me

Comment 21

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 8502972 [details] [diff] [review]
> part 1 - Move checks for animation throttling to AnimationPlayer
...
> e.g. something like:
> f (PlayState() != AnimationPlayState::Finished) {
>     // Unfinished animations can be throttled.
>     return true;
>   }
> 
>   // Our animation is ::Finished -- but if we haven't yet sent an
>   // "end" notification, then we still need a main-thread style flush
>   // and can't throttle.
>   return mSource->LastNotification() == Animation::LAST_NOTIFICATION_END;

I've made this:

  if (PlayState() != AnimationPlayState::Finished) {
    // Unfinished animations can be throttled.
    return true;
  }

  // The animation has finished but, if this is the first sample since
  // finishing, we need an unthrottled sample so we can apply the correct
  // end-of-animation behavior on the main thread (either removing the
  // animation style or applying the fill mode).
  return mSource->LastNotification() == Animation::LAST_NOTIFICATION_END;

The rest all makes sense. Thanks!

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8502974 [details] [diff] [review]
part 3 - Move animation value building down to the Animation objects

r=me on part 3, with comments below addressed. (Since this is just moving code, I mostly focused on just making sure that the code was faithfully moved.)

>+++ b/dom/animation/Animation.cpp
>+    if (aSetProperties.HasProperty(prop.mProperty)) {
>+      // Another animation already set this property.
>+      return;
>+    }

Could you extend this comment a bit, while you're here, to clarify *why* we can return early if another animation has already set this property? (Do we know for sure that it has a higher precedence than us & would override our effects?)

>+      if (segment == segmentEnd) {
>+        MOZ_ASSERT(false, "incorrect time fraction");
>+        break; // in order to continue in outer loop (just below)
[...]
>-            NS_ABORT_IF_FALSE(false, "incorrect time fraction");
>-            break; // in order to continue in outer loop (just below)

Optional note: we now have MOZ_ASSERT_UNREACHABLE("foo") in Assertions.h -- it's probably better to use that instead of MOZ_ASSERT(false, "foo") going forward. (It's a few characters longer, but it's more readable & more declarative about the intent of the assertion.)

(I'm only bringing this up since you're already tweaking the type of assertion being used here.)

(There used to be some confusion around this because we had MOZ_ASSUME_UNREACHABLE, which was actually a dangerous compiler hint, but people used it as if it were NS_UNREACHABLE and got dangerous results. Now that dangerous one has been renamed, and MOZ_ASSERT_UNREACHABLE is what you'd expect it to be.)

>+++ b/dom/animation/Animation.h
>+  void ComposeStyle(nsRefPtr<css::AnimValuesStyleRule>& aStyleRule,
>+                    nsCSSPropertySet& aSetProperties);

Please add documentation for this new ComposeStyle() method -- in particular, noting the meaning/expectations of both args, and noting that they're both in/out-params.

>+++ b/dom/animation/AnimationPlayer.h
>+  void ComposeStyle(nsRefPtr<css::AnimValuesStyleRule>& aStyleRule,
>+                    nsCSSPropertySet& aSetProperties,
>+                    bool& aNeedsRefreshes);
>+

Same for this one. (Important to note here: looks like aNeedsRefreshes is only modified if we see that a refresh is needed; otherwise, it's left untouched.)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8502975 [details] [diff] [review]
part 4 - Make the mSource and mTimeline members of AnimationPlayer, protected

>Bug 1078122 part 4 - Make the mSource and mTimeline members of AnimationPlayer, protected

Extreme grammar nit: comma doesn't make sense before "protected" there.

You probably want to drop the comma, or maybe "Move the mSource ...[etc]... to protected section", or something else.

r=me regardless.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8502976 [details] [diff] [review]
part 5 - Move CSSAnimationPlayer to nsAnimationManager

r=me, just one nit in the extended commit message:
> [...] to include also the AnimationEventInfo and EventArray types
>since these class which don't have an ns* prefix probably should be in the
>mozilla namespace anyway.

Looks like middle line there needs s/these class/these classes/.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8502979 [details] [diff] [review]
part 8 - Use the new GetTarget overload in AnimationPlayer

>+++ b/dom/animation/AnimationPlayer.cpp
>+  Element* targetElement;
>+  nsCSSPseudoElements::Type pseudoType;
>+  mSource->GetTarget(targetElement, pseudoType);
>+  if (!targetElement) {
>+    return;
>+  }

Perhaps this would be clearer if the new GetTarget() took its outparams as pointers instead of as references? So it'd be invoked like "mSource->GetTarget(&targetElement, &pseudoType);".  That would make it clearer that both args are outparams -- otherwise, from casual inspection at a call-site, it's a bit unclear whether one of the params might be an input-param.

Just a thought; it's fine as-is, too.

Comment 26

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Comment on attachment 8502979 [details] [diff] [review]
> part 8 - Use the new GetTarget overload in AnimationPlayer
> 
> >+++ b/dom/animation/AnimationPlayer.cpp
> >+  Element* targetElement;
> >+  nsCSSPseudoElements::Type pseudoType;
> >+  mSource->GetTarget(targetElement, pseudoType);
> >+  if (!targetElement) {
> >+    return;
> >+  }
> 
> Perhaps this would be clearer if the new GetTarget() took its outparams as
> pointers instead of as references? So it'd be invoked like
> "mSource->GetTarget(&targetElement, &pseudoType);".  That would make it
> clearer that both args are outparams -- otherwise, from casual inspection at
> a call-site, it's a bit unclear whether one of the params might be an
> input-param.

Yeah, that's one of my very few stylistic preferences, namely, "Use references when you can, and pointers when you have to." I thought that was a bit of a C++ convention (C++ faq lite etc. talk about it).

I find that if you don't need a null sentinel value, using references leads to much less code (less redundant null checking), less bugs (from forgetting to check for null), and cleaner code (because, if you're going to pass something in a pointer, you check for null at the call site when you de-reference it and normally that's a much better place to handle the null case).

So I'd prefer using references if that's ok.

Comment 27

[Daniel Holbert [:dholbert]]

Yup, that's fine with me.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8502977 [details] [diff] [review]
part 6 - Store the previous finished state

I haven't looked at this part in detail yet, but I wonder if mPreviousFinishedState should really be called "mIsPreviousStateFinished"? (or "mWasPreviousStateFinished")

When I see "mPreviousFinishedState", I think "the state that we were in, the last time were finished", or something like that, and it sounds like something that would be enum-flavored.  But really, I think it's just a flag telling us if the previous state was finished...?  I think the various comparisons that involve this value would be more understandable if it were a bool named "mIs{SOMETHING}", instead of "m{Something}State".

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #28)
>  But really, I think it's just a flag
> telling us if the previous state was finished...?

(and by "finished" I mean "::Finished" of course)

Comment 30

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #28)
> Comment on attachment 8502977 [details] [diff] [review]
> part 6 - Store the previous finished state
> 
> I haven't looked at this part in detail yet, but I wonder if
> mPreviousFinishedState should really be called "mIsPreviousStateFinished"?
> (or "mWasPreviousStateFinished")
> 
> When I see "mPreviousFinishedState", I think "the state that we were in, the
> last time were finished", or something like that, and it sounds like
> something that would be enum-flavored.  But really, I think it's just a flag
> telling us if the previous state was finished...?  I think the various
> comparisons that involve this value would be more understandable if it were
> a bool named "mIs{SOMETHING}", instead of "m{Something}State".

Yes, I agree, that sounds better. I was trying to match the terminology in the spec, but I think it makes sense to add 'Is' here.

Comment 31

[Daniel Holbert [:dholbert]]

Cool -- I'd suggest tying it to the spec terminology with a code comment, e.g.:
  bool mIsPreviousStateFinished; // Spec calls this "previous finished state"

Comment 32

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #1)
> Created attachment 8500741 [details] [diff] [review]
> part 1 - Move checks for animation throttling to AnimationPlayer

Actually, one question on part 1 (which adds AnimationPlayer::CanThrottle):

So, it looks to me like things will happen like so, at least in one code-path:
 (A) We enter nsAnimationManager::FlushAnimations().

 (B) It calls collection->CanThrottleAnimation(now), and uses the result to choose its EnsureStyleRuleFlags for that collection (_IsThrottled vs. _IsNotThrottled)

 (C) It calls UpdateStyleAndEvents() which calls EnsureStyleRuleFor() for the collection, and *that* actually loops over our players and check if they CanThrottle(). (And stomps on the passed-in flag, if the result disagrees)

This leads me to the question: why don't we do the per-player CanThrottle() checks inside the collection->CanThrottleAnimation call?  That seems more logical to me, and it would mean that the _IsThrottled vs. _IsNotThrottled value that's passed into EnsureStyleRuleFor() would actually be honored (since we'd be more sure about throttling *before* we call EnsureStyleRuleFor)

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8502977 [details] [diff] [review]
part 6 - Store the previous finished state

>+++ b/dom/animation/AnimationPlayer.h
>+  // FIXME: When we implement the finished promise (bug 1074630) we can
>+  // probably remove this and check if the promise has been settled yet
>+  // or not instead.
>+  bool mPreviousFinishedState;


Per comment 29-31, let's rename this, and add a comment to tie it to the "previous finished state" variable in the spec.

Also: Add a comment briefly explaining what this bool actually tracks, e.g.:
  // Indicates whether we were in the ::Finished state, during our most
  // recent unthrottled sample (our last ComposeStyle call).

r=me with that.

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8502980 [details] [diff] [review]
part 9 - Move queuing of CSS animation events to CSSAnimationPlayer

>+++ b/layout/style/nsAnimationManager.cpp
> void
>+CSSAnimationPlayer::QueueEvents(EventArray& aEventsToDispatch)
>+{
[...]
>+        uint32_t message =
>+          mLastNotification == CSSAnimationPlayer::LAST_NOTIFICATION_NONE
>+                            ? NS_ANIMATION_START
>+                            : NS_ANIMATION_ITERATION;
[...]
>-          uint32_t message =
>-            anim->LastNotification() == Animation::LAST_NOTIFICATION_NONE
>-                                        ? NS_ANIMATION_START
>-                                        : NS_ANIMATION_ITERATION;

Two nits:
 (1) You don't need the "CSSAnimationPlayer::" prefixing here, since this is in a CSSAnimationPlayer:: method.  (This applies to several places in this patch.) (The old code *did* need the prefixing, because the usages were in a nsAnimationManager function whereas the enum was defined in the "Animation::" namespace -- but now the enum & usages are all in the same class.)

 (2) The indentation on the ?/: lines is changing here (with respect to the previous line), and I think it's incorrect both before & after the change. I think those lines should be de-intended to be aligned directly under the "m" in "mLastNotification". It should look like:
        uint32_t message =
          here-is-a-very-wide-boolean-condition
          ? foo
          : bar;
Right now, it's aligned with respect to the "==", but there's no reason for that, because the "==" is just an arbitrary piece of the boolean condition.

(I believe the Coding Style guide agrees with me, under "Operators", at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )


>+    case ComputedTiming::AnimationPhase_After:
>+      // If we skipped the animation interval entirely, dispatch
>+      // 'animationstart' first
>+      nsString pseudoString = PseudoTypeAsString(targetPseudoType);

I think you need braces around this entire "case", if you're declaring a local variable directly inside it. (Without that, I'd think this shouldn't compile, though maybe I'm misremembering.) (BUT, I'm not sure we want to keep this variable, per my next point:)

It also seems a little unfortunate that we need to do this pseudoType-to-String conversion here, for each animation... Also, slightly-more-importantly, the new PsuedoTypeAsString static-method seems to duplicate logic from nsAnimationCollection::PseudoElement(), which is unfortunate:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.h?rev=a014629454d0#254

One idea to address this: since the caller (nsAnimationManager::QueueEvents) is operating on a single AnimationPlayerCollection, with a single target, could we trust *it* to look up the target/pseudo, and pass that to each player, so that the players don't have to individually look them up? (except maybe in debug builds as a sanity-check)

So e.g. I'm thinking that we'd make a single call to aCollection->PseudoElement() before the "for" loop, here, and store the result in a local nsString:
> void
>+nsAnimationManager::QueueEvents(AnimationPlayerCollection* aCollection,
>+                                EventArray& aEventsToDispatch)
> {
>   for (size_t playerIdx = aCollection->mPlayers.Length(); playerIdx-- != 0; ) {
[...]
>+    CSSAnimationPlayer* player =
>+      aCollection->mPlayers[playerIdx]->AsCSSAnimationPlayer();
>+    MOZ_ASSERT(player, "Expected a collection of CSS Animation players");
>+    player->QueueEvents(aEventsToDispatch);
>   }
> }
...and then we'd pass that string and aCollection->mElement to each player->QueueEvents() call.

What do you think?

Comment 35

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #32)
> (In reply to Brian Birtles (:birtles) from comment #1)
> > Created attachment 8500741 [details] [diff] [review]
> > part 1 - Move checks for animation throttling to AnimationPlayer
> 
> Actually, one question on part 1 (which adds AnimationPlayer::CanThrottle):
> 
> So, it looks to me like things will happen like so, at least in one
> code-path:
>  (A) We enter nsAnimationManager::FlushAnimations().
> 
>  (B) It calls collection->CanThrottleAnimation(now), and uses the result to
> choose its EnsureStyleRuleFlags for that collection (_IsThrottled vs.
> _IsNotThrottled)
> 
>  (C) It calls UpdateStyleAndEvents() which calls EnsureStyleRuleFor() for
> the collection, and *that* actually loops over our players and check if they
> CanThrottle(). (And stomps on the passed-in flag, if the result disagrees)
> 
> This leads me to the question: why don't we do the per-player CanThrottle()
> checks inside the collection->CanThrottleAnimation call?  That seems more
> logical to me, and it would mean that the _IsThrottled vs. _IsNotThrottled
> value that's passed into EnsureStyleRuleFor() would actually be honored
> (since we'd be more sure about throttling *before* we call
> EnsureStyleRuleFor)

I definitely agree that makes a lot more sense. The main trouble is transitions. nsTransitionManager currently detects if an animation has started that could be run on the compositor by checking collection->CanThrottleAnimation() and comparing it with player->IsRunningOnCompositor().

If it finds we *could* throttle a running animation but *aren't* running it on the compositor, then it updates the animation generation so we know to update the animations on the layer.

If we change collection->CanThrottleAnimation to call the per-play CanThrottle it will return *false* when the animation first starts running since IsRunningOnCompositor will be false and we'll fail to update the animation generation.

However, I think that's ok. As far as I can tell, we only use the animation generation to prevent throttling so if we fail to update it but prevent throttling (as AnimationPlayer::CanThrottle will do for as long as we're not running on the compositor) then we should be ok. That is to say, I think we could make transition manager work with this change. However, I think there's some risk involved so it's probably better done in a separate bug.

(The operation of nsTransitionManager and nsAnimationManager has all sorts of subtle differences that makes maintaining this code really hard and one of my medium-term goals is to merge as much of this code as possible. This bug is one step in that direction.)

Comment 36

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #34)
> Comment on attachment 8502980 [details] [diff] [review]
> part 9 - Move queuing of CSS animation events to CSSAnimationPlayer
...
> >+    case ComputedTiming::AnimationPhase_After:
> >+      // If we skipped the animation interval entirely, dispatch
> >+      // 'animationstart' first
> >+      nsString pseudoString = PseudoTypeAsString(targetPseudoType);
> 
> I think you need braces around this entire "case", if you're declaring a
> local variable directly inside it. (Without that, I'd think this shouldn't
> compile, though maybe I'm misremembering.)

It will compile, but the scope of pseudoString will leak beyond that case section. I've fixed it now.

> It also seems a little unfortunate that we need to do this
> pseudoType-to-String conversion here, for each animation... Also,
> slightly-more-importantly, the new PsuedoTypeAsString static-method seems to
> duplicate logic from nsAnimationCollection::PseudoElement(), which is
> unfortunate:
>  http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.
> h?rev=a014629454d0#254

That method should disappear once we move transition event dispatch to CSSTransitionPlayer (at which point we'll re-use Animation::PseudoTypeAsString).

> One idea to address this: since the caller (nsAnimationManager::QueueEvents)
> is operating on a single AnimationPlayerCollection, with a single target,
> could we trust *it* to look up the target/pseudo, and pass that to each
> player, so that the players don't have to individually look them up? (except
> maybe in debug builds as a sanity-check)
...
> What do you think?

I don't like this from a separation-of-concerns point of view. Knowledge of the animation target really belongs in the Animation. If performance is a concern, we should use atoms for these strings.

The existing code already generates strings each time we dispatch an event. This patch attempts to optimize the case where we dispatch two events at once by re-using the string but I think it actually makes it worse because we'll end up storing a string when the animation has finished and already dispatched its end event. I'll update the patch to not do that.

Comment 37

[Brian Birtles (:birtles)]

Attachment: part 9 - Move queuing of CSS animation events to CSSAnimationPlayer

Removed redundant string generalization

Comment 38

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #35)
> > This leads me to the question: why don't we do the per-player CanThrottle()
> > checks inside the collection->CanThrottleAnimation call?
[...]
> I think
> there's some risk involved so it's probably better done in a separate bug.

Sounds good to me -- could you file that bug before closing this out?

(In reply to Brian Birtles (:birtles) from comment #36)
> I don't like this from a separation-of-concerns point of view. Knowledge of
> the animation target really belongs in the Animation. If performance is a
> concern, we should use atoms for these strings.

Yup, that's fair. My main (minor) concern was the duplicated logic for pseudoType-to-string mapping, but it sounds like the new version will disappear soon, as you noted, so probably not worth worrying much about then.

Comment 39

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #38)
> (In reply to Brian Birtles (:birtles) from comment #35)
> > > This leads me to the question: why don't we do the per-player CanThrottle()
> > > checks inside the collection->CanThrottleAnimation call?
> [...]
> > I think
> > there's some risk involved so it's probably better done in a separate bug.
> 
> Sounds good to me -- could you file that bug before closing this out?

Filed bug 1084220.

Comment 40

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe98ceaaafa7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7dfd335493
https://hg.mozilla.org/integration/mozilla-inbound/rev/d54306da529f
https://hg.mozilla.org/integration/mozilla-inbound/rev/93c56f764bc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d628be8f7d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/082d6d6cf6ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e807f0f41c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a66dbf7d8e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b581cda940

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe98ceaaafa7
https://hg.mozilla.org/mozilla-central/rev/8f7dfd335493
https://hg.mozilla.org/mozilla-central/rev/d54306da529f
https://hg.mozilla.org/mozilla-central/rev/93c56f764bc3
https://hg.mozilla.org/mozilla-central/rev/4d628be8f7d0
https://hg.mozilla.org/mozilla-central/rev/082d6d6cf6ac
https://hg.mozilla.org/mozilla-central/rev/6e807f0f41c6
https://hg.mozilla.org/mozilla-central/rev/1a66dbf7d8e1
https://hg.mozilla.org/mozilla-central/rev/b3b581cda940