Refactoring animation code in accelerated path.

Refactoring animation code in accelerated path.

Introduce WebAnimationProvider to extract animation logic from
RenderLayerBacking and GraphicsLayer.

The goals of this CL are
1. Get together animation logic in one file to increase readability.
2. Resolve layer violation of GraphicsLayer that includes CSS.

BUG=N/A

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

[shawnsingh, 2013-09-19 12:57:37]

I like it!  Most the changes seem fine to me.  Vollick may be a good reviewer
since he has dealt with this animation code a lot.

Two general comments:

(1) You may wish to see this old bug: 
https://bugs.webkit.org/show_bug.cgi?id=94993 - That issue never really got
fixed, but it happens not to cause any bugs just because other code prevents
that scenario from happening for now.  Here might be a good opportunity to
assimilate that patch (assuming it's correct to do so).

(2) Maybe not in this patch, but overall I would like to see us move towards a
design where the render tree / animation control communicates directly to the
GraphicsLayer instead of in-between through RenderLayerBacking.

[dshwang, 2013-09-19 14:19:55]

On 2013/09/19 12:57:37, shawnsingh wrote:

Thank you for your review.

> I like it!  Most the changes seem fine to me.  Vollick may be a good reviewer
> since he has dealt with this animation code a lot.
> 
> Two general comments:
> 
> (1) You may wish to see this old bug: 
> https://bugs.webkit.org/show_bug.cgi?id=94993 - That issue never really got
> fixed, but it happens not to cause any bugs just because other code prevents
> that scenario from happening for now.  Here might be a good opportunity to
> assimilate that patch (assuming it's correct to do so).

This CL does not change any logic. This CL is just refactoring CL.
I think it is better to apply your webkit patch in another CL.
I you want, I can do it.

> (2) Maybe not in this patch, but overall I would like to see us move towards a
> design where the render tree / animation control communicates directly to the
> GraphicsLayer instead of in-between through RenderLayerBacking.

Now RenderLayerBacking uses WebAnimationProvider because WebAnimationProvider
has the dependency on CSS.
This CL makes it easy to move the animation logic to GraphicsLayer. If we want,
we can change that WebAnimationProvider is used by GraphicsLayer, not
WebAnimationProvider in the future.

[dshwang, 2013-09-19 14:20:58]

@vollick, could you review?

[Ian Vollick, 2013-09-19 15:04:30]

lgtm. Nice refactor.

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
File Source/core/rendering/animation/WebAnimationProvider.cpp (right):

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
Source/core/rendering/animation/WebAnimationProvider.cpp:61: return
AnimatedPropertyBackgroundColor;
Are we accelerating this already?

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
Source/core/rendering/animation/WebAnimationProvider.cpp:156:
resultAnimations.m_transformAnimation = appendWebAnimation(transformVector,
boxSize, anim, keyframes.animationName(), timeOffset);
appendWebAnimation feels a bit confusing; I can't tell what it's appending to.
Maybe createWebAnimation?

[dshwang, 2013-09-19 15:56:48]

Thank you for lgtm.

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
File Source/core/rendering/animation/WebAnimationProvider.cpp (right):

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
Source/core/rendering/animation/WebAnimationProvider.cpp:61: return
AnimatedPropertyBackgroundColor;
On 2013/09/19 15:04:30, Ian Vollick wrote:
> Are we accelerating this already?

Code looks like accelerated.
void GraphicsLayer::setBackgroundColor(const Color& color)
{
    if (color == m_backgroundColor)
        return;

    m_backgroundColor = color;
    m_layer->layer()->setBackgroundColor(m_backgroundColor.rgb());
}

https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
Source/core/rendering/animation/WebAnimationProvider.cpp:156:
resultAnimations.m_transformAnimation = appendWebAnimation(transformVector,
boxSize, anim, keyframes.animationName(), timeOffset);
On 2013/09/19 15:04:30, Ian Vollick wrote:
> appendWebAnimation feels a bit confusing; I can't tell what it's appending to.
> Maybe createWebAnimation?

I'll rename createWebAnimation. appendWebAnimation appends id to HashMap so I
named appendWebAnimation. but it is a bit confusing because it appends the id,
not WebAnimation.

[dshwang, 2013-09-19 16:00:05]

On 2013/09/19 15:56:48, dshwang wrote:
> Thank you for lgtm.
> 
>
https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
> File Source/core/rendering/animation/WebAnimationProvider.cpp (right):
> 
>
https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
> Source/core/rendering/animation/WebAnimationProvider.cpp:61: return
> AnimatedPropertyBackgroundColor;
> On 2013/09/19 15:04:30, Ian Vollick wrote:
> > Are we accelerating this already?
> 
> Code looks like accelerated.
> void GraphicsLayer::setBackgroundColor(const Color& color)
> {
>     if (color == m_backgroundColor)
>         return;
> 
>     m_backgroundColor = color;
>     m_layer->layer()->setBackgroundColor(m_backgroundColor.rgb());
> }

Currently, cc accelerates only Transform and Opacity. cc needs to accelerates
Filter and BackgroundColor.

[Ian Vollick, 2013-09-19 16:03:20]

On 2013/09/19 16:00:05, dshwang wrote:
> On 2013/09/19 15:56:48, dshwang wrote:
> > Thank you for lgtm.
> > 
> >
>
https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
> > File Source/core/rendering/animation/WebAnimationProvider.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23431021/diff/1/Source/core/rendering/animati...
> > Source/core/rendering/animation/WebAnimationProvider.cpp:61: return
> > AnimatedPropertyBackgroundColor;
> > On 2013/09/19 15:04:30, Ian Vollick wrote:
> > > Are we accelerating this already?
> > 
> > Code looks like accelerated.
> > void GraphicsLayer::setBackgroundColor(const Color& color)
> > {
> >     if (color == m_backgroundColor)
> >         return;
> > 
> >     m_backgroundColor = color;
> >     m_layer->layer()->setBackgroundColor(m_backgroundColor.rgb());
> > }
> 
> Currently, cc accelerates only Transform and Opacity. cc needs to accelerates
> Filter and BackgroundColor.

Yep, this is on our radar. ajuma@'s working on accelerating filter animations
and we were going to tackle background color soon.

[dshwang, 2013-09-19 16:10:47]

Hi, could you review again. There are little change to fix nits.

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
File Source/core/rendering/animation/WebAnimationProvider.cpp (right):

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
Source/core/rendering/animation/WebAnimationProvider.cpp:64:
ASSERT_NOT_REACHED(); // Chromiumn compositor cannot accelerate filter yet.
Add ASSERT

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
Source/core/rendering/animation/WebAnimationProvider.cpp:207:
OwnPtr<WebAnimation> webAnimation(createWebAnimation(values, animation,
animationId, timeOffset, boxSize));
rename createWebAnimationAndStoreId.
I cannot use createWebAnimation, because createWebAnimation already exists in
AnimationTranslationUtil.h

[Ian Vollick, 2013-09-19 17:00:18]

Thanks, slgtm.

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
File Source/core/rendering/animation/WebAnimationProvider.cpp (right):

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
Source/core/rendering/animation/WebAnimationProvider.cpp:64:
ASSERT_NOT_REACHED(); // Chromiumn compositor cannot accelerate filter yet.
On 2013/09/19 16:10:48, dshwang wrote:
> Add ASSERT

typo: s/Chromiumn/Chromium/

https://codereview.chromium.org/23431021/diff/19001/Source/core/rendering/ani...
Source/core/rendering/animation/WebAnimationProvider.cpp:207:
OwnPtr<WebAnimation> webAnimation(createWebAnimation(values, animation,
animationId, timeOffset, boxSize));
On 2013/09/19 16:10:48, dshwang wrote:
> rename createWebAnimationAndStoreId.
> I cannot use createWebAnimation, because createWebAnimation already exists in
> AnimationTranslationUtil.h
sgtm.

[commit-bot: I haz the power, 2013-09-19 17:47:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/23431021/23001

[dshwang, 2013-09-19 17:47:09]

On 2013/09/19 17:00:18, Ian Vollick wrote:
> typo: s/Chromiumn/Chromium/

Oops, Thank you :)

[commit-bot: I haz the power, 2013-09-19 19:55:56]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-19 20:27:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/23431021/23001

[commit-bot: I haz the power, 2013-09-19 21:02:50]

Change committed as 158052