Rename Animation -> PrimitiveAnimation

Rename Animation -> CSSAnimationData

platform/animation/Animation is a low-level Animation primitive used to communicate
animation information to cc.

We'd like to build a new Animation class that mediates CSS Transitions and Animations, as well as SVG Animations eventually.
 
It would be better to call the new thing 'Animation'.

Please also see https://docs.google.com/a/chromium.org/document/d/1957vKRcyQebygJR2bqdASy3KWZzHz-LPcr71hCd4QDw

BUG=234784
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149305

[dglazkov, 2013-04-23 02:02:51]

What's a PrimitiveAnimation?

[eseidel, 2013-04-23 04:00:46]

It's what the caveman did!  It involves charcoal.

[shans, 2013-04-23 06:24:54]

Hi,

I clearly don't know how to code review. I've added the description from the
attached bug to this issue.

[dglazkov, 2013-04-23 16:52:19]

On 2013/04/23 06:24:54, shans wrote:
> Hi,
> 
> I clearly don't know how to code review. I've added the description from the
> attached bug to this issue.

I think you're trying to match bug-per-codereview thing we did in WebKit, which
is going to be exhausting for ya :) May I suggest just filing one "Refactor
existing Animation code bug" and landing a bunch of patches for that bug?

[dglazkov, 2013-04-23 16:55:30]

https://codereview.chromium.org/14391005/diff/1/Source/core/platform/animatio...
File Source/core/platform/animation/PrimitiveAnimation.h (right):

https://codereview.chromium.org/14391005/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/PrimitiveAnimation.h:37: class PrimitiveAnimation
: public RefCounted<PrimitiveAnimation> {
If it's a primitive for animation, shouldn't it be AnimationPrimitive? :)

[jamesr, 2013-04-23 16:56:39]

"platform/animation/Animation is a low-level Animation primitive used to
communicate animation information to cc. "

This seems backwards.  The basic type in Blink called 'Animation' should be the
thing that deals with animations in blink.  The thing used to interface with the
compositor implementation is a more specialized thing and should get the longer,
less generic name.  It can only be used for a small subset of animation types,
after all.

[shans, 2013-04-23 21:56:07]

On 2013/04/23 16:56:39, jamesr wrote:
> "platform/animation/Animation is a low-level Animation primitive used to
> communicate animation information to cc. "
> 
> This seems backwards.  The basic type in Blink called 'Animation' should be
the
> thing that deals with animations in blink.  The thing used to interface with
the
> compositor implementation is a more specialized thing and should get the
longer,
> less generic name.  It can only be used for a small subset of animation types,
> after all.

As things stand right now, Animation objects are data objects, which are
constructed and filled out by StyleBuilder when generating computed styles for
elements.  They're sent to the compositor via GraphicsLayer::addAnimation. This
method creates WebKit::WebAnimation objects interally.

core/page/animation/AnimationBase is responsible for animating (it generates
animation samples, etc.). Each AnimationBase object has an Animation data
object.

SVG animations use a different path and don't share any code with CSS
animations.

At the moment, then, AnimationBase is the thing that deals with CSS animations
in blink, while Animation objects are data objects that are used to store
information for CSS animations destined for both the main-thread and the
compositor.

What we want to do is rename Animation so that we can build a replacement for
AnimationBase which supports both CSS and SVG animations. We think the
replacement should be called 'Animation' because:
(1) it will be the general Animation abstraction for Blink
(2) once we start working on the Web Animations JavaScript interface, these
objects will be exposed to JavaScript as Animation objects.

But what should we rename Animation to? I'd suggested 'PrimitiveAnimation'.
Dmitri thinks 'AnimationPrimitive' would be better. Looking at the current role
of Animation, another option would be 'AnimationData'.

[jamesr, 2013-04-23 22:04:58]

On 2013/04/23 21:56:07, shans wrote:
> On 2013/04/23 16:56:39, jamesr wrote:
> > "platform/animation/Animation is a low-level Animation primitive used to
> > communicate animation information to cc. "
> > 
> > This seems backwards.  The basic type in Blink called 'Animation' should be
> the
> > thing that deals with animations in blink.  The thing used to interface with
> the
> > compositor implementation is a more specialized thing and should get the
> longer,
> > less generic name.  It can only be used for a small subset of animation
types,
> > after all.
> 
> As things stand right now, Animation objects are data objects, which are
> constructed and filled out by StyleBuilder when generating computed styles for
> elements.  They're sent to the compositor via GraphicsLayer::addAnimation.
This
> method creates WebKit::WebAnimation objects interally.

Only a small subset of animations go into GraphicsLayer::addAnimation.  Most CSS
animations never touch GraphicsLayer.

> SVG animations use a different path and don't share any code with CSS
> animations.
> 
> At the moment, then, AnimationBase is the thing that deals with CSS animations
> in blink, while Animation objects are data objects that are used to store
> information for CSS animations destined for both the main-thread and the
> compositor.

I think you're overstating the prevalence of the compositor in all this.

> What we want to do is rename Animation so that we can build a replacement for
> AnimationBase which supports both CSS and SVG animations. We think the
> replacement should be called 'Animation' because:
> (1) it will be the general Animation abstraction for Blink

Isn't Animation already the general animation abstraction for blink?  Or is your
issue that WebCore::Animation is CSS-specific today and you want something that
generalizes to SVG?  If that's the concern, then maybe calling it CSSAnimation
is better.

> (2) once we start working on the Web Animations JavaScript interface, these
> objects will be exposed to JavaScript as Animation objects.
> 
> But what should we rename Animation to? I'd suggested 'PrimitiveAnimation'.
> Dmitri thinks 'AnimationPrimitive' would be better. Looking at the current
role
> of Animation, another option would be 'AnimationData'.

[esprehn, 2013-04-23 22:12:52]

I fixed the description on this codereview. BUG= lines go last, and there's no
reason to put weird quotations and such in the message. The message on here is
what ends up in the git log, so it should be like the ChangeLog message from the
webkit days. :)

[shans, 2013-04-23 22:13:23]

On 2013/04/23 22:04:58, jamesr wrote:
> On 2013/04/23 21:56:07, shans wrote:
> > On 2013/04/23 16:56:39, jamesr wrote:
> > > "platform/animation/Animation is a low-level Animation primitive used to
> > > communicate animation information to cc. "
> > > 
> > > This seems backwards.  The basic type in Blink called 'Animation' should
be
> > the
> > > thing that deals with animations in blink.  The thing used to interface
with
> > the
> > > compositor implementation is a more specialized thing and should get the
> > longer,
> > > less generic name.  It can only be used for a small subset of animation
> types,
> > > after all.
> > 
> > As things stand right now, Animation objects are data objects, which are
> > constructed and filled out by StyleBuilder when generating computed styles
for
> > elements.  They're sent to the compositor via GraphicsLayer::addAnimation.
> This
> > method creates WebKit::WebAnimation objects interally.
> 
> Only a small subset of animations go into GraphicsLayer::addAnimation.  Most
CSS
> animations never touch GraphicsLayer.

I think all animations call addAnimation, but mostly addAnimation returns false
rather than passing the animation down to the compositor? At any rate, this
isn't that relevant to what we're trying to do.

> > SVG animations use a different path and don't share any code with CSS
> > animations.
> > 
> > At the moment, then, AnimationBase is the thing that deals with CSS
animations
> > in blink, while Animation objects are data objects that are used to store
> > information for CSS animations destined for both the main-thread and the
> > compositor.
> 
> I think you're overstating the prevalence of the compositor in all this.

Forget the compositor :) The important thing here is that Animation is a data
object, while AnimationBase objects actually run the animations.

> > What we want to do is rename Animation so that we can build a replacement
for
> > AnimationBase which supports both CSS and SVG animations. We think the
> > replacement should be called 'Animation' because:
> > (1) it will be the general Animation abstraction for Blink
> 
> Isn't Animation already the general animation abstraction for blink?  Or is
your
> issue that WebCore::Animation is CSS-specific today and you want something
that
> generalizes to SVG?  If that's the concern, then maybe calling it CSSAnimation
> is better.

AnimationBase is the general animation abstraction for blink (at least for CSS).
Contained within each
AnimationBase is an Animation data object.

I have no problems with the idea of renaming Animation to CSSAnimation, though
actually I'm leaning towards AnimationData as being a more descriptive name.
Maybe CSSAnimationData? Dmitri, what do you think?

> > (2) once we start working on the Web Animations JavaScript interface, these
> > objects will be exposed to JavaScript as Animation objects.
> > 
> > But what should we rename Animation to? I'd suggested 'PrimitiveAnimation'.
> > Dmitri thinks 'AnimationPrimitive' would be better. Looking at the current
> role
> > of Animation, another option would be 'AnimationData'.

[dglazkov, 2013-04-23 22:14:47]

On 2013/04/23 22:13:23, shans wrote:
> Maybe CSSAnimationData? Dmitri, what do you think?

Purple. Though CSSAnimationData is fine.

[shans, 2013-04-23 23:24:45]

On 2013/04/23 22:14:47, Dimitri Glazkov wrote:
> On 2013/04/23 22:13:23, shans wrote:
> > Maybe CSSAnimationData? Dmitri, what do you think?
> 
> Purple. Though CSSAnimationData is fine.

OK, updated the patch to rename Animation to Purple.

[dglazkov, 2013-04-24 02:41:34]

On 2013/04/23 23:24:45, shans wrote:
> OK, updated the patch to rename Animation to Purple.

YOU TRICK ME.

[dglazkov, 2013-04-24 02:41:57]

LGTM.

[commit-bot: I haz the power, 2013-04-28 21:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/14391005/22001

[commit-bot: I haz the power, 2013-04-28 22:01:32]

Change committed as 149305

[eseidel, 2013-04-29 07:53:17]

CSS* normally are for CSSOM objects.  Is this part of the CSSOM?  I don't see an
IDL.  Style* is what our interal objects more commonly use.

[eseidel, 2013-04-29 07:53:57]

(I'm not saying this is wrong, btw, just attempting to provide some context.)

[dglazkov, 2013-04-29 16:13:21]

On 2013/04/29 07:53:57, Eric Seidel (Google) wrote:
> (I'm not saying this is wrong, btw, just attempting to provide some context.)

No, you're right. The convention is a bit hokey, but we should stick with it
until we have a new convention.

[shans, 2013-04-29 21:05:30]

On 2013/04/29 16:13:21, Dimitri Glazkov wrote:
> On 2013/04/29 07:53:57, Eric Seidel (Google) wrote:
> > (I'm not saying this is wrong, btw, just attempting to provide some
context.)
> 
> No, you're right. The convention is a bit hokey, but we should stick with it
> until we have a new convention.

https://codereview.chromium.org/14409013/