Web Animations API: Implement step-middle and steps(x, middle) timing functions.

Web Animations API: Implement step-middle and steps(x, middle) timing functions.

step-middle and steps(x, middle) need to work in the Web Animations API but not in CSS. To achieve that we check for step-middle and steps(x, middle) in CSSToStyleMap::mapAnimationTimingFunction, where CSS-land timing functions are set. Step middle functions won’t behave like other invalid inputs; when an animation is made with timing-function: step-middle, or the timing function is set to step-middle after creating the animation, the timing-function value is set to “ease”.

In AbstractPropertySetCSSStyleDeclaration::setPropertyInternal (in PropertySetCSSStyleDeclaration.cpp) there is no good way to check the value of the property to be set, so we just don’t. This means that if you do

    element.style.animationTimingFunction = 'step-middle';

then element.style.animationTimingFunciton will return “step-middle”, but “ease” is used as the actual timing function. This issue is partially mitigated by the fact that we also check RuntimeEnabledFeatures::webAnimationsAPIEnabled at the parsing layer, and don’t parse step middle functions if it’s false.


This CL also removes TimingFunctionTestHelper and TimingFunctionTestHelperTest and  adds TimingFunctionTest.

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

[rjwright, 2014-01-29 07:26:06]

ptal :)

[shans, 2014-01-30 09:55:08]

lgtm

needs review from someone familiar with the parser.

https://codereview.chromium.org/149363002/diff/20001/LayoutTests/animations/a...
File LayoutTests/animations/animations-parsing.html (right):

https://codereview.chromium.org/149363002/diff/20001/LayoutTests/animations/a...
LayoutTests/animations/animations-parsing.html:291: 
Don't need this line

https://codereview.chromium.org/149363002/diff/20001/LayoutTests/animations/t...
File LayoutTests/animations/timing-functions.html (right):

https://codereview.chromium.org/149363002/diff/20001/LayoutTests/animations/t...
LayoutTests/animations/timing-functions.html:46: #box9 {
maybe a comment to indicate that these should act the same way as 'ease', at
least for now?

[dstockwell, 2014-02-03 01:18:18]

lgtm

I'm not sure about the flag in the parser, but can't think of  a better way.

Eric, what do you think?

[Timothy Loh, 2014-02-03 01:49:48]

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1146: bool
BisonCSSParser::parseValue(MutableStylePropertySet* declaration, CSSPropertyID
propertyID, const String& string, bool important, CSSParserMode cssParserMode,
StyleSheetContents* contextStyleSheet, bool parseStepMiddleTimingFunction)
Why not just inline the relevant parts of this function into
parseAnimationTimingFunctionValue up above?

[rjwright, 2014-02-03 02:47:13]

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1146: bool
BisonCSSParser::parseValue(MutableStylePropertySet* declaration, CSSPropertyID
propertyID, const String& string, bool important, CSSParserMode cssParserMode,
StyleSheetContents* contextStyleSheet, bool parseStepMiddleTimingFunction)
On 2014/02/03 01:49:49, Timothy Loh wrote:
> Why not just inline the relevant parts of this function into
> parseAnimationTimingFunctionValue up above?

I could do that. Although nothing else calls the private parseValue except these
static parseValues. And I'd still need the flag on the parser.

[alancutter (OOO until 2018), 2014-02-04 01:19:11]

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:4458: }
The above 8 lines might be cleaner as a switch on v->id.

[alancutter (OOO until 2018), 2014-02-04 02:11:15]

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1146: bool
BisonCSSParser::parseValue(MutableStylePropertySet* declaration, CSSPropertyID
propertyID, const String& string, bool important, CSSParserMode cssParserMode,
StyleSheetContents* contextStyleSheet, bool parseStepMiddleTimingFunction)
Should we add a FIXME to make step middle part of the CSS animation spec so this
parameter can be removed?

[eseidel, 2014-02-05 01:41:17]

I'm about to get on a plane, but as of tomorrow I"ll be on your timezone for a
week and happy to talk more or VC.

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimationsTest.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/Co...
Source/core/animation/CompositorAnimationsTest.cpp:86: m_stepTimingFunction =
StepsTimingFunction::create(1, StepsTimingFunction::StepAtEnd);
Glad to see a move to enums!

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
Source/core/animation/ElementAnimation.cpp:123: RefPtr<CSSValue>
timingFunctionValue =
BisonCSSParser::parseAnimationTimingFunctionValue(timingFunctionString, true);
Boo hiss to bools as args :p

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
File Source/core/animation/ElementAnimationTest.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
Source/core/animation/ElementAnimationTest.cpp:584:
EXPECT_EQ(*(StepsTimingFunction::create(5,
StepsTimingFunction::StepAtMiddle).get()), *timing.timingFunction.get());
I'm surprised we have to do this *foo.get() dance every time.  Maybe we should
add a * to this smart ptr type? Maybe it already has one?

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/CSSTimin...
File Source/core/css/CSSTimingFunctionValue.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/CSSTimin...
Source/core/css/CSSTimingFunctionValue.cpp:52: {
Indent looks wrong.

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:202: ,
m_parseStepMiddleTimingFunction(false)
This can't be right.  We really need to disintangle selector parsing from
tokenization from rule parsing in this class.  No sense in the whole class
carrying state for a single selector parsing algorithm.

[eseidel, 2014-02-07 00:12:34]

I guess I don't understand quite what this CL is trying to do, and why it needs
a bool in the parser.  Could you update the CL description to be longer and
explain more what's being done here. :)  Teh CL descriptions are important for
later review when someone is loking back at your chagne via svn annotate as they
become the SVN commit message.

[rjwright, 2014-02-17 07:16:52]

Just uploading after small changes + rebase.

Still need to sort out that bool arg issue.

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
File Source/core/animation/ElementAnimationTest.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/animation/El...
Source/core/animation/ElementAnimationTest.cpp:584:
EXPECT_EQ(*(StepsTimingFunction::create(5,
StepsTimingFunction::StepAtMiddle).get()), *timing.timingFunction.get());
On 2014/02/05 01:41:18, eseidel wrote:
> I'm surprised we have to do this *foo.get() dance every time.  Maybe we should
> add a * to this smart ptr type? Maybe it already has one?

Done.

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/CSSTimin...
File Source/core/css/CSSTimingFunctionValue.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/CSSTimin...
Source/core/css/CSSTimingFunctionValue.cpp:52: {
On 2014/02/05 01:41:18, eseidel wrote:
> Indent looks wrong.

Done.

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/60001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:4458: }
On 2014/02/04 01:19:12, alancutter wrote:
> The above 8 lines might be cleaner as a switch on v->id.

Done.

[rjwright, 2014-02-19 02:37:51]

This works, except that if you do

elem.style.animationTimingFunction = 'step-middle';
console.log(elem.style.animationTimingFunction);

with experimental features on you get 'step-middle'. But step-middle isn't
actually used as the timing function.

In addition, I might have added redundant checks to make sure this isn't enabled
in CSS. I will try to work it out after lunch.

[alancutter (OOO until 2018), 2014-02-19 03:17:44]

https://codereview.chromium.org/149363002/diff/510001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:4364: return 0;
Avoiding else branches by returning earlier is preferred.
Example:
if (...)
    return 0;
a = b;
...

[dstockwell, 2014-02-24 04:58:39]

Need to update description about how step-middle has been disabled for CSS and
the quirk relating to the inline style.

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1025: if
(stepsTimingFunction->stepAtPosition() != StepsTimingFunction::StepAtMiddle)
Probably better to invert this condition.

Need a fixme about why this is here.

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1027: return
cssValuePool().createIdentifierValue(CSSValueLinear);
Why Linear rather than Ease?

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser.h:94: static PassRefPtr<CSSValueList>
parseFontFaceValue(const AtomicString&);
Appears this was not rebased correctly?

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/resolve...
File Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/resolve...
Source/core/css/resolver/CSSToStyleMap.cpp:506: if (!isStepMiddleFunction)
need a fixme about why

https://codereview.chromium.org/149363002/diff/670001/Source/platform/animati...
File Source/platform/animation/TimingFunction.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/platform/animati...
Source/platform/animation/TimingFunction.cpp:50: // FIXME: Unit test this
thoroughly
Not a good fixme.

[rjwright, 2014-02-24 11:17:46]

https://codereview.chromium.org/149363002/diff/510001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/149363002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:4364: return 0;
On 2014/02/19 03:17:45, alancutter wrote:
> Avoiding else branches by returning earlier is preferred.
> Example:
> if (...)
>     return 0;
> a = b;
> ...

Done.

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1025: if
(stepsTimingFunction->stepAtPosition() != StepsTimingFunction::StepAtMiddle)
On 2014/02/24 04:58:40, dstockwell wrote:
> Probably better to invert this condition.
> 
> Need a fixme about why this is here.

Redundant and removed altogether.

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1027: return
cssValuePool().createIdentifierValue(CSSValueLinear);
On 2014/02/24 04:58:40, dstockwell wrote:
> Why Linear rather than Ease?

Oops. Yes, this should be ease (Although it also seems to be redundant. I can't
make an example that hits this).

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser.h (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser.h:94: static PassRefPtr<CSSValueList>
parseFontFaceValue(const AtomicString&);
On 2014/02/24 04:58:40, dstockwell wrote:
> Appears this was not rebased correctly?

Done.

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/resolve...
File Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/core/css/resolve...
Source/core/css/resolver/CSSToStyleMap.cpp:506: if (!isStepMiddleFunction)
On 2014/02/24 04:58:40, dstockwell wrote:
> need a fixme about why

Done as comment. Do you want it to be a fixme?

https://codereview.chromium.org/149363002/diff/670001/Source/platform/animati...
File Source/platform/animation/TimingFunction.cpp (right):

https://codereview.chromium.org/149363002/diff/670001/Source/platform/animati...
Source/platform/animation/TimingFunction.cpp:50: // FIXME: Unit test this
thoroughly
On 2014/02/24 04:58:40, dstockwell wrote:
> Not a good fixme.

Done.

[rjwright, 2014-02-28 03:48:14]

Test added.

[rjwright, 2014-03-03 05:18:13]

The CQ bit was checked by rjwright@chromium.org

[dstockwell, 2014-03-03 05:18:16]

lgtm

[commit-bot: I haz the power, 2014-03-03 05:18:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1060001

[rjwright, 2014-03-03 05:32:57]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-03 06:54:39]

On 2014/03/03 05:32:57, rjwright wrote:
> The CQ bit was unchecked by mailto:rjwright@chromium.org

Missing platform owners. Will probably also need rebase before merge.

eseidel: ptal

[eseidel, 2014-03-03 07:10:47]

I feel like you've just gotten the run-around on this patch set, and we should
just land something and move on.  But that operator == sure looks strange to my
eyes.  rslgtm.

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
File Source/platform/animation/TimingFunction.cpp (right):

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
Source/platform/animation/TimingFunction.cpp:164: switch (lhs.type()) {
This seems like a really odd way to do this, but I'm not sure I have a better
suggestion.

Why not check the types == each other first? and then have separate compare
functions for both sides being the same type?

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
File Source/platform/animation/TimingFunctionTest.cpp (right):

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
Source/platform/animation/TimingFunctionTest.cpp:280: EXPECT_EQ(1.00,
stepsTimingCustomStart->evaluate(1.00, 0));
I'm surprised we need this many test cases, but OK.

[rjwright, 2014-03-03 08:31:56]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-03 08:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1060001

[rjwright, 2014-03-03 08:38:33]

I'm going to land this as I think dstockwell is going to need it soon.

Will follow up with a patch to clean up equals.

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
File Source/platform/animation/TimingFunction.cpp (right):

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
Source/platform/animation/TimingFunction.cpp:164: switch (lhs.type()) {
On 2014/03/03 07:10:47, eseidel wrote:
> This seems like a really odd way to do this, but I'm not sure I have a better
> suggestion.
> 
> Why not check the types == each other first? and then have separate compare
> functions for both sides being the same type?

I moved this code and haven't walked carefully through its logic for a while.
I'll see if I can clean it up in a follow-up patch.

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
File Source/platform/animation/TimingFunctionTest.cpp (right):

https://codereview.chromium.org/149363002/diff/1060001/Source/platform/animat...
Source/platform/animation/TimingFunctionTest.cpp:280: EXPECT_EQ(1.00,
stepsTimingCustomStart->evaluate(1.00, 0));
On 2014/03/03 07:10:47, eseidel wrote:
> I'm surprised we need this many test cases, but OK.

Yeah these could be reduced to steps(2, x) with a few less samples. This can go
in a follow-up patch to rework equals.

[commit-bot: I haz the power, 2014-03-03 09:34:09]

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

[commit-bot: I haz the power, 2014-03-03 09:34:10]

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

[rjwright, 2014-03-03 23:29:56]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-03 23:30:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1080001

[rjwright, 2014-03-03 23:51:15]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-04 00:31:22]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-04 00:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1100001

[rjwright, 2014-03-04 03:45:07]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-04 03:45:08]

The CQ bit was checked by rjwright@chromium.org

[rjwright, 2014-03-04 03:49:38]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-04 03:50:00]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-04 03:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1100001

[rjwright, 2014-03-04 05:38:10]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-04 05:38:20]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-04 05:38:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1100001

[commit-bot: I haz the power, 2014-03-04 10:22:46]

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

[commit-bot: I haz the power, 2014-03-04 10:22:47]

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

[rjwright, 2014-03-05 00:46:47]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:47:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/149363002/1120001

[rjwright, 2014-03-05 03:01:13]

The CQ bit was unchecked by rjwright@chromium.org

[rjwright, 2014-03-05 03:01:14]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-05 17:54:13]

Change committed as 168500