Web Animations CSS: Assert underlying value is not used

Web Animations CSS: Assert underlying value is not used

This assert makes it clear we don't expect CompositeAdd keyframes to
be used in the CSS transitions and animations implementation.

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

[dstockwell, 2013-08-09 01:12:11]

Compositing doesn't really work at all yet, we don't build and composite the
stack for each property, and we don't require it for CSS, so not sure we need to
do this yet.

https://codereview.chromium.org/22707006/diff/3001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/22707006/diff/3001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:1166: RefPtr<CSSValue>
animatableValue =
iter->value->compositeOnto(iter->value->dependsOnUnderlyingValue() ?
CSSAnimatableValueFactory::create(property, state.style()).get() :
0)->toCSSValue();
Why are we producing a CSSValue now?

[alancutter (OOO until 2018), 2013-08-09 01:21:25]

On 2013/08/09 01:12:11, dstockwell wrote:
> Compositing doesn't really work at all yet, we don't build and composite the
> stack for each property, and we don't require it for CSS, so not sure we need
to
> do this yet.

It's not essential at the moment but it's something small that I noticed that
could be fixed.
Using neutralValue here is not the right thing even as a placeholder.

>
https://codereview.chromium.org/22707006/diff/3001/Source/core/css/resolver/S...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/22707006/diff/3001/Source/core/css/resolver/S...
> Source/core/css/resolver/StyleResolver.cpp:1166: RefPtr<CSSValue>
> animatableValue =
> iter->value->compositeOnto(iter->value->dependsOnUnderlyingValue() ?
> CSSAnimatableValueFactory::create(property, state.style()).get() :
> 0)->toCSSValue();
> Why are we producing a CSSValue now?

Whoops! Bad rebase, ptal. :)

[dstockwell, 2013-08-09 01:49:58]

On 2013/08/09 01:21:25, alancutter wrote:
> It's not essential at the moment but it's something small that I noticed that
> could be fixed.
> Using neutralValue here is not the right thing even as a placeholder.

I think I'd rather just ASSERT that we don't depend on the underlying value
(compositing not yet implemented), we probably should have just done that in the
first place.

[alancutter (OOO until 2018), 2013-08-09 04:02:06]

On 2013/08/09 01:49:58, dstockwell wrote:
> On 2013/08/09 01:21:25, alancutter wrote:
> > It's not essential at the moment but it's something small that I noticed
that
> > could be fixed.
> > Using neutralValue here is not the right thing even as a placeholder.
> 
> I think I'd rather just ASSERT that we don't depend on the underlying value
> (compositing not yet implemented), we probably should have just done that in
the
> first place.

ASSERTing now instead of using underlying value.

[dstockwell, 2013-08-09 04:06:22]

lgtm

[Steve Block, 2013-08-09 04:32:31]

lgtm

The commit message is now quite misleading, so should be updated!

https://codereview.chromium.org/22707006/diff/17001/Source/core/animation/Key...
File Source/core/animation/KeyframeAnimationEffectTest.cpp (right):

https://codereview.chromium.org/22707006/diff/17001/Source/core/animation/Key...
Source/core/animation/KeyframeAnimationEffectTest.cpp:360:
EXPECT_TRUE(effect->sample(0,
0.25)->begin()->value->dependsOnUnderlyingValue());
Testing at 0.25 is a little confusing - it implies there's something special
about the half-way point. We expect the value to be true all the way up to
0.499999, right?

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

https://codereview.chromium.org/22707006/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1165:
ASSERT_WITH_MESSAGE(!iter->value->dependsOnUnderlyingValue(), "Not yet
implemented: An interface for compositing onto the underlying value.");
Should this be a release assert? I thought we agreed on using release assert for
these 'not yet implemented' checks, to simplify testing across debug and release
builds.

[alancutter (OOO until 2018), 2013-08-09 05:46:30]

https://codereview.chromium.org/22707006/diff/17001/Source/core/animation/Key...
File Source/core/animation/KeyframeAnimationEffectTest.cpp (right):

https://codereview.chromium.org/22707006/diff/17001/Source/core/animation/Key...
Source/core/animation/KeyframeAnimationEffectTest.cpp:360:
EXPECT_TRUE(effect->sample(0,
0.25)->begin()->value->dependsOnUnderlyingValue());
On 2013/08/09 04:32:31, Steve Block wrote:
> Testing at 0.25 is a little confusing - it implies there's something special
> about the half-way point. We expect the value to be true all the way up to
> 0.499999, right?

Added more stops to test.

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

https://codereview.chromium.org/22707006/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1165:
ASSERT_WITH_MESSAGE(!iter->value->dependsOnUnderlyingValue(), "Not yet
implemented: An interface for compositing onto the underlying value.");
On 2013/08/09 04:32:31, Steve Block wrote:
> Should this be a release assert? I thought we agreed on using release assert
for
> these 'not yet implemented' checks, to simplify testing across debug and
release
> builds.

Done.

[alancutter (OOO until 2018), 2013-08-09 05:46:50]

On 2013/08/09 04:32:31, Steve Block wrote:
> The commit message is now quite misleading, so should be updated!
> 

Good catch! Updated.

[commit-bot: I haz the power, 2013-08-09 06:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/22707006/24001

[commit-bot: I haz the power, 2013-08-09 18:28:40]

Change committed as 155855