Splitting this off from bug 999922. This bug corresponds to the "Phase one" described in that bug.

This patch takes StyleAnimation and makes it ref-counted heap object. This should allow us to store StyleAnimation and its subclasses (transitions only currently) in a consistent fashion (an array of base-class pointers). Furthermore, this will be helpful if we want these things to be pointed to from Javascript objects that may, for example, preserve their lifetime beyond that of the element that currently owns them. This patch also introduces a typedef for an array of refptrs to StyleAnimation objects (and similarly for the subclass ElementPropertyTransition) to simplify the code somewhat.

I'm not quite sure what ElementPropertyTransition is supposed to mean. As I understand it, there is also nsTransition which is an abstract definition of a transition and ElementPropertyTransition is the concrete version of that once applied to a specific element. I'd rather we had TransitionDef(inition)/TransitionSpec/TransitionParams or something similar for the abstract definition and Transition for the concrete application. I wonder if StyleTransition is suitable and aligns better with the superclass StyleAnimation? In this patch I've gone with StyleTransition but I'm open to other suggestions. I suspect StyleAnimation might later become mozilla::dom::Animation so this maybe isn't worth too much concern. This patch also fixes a few minor instances of trailing whitespace.

Add a method for downcasting from a StyleAnimation to a StyleTransition (when the underlying object is a StyleTransition). This, unfortunately, adds a vtable to StyleAnimation but in the long term I hope we will be able to isolate transition-specific code to a specific kind of TransitionEffect that hangs off StyleAnimation and put the vtable on AnimationEffect instead.

As a result, transitions are now stored using a pointer to the base class, mozilla::StyleAnimation. We downcast to a transition only when necessary. No error-checking of the result of AsTransition is performed since we only ever call it on the mAnimations member of ElementTransitions.

Comment on attachment 8415767 [details] [diff] [review] part 1 - Put StyleAnimation on the heap nsAnimationManager::BuildAnimations should probably rename aDest -> dest and aSrc -> src, since they're not arguments anymore. (That's ok to do in a separate patch on top of other patches if you prefer, or ok to roll into this one.) I'm not crazy about the pattern of using reference variables by assigning a dereferenced pointer into them like this: ElementPropertyTransition &pt = *pts[i]; I might have preferred to convert the variable to a pointer. But I think it's ok as is.

Comment on attachment 8415768 [details] [diff] [review] part 2 - Rename ElementPropertyTransition to StyleTransition >I'm not quite sure what ElementPropertyTransition is supposed to mean. As It means that it's an object that represents the transition of one property on one element (as opposed to ElementTransitions, which represents all the transitions for that element). It could just be ElementTransition, but then we'd be distinguishing two classes that differ only in the final "s", which is rather annoying. I guess I'm ok with renaming to StyleTransition, although I slightly prefer the old name. But see later comments. (But we should really rename nsStyleAnimation::Value to AnimationValue (or StyleAnimationValue) and move the static methods on nsStyleAnimation to be static methods on AnimationValue, to avoid confusing StyleAnimation and nsStyleAnimation.) >I understand it, there is also nsTransition which is an abstract definition of >a transition nsTransition is one item in the computed value of the CSS 'transition' shorthand property. It might make sense to rename nsTransition -> StyleTransition and nsAnimation -> StyleAnimation, actually. Though I'd be ok with names including the word definition. > and ElementPropertyTransition is the concrete version of that >once applied to a specific element. I'd rather we had >TransitionDef(inition)/TransitionSpec/TransitionParams or something similar for >the abstract definition and Transition for the concrete application. >I wonder if StyleTransition is suitable and aligns better with the superclass >StyleAnimation? What's TransitionParams? And I prefer ElementTransition (or ElementPropertyTransition) to TransitionSpec if the point is that it's a transition running on a particular element. review- because I think we want a little more discussion here

Could you explain where patches 3 and 4 are taking us? What does having a vtable here and the ability to share this get you?

Comment on attachment 8415769 [details] [diff] [review] part 3 - Add StyleAnimation::AsTransition virtual method >+ virtual ~StyleAnimation() { } >+ >+ // FIXME: Remove this once we isolate transition-specific code to the >+ // animation effect >+ virtual StyleTransition* AsTransition() { return nullptr; } >+ virtual const StyleTransition* AsTransition() const { return nullptr; } If this FIXME means removing the virtual destructor as well (I'm not sure), it should say so explicitly. Also, I'm not sure what "the animation effect" is; could you clarify the comment? r=dbaron with that

Comment on attachment 8415770 [details] [diff] [review] part 4 - Move mAnimations to CommonElementAnimationData Can CanPerformOnCompositorThread move to the base class now (in a separate patch)? r=dbaron

> nsAnimationManager::BuildAnimations should probably rename aDest -> dest > and aSrc -> src, since they're not arguments anymore. (That's ok to do > in a separate patch on top of other patches if you prefer, or ok to roll > into this one.) Rolled in here. > I'm not crazy about the pattern of using reference variables by > assigning a dereferenced pointer into them like this: > ElementPropertyTransition &pt = *pts[i]; > I might have preferred to convert the variable to a pointer. But > I think it's ok as is. I'm not so keen on it either so I've changed the variables like this to pointers. After the renaming in part 2 I'm going to have to rebase all the remaining patches anyway so doing this doesn't make it much worse.

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #6) > Comment on attachment 8415768 [details] [diff] [review] > part 2 - Rename ElementPropertyTransition to StyleTransition > > >I'm not quite sure what ElementPropertyTransition is supposed to mean. As > > It means that it's an object that represents the transition of one property > on one element (as opposed to ElementTransitions, which represents all the > transitions for that element). It could just be ElementTransition, but then > we'd be distinguishing two classes that differ only in the final "s", which > is rather annoying. > > I guess I'm ok with renaming to StyleTransition, although I slightly prefer > the old name. But see later comments. > > (But we should really rename nsStyleAnimation::Value to AnimationValue (or > StyleAnimationValue) and move the static methods on nsStyleAnimation to be > static methods on AnimationValue, to avoid confusing StyleAnimation and > nsStyleAnimation.) > > >I understand it, there is also nsTransition which is an abstract definition of > >a transition > > nsTransition is one item in the computed value of the CSS 'transition' > shorthand property. It might make sense to rename nsTransition -> > StyleTransition and nsAnimation -> StyleAnimation, actually. Though I'd be > ok with names including the word definition. > > > and ElementPropertyTransition is the concrete version of that > >once applied to a specific element. I'd rather we had > >TransitionDef(inition)/TransitionSpec/TransitionParams or something similar for > >the abstract definition and Transition for the concrete application. > >I wonder if StyleTransition is suitable and aligns better with the superclass > >StyleAnimation? > > What's TransitionParams? And I prefer ElementTransition (or > ElementPropertyTransition) to TransitionSpec if the point is that it's a > transition running on a particular element. > > > review- because I think we want a little more discussion here That explanation makes sense. I propose the following renaming: StyleAnimation -> ElementAnimation (eventually will become mozilla::dom::Animation) ElementPropertyTransition -> ElementTransition nsTransition -> StyleTransition nsAnimation -> StyleAnimation I think "ElementTransition" is ok despite being very similar to ElementTransitions since by the end of bug 999922 (specifically phase 3), ElementTransitions should disappear. I'm going to go ahead and make up a patch or two for this and request review on that.

(In reply to Brian Birtles (:birtles) from comment #11) > That explanation makes sense. I propose the following renaming: > > StyleAnimation -> ElementAnimation (eventually will become > mozilla::dom::Animation) > ElementPropertyTransition -> ElementTransition > nsTransition -> StyleTransition > nsAnimation -> StyleAnimation > > I think "ElementTransition" is ok despite being very similar to > ElementTransitions since by the end of bug 999922 (specifically phase 3), > ElementTransitions should disappear. Having looked into this a bit more I think what I'll do is, in this bug: * mozilla::StyleAnimation -> mozilla::ElementAnimation * ElementPropertyTransition -> ElementTransition (should this go in mozilla namespace at the same time?) I'm actually less worried about the last change now. I was mostly concerned about aligning mozilla::StyleAnimation and ElementPropertyTransition somehow. Then, in a separate bug, do the following: * nsStyleAnimation::Value -> mozilla::StyleAnimationValue including folding in static methods in nsStyleAnimation * nsAnimation -> mozilla::StyleAnimation * nsTransition -> mozilla::StyleTransition Given the detailed comments in comment 6, I think it's probably ok to get dholbert to review this if dbaron is not available, but David, if you have a moment, please let me know if you think this seems ok.

We currently have mozilla::StyleAnimation as well as nsStyleAnimation. This patch renames StyleAnimation back to ElementAnimation. Although ElementAnimation is very similar to ElementAnimations, in the near future we expect to retire ElementAnimations and replace it with a common AnimationSet-like structure that is covers the features of ElementAnimations and ElementTransitions.

The previous naming ElementPropertyTransition referred to the fact that this object represents the transition of a single property on an element. Renaming to ElementTransition hides the fact that we use one object per-property but otherwise simplifies the naming somewhat. This patch also fixes a few minor instances of trailing whitespace.

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #7) > Could you explain where patches 3 and 4 are taking us? What does having a > vtable here and the ability to share this get you? The overall picture is outlined in bug 999922 which is part of bug 999921. At a glance it looks like this: * Refactor the transition and animation objects to the point where the amount of transition-specific code is limited. This allows us to store an array of base-class objects on the superclass of ElementAnimations/ElementTransitions * Then we gradually move methods from ElementAnimations and ElementTransitions to this base class until we no longer need ElementAnimations/ElementTransitions and have simply "AnimationSet" This means: -> More shared code for timing calculations (removing duplication not only in the animation managers but also in nsLayoutUtils etc. and hopefully the macros in AnimationCommon) -> A generic "animation" object we can map a DOM API onto, and re-use for SVG and script-generated animations -> (Maybe) A single list of animations per-element we can use for determining animation stacking (rather than consulting 4 different animations managers: transitions, animations, SVG, script). Web Animations has a notion of an "animation effect" which is the part of an animation that actually updates property values. If we end up matching that layout, then my hope is that we can move the transition-specific behavior currently contained in ElementPropertyTransition to the effect. That would mean we could make the animation superclass here non-virtual by shifting the vtable to the effect.

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #9) > Comment on attachment 8415770 [details] [diff] [review] > part 4 - Move mAnimations to CommonElementAnimationData > > Can CanPerformOnCompositorThread move to the base class now (in a separate > patch)? > > r=dbaron Yes, that's part of phase 3 in bug 999922

Comment on attachment 8420763 [details] [diff] [review] part 2.2 - Rename ElementPropertyTransition to ElementTransition RE this patch: (In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #6) > It could just be ElementTransition, but then > we'd be distinguishing two classes that differ only in the final "s", which > is rather annoying. (In reply to Brian Birtles (:birtles) from comment #11) > I think "ElementTransition" is ok despite being very similar to > ElementTransitions since by the end of bug 999922 (specifically phase 3), > ElementTransitions should disappear. Hmm... Is that expected to happen imminently? Given that it's in a separate phase, it sounds like it could be a while... and until it happens, we'd have this confusing ElementTransition/ElementTransitions situation that dbaron was worried about. Given that this renaming is cosmetic (per bug 999922 comment 1) and not too substantial (just dropping part of the name), I wonder if it might be better to hold off on this renaming & do it *after* ElementTransitions has been removed (at which point the ambiguity will be gone). Would that make sense? (I think you initially were doing the renaming up-front because nothing blocked it, and you'd picked a name (StyleTransition) that didn't collide with anything. However, given that the updated name does sorta have a naming collision for the moment, it seems like it might make sense to defer this part.)

I suppose comment 18 applies to "part 2.1" for ElementAnimation, too, since (as noted in the patch's extended commit message) we already have ElementAnimations (with an "s"), which is also expected to eventually be removed. Given that it seems to have been agreed that ElementAnimation and ElementTransition are appropriate names *except* for this (temporary) one-character-difference naming confusion, and their plural versions are being removed eventually (and hence their name matters less), maybe we could rename the plural versions? (Just the type names; not necessarily variable names) That would make me more comfortable about r+'ing these patches despite comment 6. e.g. s/ElementAnimations/ElementAnimationGroup/ (or Set or List, though those sort of imply a particular underlying data type which we don't actually use)

(In reply to Daniel Holbert [:dholbert] from comment #18) > Hmm... Is that expected to happen imminently? Given that it's in a separate > phase, it sounds like it could be a while... and until it happens, we'd have > this confusing ElementTransition/ElementTransitions situation that dbaron > was worried about. My guess is ~4 weeks. Will anyone else really be touching this part of the code in a big way in that timeframe? My impression is that the code is even more confusing now: ElementPropertyTransition now inherits from StyleAnimation and at the same time nsStyleAnimation already exists. We already had this situation of having both ElementAnimation and ElementAnimations before I "fixed" things: https://hg.mozilla.org/mozilla-central/rev/85114363d5ae (I was ignorant of the existence of nsStyleAnimation at that time so I just followed the suggestion in bug 880596 comment 33). (In reply to Daniel Holbert [:dholbert] from comment #19) > I suppose comment 18 applies to "part 2.1" for ElementAnimation, too, since > (as noted in the patch's extended commit message) we already have > ElementAnimations (with an "s"), which is also expected to eventually be > removed. Yes, it does, but as mentioned above we used to be in exactly that same situation. This patch simply reverts my misguided efforts at tidying up. :)

(In reply to Brian Birtles (:birtles) from comment #20) > My guess is ~4 weeks. Yeah, so 4 weeks feels like a long-ish time for a naming ambiguity to exist, particularly after applying the standard "multiply by two" heuristic for converting coding-project time projections to real-world-times. (and it's always possible some high-priority thing might steal you partway through, too, so it could be longer for various reasons) If the name-collision only lasted on trunk for a few days, then I'd be confident enough that dbaron's concern about it in comment 6 is ignorable. Given that it could last a month or more, though, I'm less sure he'd be fine with that, and I'm not comfortable enough to r+ this on his behalf, at least. > My impression is that the code is even > more confusing now: ElementPropertyTransition now inherits from > StyleAnimation and at the same time nsStyleAnimation already exists. (Perhaps, though I don't see how dropping "Property" would address that.) > > I suppose comment 18 applies to "part 2.1" for ElementAnimation, too > > Yes, it does, but as mentioned above we used to be in exactly that same > situation. This patch simply reverts my misguided efforts at tidying up. :) Gotcha. OK, I'm happy signing off on that patch, then, since it's just a revert.

(In reply to Daniel Holbert [:dholbert] from comment #18) > Given that this renaming is cosmetic (per bug 999922 comment 1) and not too > substantial (just dropping part of the name), I wonder if it might be better > to hold off on this renaming & do it *after* ElementTransitions has been > removed (at which point the ambiguity will be gone). Would that make sense? Filed bug 1010067 for this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b9f80931b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d6badef39f08 https://hg.mozilla.org/integration/mozilla-inbound/rev/5177486bb831 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec965419196d Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c4d71bf8348

https://hg.mozilla.org/mozilla-central/rev/b7b9f80931b8 https://hg.mozilla.org/mozilla-central/rev/d6badef39f08 https://hg.mozilla.org/mozilla-central/rev/5177486bb831 https://hg.mozilla.org/mozilla-central/rev/ec965419196d

Comment on attachment 8420763 [details] [diff] [review] part 2.2 - Rename ElementPropertyTransition to ElementTransition Obsoleting & canceling review on part 2.2, since that ended up getting spun off into bug 1010067 per comment 18 / comment 21 / comment 22.