Web Animations: Use legacy cascade behaviour and the MatchedPropertiesCache

Web Animations: Use legacy cascade behaviour and the MatchedPropertiesCache

This patch changes the behaviour of the Web Animations backed animations
and transitions implementation to apply animations / transitions at the
last step of the cascade. This is the same behaviour as the legacy code
but not correct according to the spec[1]. The current behaviour is a bit
strange, it attempts to follow the spec but property dependencies (such
as using ems) do not work correctly as low priority properties will only
be re-applied if they are !important and we are running animations on a
low priority property.

As a result of moving animations / transitions to the end of the cascade
we can easily use the MatchedPropertiesCache for all running animations
and transitions. Once we correct the cascade order we'll need to disable
the cache in several cases.

[1] http://www.w3.org/TR/css3-cascade/#cascading

BUG=232273, 314952
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161985

[esprehn, 2013-11-13 07:37:42]

This needs a test, it seems you're fixing something related to ems?

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
File
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt
(left):

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt:2:
PASS - "background-color" property for "test" element at 0.25s saw something
close to: rgb(0, 128, 0)
Why are you deleting an expected result but not disabling any test or changing
any tests?

[Timothy Loh, 2013-11-13 23:12:14]

This patch won't fix behaviour in any case, it actually makes things less
correct. We decided to revert to the old behaviour before we turn on the
web-animations implementation since what we currently have only half works and
may end up being confusing to developers.

The legacy behaviour (that this patch makes us match) is to have
animations/transitions apply after applying all other rules and as a result the
property dependencies don't work at all. The behaviour we're removing in this
patch has !important rules overriding animations as per spec, but property
dependencies only work in restricted cases (see patch description).

We of course intend to fix our behaviour to match the spec in the future, but we
don't want to block turning on the flag on this.

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
File
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt
(left):

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt:2:
PASS - "background-color" property for "test" element at 0.25s saw something
close to: rgb(0, 128, 0)
On 2013/11/13 07:37:43, esprehn wrote:
> Why are you deleting an expected result but not disabling any test or changing
> any tests?

I'm not sure what the standard thing to do here is; the test has an expected.txt
in LayoutTests/animations with FAIL lines in it, which we will match after this
change. Should I remove the test until we have the cascade working correctly?

[dstockwell, 2013-11-13 23:19:50]

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
File
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt
(left):

https://codereview.chromium.org/71083002/diff/1/LayoutTests/virtual/web-anima...
LayoutTests/virtual/web-animations-css/animations/cascade-important-expected.txt:2:
PASS - "background-color" property for "test" element at 0.25s saw something
close to: rgb(0, 128, 0)
On 2013/11/13 23:12:14, Timothy Loh wrote:
> On 2013/11/13 07:37:43, esprehn wrote:
> > Why are you deleting an expected result but not disabling any test or
changing
> > any tests?
> 
> I'm not sure what the standard thing to do here is; the test has an
expected.txt
> in LayoutTests/animations with FAIL lines in it, which we will match after
this
> change. Should I remove the test until we have the cascade working correctly?

No, keeping FAIL expectations is fine. 232273 will track this.

[dstockwell, 2013-11-13 23:37:19]

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1198: applyAnimatedOnly =
element->hasActiveAnimations()
Would it be cleaner to pull out the application of animated properties into a
separate function and just call that here (and below)?

[Steve Block, 2013-11-14 02:17:16]

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1197: if
(RuntimeEnabledFeatures::webAnimationsEnabled()) {
This should probably be WebAnimationsCSSEnabled()

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1198: applyAnimatedOnly =
element->hasActiveAnimations()
Agreed

[dstockwell, 2013-11-14 02:59:48]

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1197: if
(RuntimeEnabledFeatures::webAnimationsEnabled()) {
On 2013/11/14 02:17:18, Steve Block wrote:
> This should probably be WebAnimationsCSSEnabled()

It's a bit mixed up, we could just kill the 'WebAnimations' runtime flag and
block enabling the other web animations features on having enabled
WebAnimationsCSS.

Either way will work for now. We can remove it in a separate patch.

[Timothy Loh, 2013-11-14 03:06:31]

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1197: if
(RuntimeEnabledFeatures::webAnimationsEnabled()) {
On 2013/11/14 02:59:49, dstockwell wrote:
> On 2013/11/14 02:17:18, Steve Block wrote:
> > This should probably be WebAnimationsCSSEnabled()
> 
> It's a bit mixed up, we could just kill the 'WebAnimations' runtime flag and
> block enabling the other web animations features on having enabled
> WebAnimationsCSS.
> 
> Either way will work for now. We can remove it in a separate patch.

Erm; changed the callers in this file anyway.

https://codereview.chromium.org/71083002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1198: applyAnimatedOnly =
element->hasActiveAnimations()
On 2013/11/13 23:37:19, dstockwell wrote:
> Would it be cleaner to pull out the application of animated properties into a
> separate function and just call that here (and below)?

Done. I wasn't sure if this would make it cleaner, but given that it makes the
diff much simpler I'm pretty sure this is better.

[Steve Block, 2013-11-14 03:31:18]

lgtm

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1060: if (state.animationUpdate()) {
Early-out?

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1353:
ASSERT(!state.fontBuilder().fontDirty());
Did you intend to move this assert below the caching?

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.h:259: bool
applyAnimatedProperties(StyleResolverState&, const
AnimationEffect::CompositableValueMap&);
Do we need the return value any more?

[Timothy Loh, 2013-11-14 03:54:01]

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1060: if (state.animationUpdate()) {
On 2013/11/14 03:31:18, Steve Block wrote:
> Early-out?

Done.

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1353:
ASSERT(!state.fontBuilder().fontDirty());
On 2013/11/14 03:31:18, Steve Block wrote:
> Did you intend to move this assert below the caching?

Yep; in particular below applying animated properties.

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.h:259: bool
applyAnimatedProperties(StyleResolverState&, const
AnimationEffect::CompositableValueMap&);
On 2013/11/14 03:31:18, Steve Block wrote:
> Do we need the return value any more?

Removed. I guess we don't need the StyleApplicationPass anymore either, but I'll
leave that in for now.

[Steve Block, 2013-11-14 04:07:32]

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/71083002/diff/210001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1353:
ASSERT(!state.fontBuilder().fontDirty());
OK, makes sense

[dstockwell, 2013-11-14 04:19:26]

lgtm

[commit-bot: I haz the power, 2013-11-14 04:21:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timloh@chromium.org/71083002/250001

[commit-bot: I haz the power, 2013-11-14 05:21:37]

Change committed as 161985