Optimize style recalc when adding @keyframes.

Optimize style recalc when adding @keyframes.

We only need to recalculate the elements with running animations unless
we tried to find an @keyframes rule and couldn't, for which we fall
back to a full document recalc.

The motivation for doing this is that the current WIP for issue 567021
shows that we're still getting full document recalcs due to the
presence of @keyframes rules.

R=dstockwell@chromium.org,alancutter@chromium.org
BUG=623911, 567021
Committed: https://crrev.com/9c88a15f059aec65af294b8789a27cc377363da6
Cr-Commit-Position: refs/heads/master@{#403118}

[rune, 2016-06-28 10:49:12]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 10:49:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rune, 2016-06-28 11:36:35]

ptal

[commit-bot: I haz the power, 2016-06-28 12:00:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 12:00:03]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2016-06-29 03:41:32]

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/AnimationTimeline.cpp:75: ,
m_lastCurrentTimeInternal(0)
We should initialise m_hasUnresolvedKeyframesRule here.

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/AnimationTimeline.h:115: bool
m_hasUnresolvedKeyframesRule;
Could this live on Document instead? I don't think CSS specific concepts should
live on the AnimationTimeline.

[alancutter (OOO until 2018), 2016-06-29 08:10:21]

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/AnimationTimeline.h:115: bool
m_hasUnresolvedKeyframesRule;
On 2016/06/29 at 03:41:32, alancutter wrote:
> Could this live on Document instead? I don't think CSS specific concepts
should live on the AnimationTimeline.

One option to keep it scoped to animations is to make DocumentAnimation a member
of Document and put it in there.

[rune, 2016-06-29 08:29:12]

On 2016/06/29 08:10:21, alancutter wrote:
>
https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/animation/AnimationTimeline.h (right):
> 
>
https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/animation/AnimationTimeline.h:115: bool
> m_hasUnresolvedKeyframesRule;
> On 2016/06/29 at 03:41:32, alancutter wrote:
> > Could this live on Document instead? I don't think CSS specific concepts
> should live on the AnimationTimeline.
> 
> One option to keep it scoped to animations is to make DocumentAnimation a
member
> of Document and put it in there.

I actually thought about putting it in StyleEngine first.

[rune, 2016-06-29 09:48:06]

On 2016/06/29 08:10:21, alancutter wrote:
>
https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/animation/AnimationTimeline.h (right):
> 
>
https://codereview.chromium.org/2105743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/animation/AnimationTimeline.h:115: bool
> m_hasUnresolvedKeyframesRule;
> On 2016/06/29 at 03:41:32, alancutter wrote:
> > Could this live on Document instead? I don't think CSS specific concepts
> should live on the AnimationTimeline.
> 
> One option to keep it scoped to animations is to make DocumentAnimation a
member
> of Document and put it in there.

As far as I could figure out, the timeline is the only place I can find all
current KeyframeEffect without traversing the DOM tree, looking up
elementAnimations. It would be good not having to traverse the DOM tree. Do you
think it's acceptable to still have a for-loop in the timeline marking the
target elements for KeyframeEffect with setNeedsStyleRecalc, or should the
timeline return the current KeyframeEffects/targets and make StyleEngine call
setNeedsStyleRecalc or something?

[rune, 2016-06-29 11:16:46]

I've moved the logic out of AnimationTimeline. Does this look acceptable?

[rune, 2016-06-29 11:17:16]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 11:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2016-06-29 12:41:53]

I'm concerned about the number of references to KeyframeEffect present in
Animation.cpp but that's something for the animation team to deal with.

lgtm with test nits.
Please wait for dstockwell's review.

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html
(right):

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html:39: }
We should test that adding a second "@keyframes later" performs the style
recalc.

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html:60:
assert_equals(internals.updateStyleAndReturnAffectedElementCount(), 3,
"Incorrect recalc count.")
I don't agree with using failure messages for the the assertion descriptions,
it's inconsistent with all the other testharness.js tests and makes the test
code harder to read.

[commit-bot: I haz the power, 2016-06-29 14:10:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 14:10:23]

Dry run: This issue passed the CQ dry run.

[rune, 2016-06-29 21:09:46]

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html
(right):

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html:39: }
On 2016/06/29 12:41:53, alancutter wrote:
> We should test that adding a second "@keyframes later" performs the style
> recalc.

Done.

https://codereview.chromium.org/2105743002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/add-keyframes-recalc.html:60:
assert_equals(internals.updateStyleAndReturnAffectedElementCount(), 3,
"Incorrect recalc count.")
On 2016/06/29 12:41:53, alancutter wrote:
> I don't agree with using failure messages for the the assertion descriptions,
> it's inconsistent with all the other testharness.js tests and makes the test
> code harder to read.

Done.

[rune, 2016-06-29 21:14:54]

On 2016/06/29 12:41:53, alancutter wrote:
> I'm concerned about the number of references to KeyframeEffect present in
> Animation.cpp but that's something for the animation team to deal with.

Yes, my change falls into a pattern there.

> lgtm with test nits.
> Please wait for dstockwell's review.

Will do.

[dstockwell, 2016-06-30 00:46:59]

lgtm

[rune, 2016-06-30 05:31:30]

The CQ bit was checked by rune@opera.com

[rune, 2016-06-30 05:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2105743002/#ps40001
(title: "Test review issues")

[commit-bot: I haz the power, 2016-06-30 05:31:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 08:10:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-30 08:12:15]

Description was changed from

==========
Optimize style recalc when adding @keyframes.

We only need to recalculate the elements with running animations unless
we tried to find an @keyframes rule and couldn't, for which we fall
back to a full document recalc.

The motivation for doing this is that the current WIP for issue 567021
shows that we're still getting full document recalcs due to the
presence of @keyframes rules.

R=dstockwell@chromium.org,alancutter@chromium.org
BUG=623911,567021
==========

to

==========
Optimize style recalc when adding @keyframes.

We only need to recalculate the elements with running animations unless
we tried to find an @keyframes rule and couldn't, for which we fall
back to a full document recalc.

The motivation for doing this is that the current WIP for issue 567021
shows that we're still getting full document recalcs due to the
presence of @keyframes rules.

R=dstockwell@chromium.org,alancutter@chromium.org
BUG=623911,567021

Committed: https://crrev.com/9c88a15f059aec65af294b8789a27cc377363da6
Cr-Commit-Position: refs/heads/master@{#403118}
==========

[commit-bot: I haz the power, 2016-06-30 08:12:17]

Patchset 3 (id:??) landed as
https://crrev.com/9c88a15f059aec65af294b8789a27cc377363da6
Cr-Commit-Position: refs/heads/master@{#403118}