Web Animations API: Refactor IDL input conversion out of Animation

Web Animations API: Refactor IDL input conversion out of Animation

This change moves effect and timing conversion logic out of Animation
and into EffectInput and TimingInput respectively.

This refactor includes a minor change in behaviour:
Animations created on elements without an active document renderer will no
longer return null, instead the animation's effect will be null.
This behaviour still avoids calls to the CSS parser and style resolver if
the check fails.

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

[dstockwell, 2014-02-27 09:49:36]

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:44: static Animation* animate(Element&
element, E effect, T timing = Timing::initialIterationDuration())
Body should stay in cpp.

I think this would work better as:

static Animation* animate(Element& element, E effect)
{
    animate(..., Timing::defaults().iterationDuration);
}

static Animation* animate(Element& element, E effect, T timing)
{
    RefPtr<Animation> animation = Animation::create(&element,
EffectInput::convert(effect), TimingInput::convert(timing));
}

Then we can remove the additional create() functions from Animation and separate
out the different types of input conversion.

[shans, 2014-02-27 20:39:33]

lgtm with Doug's suggested change.

Would be worth syncing up with Tim, who has a similar problem wrt custom
effects.

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:44: static Animation* animate(Element&
element, E effect, T timing = Timing::initialIterationDuration())
On 2014/02/27 09:49:36, dstockwell wrote:
> Body should stay in cpp.

Body can't remain in cpp. This is the joy, the wonder, the brightly illuminating
splendor, of templates.
 
> I think this would work better as:
> 
> static Animation* animate(Element& element, E effect)
> {
>     animate(..., Timing::defaults().iterationDuration);
> }
> 
> static Animation* animate(Element& element, E effect, T timing)
> {
>     RefPtr<Animation> animation = Animation::create(&element,
> EffectInput::convert(effect), TimingInput::convert(timing));
> }
> 
> Then we can remove the additional create() functions from Animation and
separate
> out the different types of input conversion.

[alancutter (OOO until 2018), 2014-02-28 00:40:22]

These review changes move a bit of code around and necessitate a minor change in
behaviour to support the unit tests (see updated description).
PTAL.

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:44: static Animation* animate(Element&
element, E effect, T timing = Timing::initialIterationDuration())
On 2014/02/27 09:49:36, dstockwell wrote:
> Body should stay in cpp.
> 
> I think this would work better as:
> 
> static Animation* animate(Element& element, E effect)
> {
>     animate(..., Timing::defaults().iterationDuration);
> }

AFAICT this is the same as using a default parameter except more code
duplication. Am I wrong in thinking this?

> static Animation* animate(Element& element, E effect, T timing)
> {
>     RefPtr<Animation> animation = Animation::create(&element,
> EffectInput::convert(effect), TimingInput::convert(timing));
> }
> 
> Then we can remove the additional create() functions from Animation and
separate
> out the different types of input conversion.

Moved timing input conversion into the Timing class (will be useful for other
timed items that want to make use of the same constructor overloads in future).

Kept effect input conversion in the Animation class, I don't forsee that being
used elsewhere. The Vector<Dictionary> conversion can be moved into
KeyframeEffectModel in a separate patch.

[Timothy Loh, 2014-02-28 00:55:24]

lgtm

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.cpp (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:14: 
Extra blank line :<

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:48: if (!std::isnan(iterationStart) &&
!std::isinf(iterationStart))
While you're here, std::isfinite instead of the two checks.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:72: if (!std::isnan(playbackRate) &&
!std::isinf(playbackRate))
std::isfinite here too.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:71: Timing(double duration)
I wouldn't add an extra ctor here, just call the regular one and set default.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:84: static double initialStartDelay() { return 0;
}
Check with dstockwell here since he does a similar thing in a different way in
crrev.com/178663007.

[dstockwell, 2014-02-28 00:56:36]

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:44: static Animation* animate(Element&
element, E effect, T timing = Timing::initialIterationDuration())
On 2014/02/28 00:40:22, alancutter wrote:
> On 2014/02/27 09:49:36, dstockwell wrote:
> > Body should stay in cpp.
> > 
> > I think this would work better as:
> > 
> > static Animation* animate(Element& element, E effect)
> > {
> >     animate(..., Timing::defaults().iterationDuration);
> > }
> 
> AFAICT this is the same as using a default parameter except more code
> duplication. Am I wrong in thinking this?

The result is the same, but it's more readable.
 
> > static Animation* animate(Element& element, E effect, T timing)
> > {
> >     RefPtr<Animation> animation = Animation::create(&element,
> > EffectInput::convert(effect), TimingInput::convert(timing));
> > }
> > 
> > Then we can remove the additional create() functions from Animation and
> separate
> > out the different types of input conversion.
> 
> Moved timing input conversion into the Timing class (will be useful for other
> timed items that want to make use of the same constructor overloads in
future).
> 
> Kept effect input conversion in the Animation class, I don't forsee that being
> used elsewhere. The Vector<Dictionary> conversion can be moved into
> KeyframeEffectModel in a separate patch.

I'm trying to keep these concerns separate. Handing input is unrelated to the
model.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:84: static double initialStartDelay() { return 0;
}
I'd rather keep these in the constructor and expose a default instance (like in
my other patch).

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:94: static Timing convertInput(const Dictionary&
timingInputDictionary);
Timing doesn't need to know anything about input. These should be in
TimingInput.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:98: void setStartDelay(double);
These also belong in TimingInput, it's never valid outside the API bindings to
specify inputs that are coerced.

[esprehn, 2014-02-28 00:57:15]

I think I'd rather see the big stack of overloads to be honest, this doesn't
seem like a great use of templates :/

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/An...
Source/core/animation/Animation.cpp:58:
element->document().updateStyleIfNeeded();
Animation::create is called from CSSAnimations::maybeApplyPendingUpdate, you
shouldn't be trying to re-enter style recalc from down there. It seems like you
have an abstraction problem here. The script API should be different from the
internal one.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.cpp (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:28: this->endDelay = 0;
This is wrong, you shouldn't make everything public if you expect folks to use a
setter. You want endDelay() getter and m_endDelay

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:104: Timing Timing::convertInput(const
Dictionary& timingInputDictionary)
This is a lot of copying of values. You copy a ton of member vars and a RefPtr?

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:71: Timing(double duration)
explicit

[Timothy Loh, 2014-02-28 00:59:02]

On 2014/02/28 00:57:15, esprehn wrote:
> I think I'd rather see the big stack of overloads to be honest, this doesn't
> seem like a great use of templates :/

We're going to be adding two more effect types (EffectCallback and
MotionPathEffect) in the near future.

[esprehn, 2014-02-28 01:02:56]

On 2014/02/28 00:59:02, Timothy Loh wrote:
> On 2014/02/28 00:57:15, esprehn wrote:
> > I think I'd rather see the big stack of overloads to be honest, this doesn't
> > seem like a great use of templates :/
> 
> We're going to be adding two more effect types (EffectCallback and
> MotionPathEffect) in the near future.

It sounds like you want a virtual base class then, templates seem like a weird
way to do polymorphism of the callers.

[dstockwell, 2014-02-28 01:04:26]

On 2014/02/28 00:59:02, Timothy Loh wrote:
> On 2014/02/28 00:57:15, esprehn wrote:
> > I think I'd rather see the big stack of overloads to be honest, this doesn't
> > seem like a great use of templates :/
> 
> We're going to be adding two more effect types (EffectCallback and
> MotionPathEffect) in the near future.

MotionPathEffect doesn't really count. As I see it there is only going to be
AnimationEffect and whatever EffectCallback is represented by.

[dstockwell, 2014-02-28 01:06:45]

On 2014/02/28 01:04:26, dstockwell wrote:
> On 2014/02/28 00:59:02, Timothy Loh wrote:
> > On 2014/02/28 00:57:15, esprehn wrote:
> > > I think I'd rather see the big stack of overloads to be honest, this
doesn't
> > > seem like a great use of templates :/
> > 
> > We're going to be adding two more effect types (EffectCallback and
> > MotionPathEffect) in the near future.
> 
> MotionPathEffect doesn't really count. As I see it there is only going to be
> AnimationEffect and whatever EffectCallback is represented by.

Oh, and what we have now (keyframe input?)

[alancutter (OOO until 2018), 2014-02-28 01:49:08]

There will be four different types for the effect input (AnimationEffect,
EffectCallback, Vector<Dictionary> and Dictionary) and three for the timing
input (Dictionary, double and void).
These parameter sets appear twice, once in Animation's constructor and again in
element.animate().

[alancutter (OOO until 2018), 2014-03-03 05:09:45]

PTAL.

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:44: static Animation* animate(Element&
element, E effect, T timing = Timing::initialIterationDuration())
On 2014/02/28 00:56:37, dstockwell wrote:
> On 2014/02/28 00:40:22, alancutter wrote:
> > On 2014/02/27 09:49:36, dstockwell wrote:
> > > Body should stay in cpp.
> > > 
> > > I think this would work better as:
> > > 
> > > static Animation* animate(Element& element, E effect)
> > > {
> > >     animate(..., Timing::defaults().iterationDuration);
> > > }
> > 
> > AFAICT this is the same as using a default parameter except more code
> > duplication. Am I wrong in thinking this?
> 
> The result is the same, but it's more readable.

Done.

> > > static Animation* animate(Element& element, E effect, T timing)
> > > {
> > >     RefPtr<Animation> animation = Animation::create(&element,
> > > EffectInput::convert(effect), TimingInput::convert(timing));
> > > }
> > > 
> > > Then we can remove the additional create() functions from Animation and
> > separate
> > > out the different types of input conversion.
> > 
> > Moved timing input conversion into the Timing class (will be useful for
other
> > timed items that want to make use of the same constructor overloads in
> future).
> > 
> > Kept effect input conversion in the Animation class, I don't forsee that
being
> > used elsewhere. The Vector<Dictionary> conversion can be moved into
> > KeyframeEffectModel in a separate patch.
> 
> I'm trying to keep these concerns separate. Handing input is unrelated to the
> model.

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/An...
Source/core/animation/Animation.cpp:58:
element->document().updateStyleIfNeeded();
On 2014/02/28 00:57:16, esprehn wrote:
> Animation::create is called from CSSAnimations::maybeApplyPendingUpdate, you
> shouldn't be trying to re-enter style recalc from down there. It seems like
you
> have an abstraction problem here. The script API should be different from the
> internal one.

This particular check only occurs when an Animation is created from script, the
call in maybeApplyPendingUpdate does not invoke this code.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.cpp (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:14: 
On 2014/02/28 00:55:24, Timothy Loh wrote:
> Extra blank line :<

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:28: this->endDelay = 0;
On 2014/02/28 00:57:16, esprehn wrote:
> This is wrong, you shouldn't make everything public if you expect folks to use
a
> setter. You want endDelay() getter and m_endDelay

Done, moved setters to a separate TimingInput class.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:48: if (!std::isnan(iterationStart) &&
!std::isinf(iterationStart))
On 2014/02/28 00:55:24, Timothy Loh wrote:
> While you're here, std::isfinite instead of the two checks.

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:72: if (!std::isnan(playbackRate) &&
!std::isinf(playbackRate))
On 2014/02/28 00:55:24, Timothy Loh wrote:
> std::isfinite here too.

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.cpp:104: Timing Timing::convertInput(const
Dictionary& timingInputDictionary)
On 2014/02/28 00:57:16, esprehn wrote:
> This is a lot of copying of values. You copy a ton of member vars and a
RefPtr?

This function is copying data from V8 into Blink, I'm not aware of an
alternative.
This code is just being moved by this patch out of Animation.cpp, any proposed
changes to it would be for a separate patch.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:71: Timing(double duration)
On 2014/02/28 00:55:24, Timothy Loh wrote:
> I wouldn't add an extra ctor here, just call the regular one and set default.

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:84: static double initialStartDelay() { return 0;
}
On 2014/02/28 00:56:37, dstockwell wrote:
> I'd rather keep these in the constructor and expose a default instance (like
in
> my other patch).

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:94: static Timing convertInput(const Dictionary&
timingInputDictionary);
On 2014/02/28 00:56:37, dstockwell wrote:
> Timing doesn't need to know anything about input. These should be in
> TimingInput.

Done.

https://codereview.chromium.org/182063005/diff/40001/Source/core/animation/Ti...
Source/core/animation/Timing.h:98: void setStartDelay(double);
On 2014/02/28 00:56:37, dstockwell wrote:
> These also belong in TimingInput, it's never valid outside the API bindings to
> specify inputs that are coerced.

Done.

[rjwright, 2014-03-03 06:51:36]

lgtm.

I can't comment on whether templates are the right way to go, but this is better
than listing out all the overloads we're going to need.

Other structural changes in this patch look good. Nice one Alan :)

[esprehn, 2014-03-03 07:35:44]

On 2014/03/03 06:51:36, rjwright wrote:
> lgtm.
> 
> I can't comment on whether templates are the right way to go, but this is
better
> than listing out all the overloads we're going to need.
> 
> Other structural changes in this patch look good. Nice one Alan :)

I'd really rather you just listed out all the overloads, having 5 of them isn't
that many. :)

Using templates just obfuscates what the code is doing and what parameters are
acceptable for someone else looking at the code.

[rjwright, 2014-03-03 23:08:00]

On 2014/03/03 07:35:44, esprehn wrote:
> On 2014/03/03 06:51:36, rjwright wrote:
> > lgtm.
> > 
> > I can't comment on whether templates are the right way to go, but this is
> better
> > than listing out all the overloads we're going to need.
> > 
> > Other structural changes in this patch look good. Nice one Alan :)
> 
> I'd really rather you just listed out all the overloads, having 5 of them
isn't
> that many. :)
> 
> Using templates just obfuscates what the code is doing and what parameters are
> acceptable for someone else looking at the code.

I was under the impression that we'd have 10 for each function (Timing Input
Types x Effect Types - 2 for EffectCallback), which is a total of 20 methods, on
top of corresponding entries in the WebIDL. I guess it really depends on how
many there is going to be. Do you know exactly how many versions we expect to
need, Alan?

[alancutter (OOO until 2018), 2014-03-03 23:13:33]

On 2014/03/03 07:35:44, esprehn wrote:
> I'd really rather you just listed out all the overloads, having 5 of them
isn't
> that many. :)

We're looking at having 12 overloads in two places without templates.
After the changes that dstockwell suggested with EffectInput::convert and
TimingInput::convert the code duplication is significantly reduced, having 12x2
copy pastes isn't quite as bad as it would have been so I'm not against dropping
the templates.

[rjwright1, 2014-03-03 23:26:51]

I'm not passionately against listing the overloads as long as the duplicate
methods are compact. The only thing that saves templating here, IMO, is
that they're instantiated right out of the WebIDL, so we have the WebIDL as
a directory for the template instances.


On Tue, Mar 4, 2014 at 10:13 AM, <alancutter@chromium.org> wrote:

> On 2014/03/03 07:35:44, esprehn wrote:
>
>> I'd really rather you just listed out all the overloads, having 5 of them
>>
> isn't
>
>> that many. :)
>>
>
> We're looking at having 12 overloads in two places without templates.
> After the changes that dstockwell suggested with EffectInput::convert and
> TimingInput::convert the code duplication is significantly reduced, having
> 12x2
> copy pastes isn't quite as bad as it would have been so I'm not against
> dropping
> the templates.
>
> https://codereview.chromium.org/182063005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[alancutter (OOO until 2018), 2014-03-04 00:39:44]

Templates removed from patch (previously named "Make the Animation constructor
templated").
PTAL.

[esprehn, 2014-03-04 00:59:41]

You definitely don't want to be passing a Vector<Dictionary> by value all the
time.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:25:
PassRefPtrWillBeRawPtr<AnimationEffect> EffectInput::convert(Element* element,
Vector<Dictionary> keyframeDictionaryVector, bool unsafe)
You almost certainly want to pass const Vector<Dicionary>&, you don't want to be
copying the Dictionary and the Vector constantly.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:44: if
(keyframeDictionaryVector[i].get("offset", offset)) {
no braces on a one line if

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:56: if (timingFunctionValue) {
ditto

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/ElementAnimation.h:78: static Animation*
animateImpl(Element& element, PassRefPtrWillBeRawPtr<AnimationEffect> effect,
Timing timing)
animateInternal?

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
File Source/core/animation/TimingInput.cpp (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
Source/core/animation/TimingInput.cpp:14: void
TimingInput::setStartDelay(Timing& timing, double startDelay)
This is really weird, why not make Timing a class with methods?

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
Source/core/animation/TimingInput.cpp:160: Timing TimingInput::convert()
This seems like it wants to be createDefault() and the other methods want to be
create? not convert() ?

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
File Source/core/animation/TimingInput.h (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
Source/core/animation/TimingInput.h:14: class TimingInput {
Can we just make Timing a class with methods?

[alancutter (OOO until 2018), 2014-03-04 01:21:41]

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:25:
PassRefPtrWillBeRawPtr<AnimationEffect> EffectInput::convert(Element* element,
Vector<Dictionary> keyframeDictionaryVector, bool unsafe)
On 2014/03/04 00:59:42, esprehn wrote:
> You almost certainly want to pass const Vector<Dicionary>&, you don't want to
be
> copying the Dictionary and the Vector constantly.

Done.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:44: if
(keyframeDictionaryVector[i].get("offset", offset)) {
On 2014/03/04 00:59:42, esprehn wrote:
> no braces on a one line if

Done.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:56: if (timingFunctionValue) {
On 2014/03/04 00:59:42, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/ElementAnimation.h:78: static Animation*
animateImpl(Element& element, PassRefPtrWillBeRawPtr<AnimationEffect> effect,
Timing timing)
On 2014/03/04 00:59:42, esprehn wrote:
> animateInternal?

Done.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
File Source/core/animation/TimingInput.cpp (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
Source/core/animation/TimingInput.cpp:14: void
TimingInput::setStartDelay(Timing& timing, double startDelay)
On 2014/03/04 00:59:42, esprehn wrote:
> This is really weird, why not make Timing a class with methods?

dstockwell suggested separating these methods into TimingInput as these will
only ever be invoked by the IDL bindings to coerce script inputs into acceptable
timing model values.
These methods are public and not static to the CPP file because TimedItemTiming
(an IDL specific class) also uses them.

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/T...
Source/core/animation/TimingInput.cpp:160: Timing TimingInput::convert()
On 2014/03/04 00:59:42, esprehn wrote:
> This seems like it wants to be createDefault() and the other methods want to
be
> create? not convert() ?

Not sure what I was thinking here.
Removed this and instead am using the default Timing constructor.

[dstockwell, 2014-03-04 01:29:30]

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/182063005/diff/110001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:44: if
(keyframeDictionaryVector[i].get("offset", offset)) {
On 2014/03/04 00:59:42, esprehn wrote:
> no braces on a one line if

The blink style guide was relaxed about this a long time ago, it reads:
"Curly braces are not required for single-line conditionals or loop bodies"

https://codereview.chromium.org/182063005/diff/130001/Source/core/animation/E...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/182063005/diff/130001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
This doesn't look like a brand new file. There was a discussion on blink-dev, I
think the summary was: unless you are creating a brand new file you should keep
the header from the old file.

[alancutter (OOO until 2018), 2014-03-04 01:35:14]

https://codereview.chromium.org/182063005/diff/130001/Source/core/animation/E...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/182063005/diff/130001/Source/core/animation/E...
Source/core/animation/EffectInput.cpp:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2014/03/04 01:29:31, dstockwell wrote:
> This doesn't look like a brand new file. There was a discussion on blink-dev,
I
> think the summary was: unless you are creating a brand new file you should
keep
> the header from the old file.

Done.

[esprehn, 2014-03-04 01:37:28]

lgtm, thanks for the iteration! :)

[dstockwell, 2014-03-04 01:43:31]

lgtm

[commit-bot: I haz the power, 2014-03-04 01:43:38]

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

[alancutter (OOO until 2018), 2014-03-04 01:45:10]

Thanks for the speedy reviews. :)

[commit-bot: I haz the power, 2014-03-04 17:24:24]

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

[commit-bot: I haz the power, 2014-03-04 17:24:24]

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

[alancutter (OOO until 2018), 2014-03-04 22:58:38]

The CQ bit was checked by alancutter@chromium.org

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

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

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

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

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

Failed to apply patch for Source/core/animation/TimingInputTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/core/animation/TimingInputTest.cpp
  Copied Source/core/animation/AnimationTimingInputTest.cpp ->
Source/core/animation/TimingInputTest.cpp
  patching file Source/core/animation/TimingInputTest.cpp
  Hunk #2 FAILED at 14.
  Hunk #3 FAILED at 29.
  Hunk #4 FAILED at 49.
  Hunk #5 succeeded at 65 (offset -2 lines).
  Hunk #6 succeeded at 85 (offset -2 lines).
  Hunk #7 succeeded at 95 (offset -2 lines).
  Hunk #8 succeeded at 109 (offset -2 lines).
  Hunk #9 succeeded at 125 (offset -2 lines).
  Hunk #10 succeeded at 135 (offset -2 lines).
  Hunk #11 succeeded at 147 (offset -2 lines).
  Hunk #12 FAILED at 172.
  4 out of 12 hunks FAILED -- saving rejects to file
Source/core/animation/TimingInputTest.cpp.rej

Patch:   NR 
Source/core/animation/AnimationTimingInputTest.cpp->Source/core/animation/TimingInputTest.cpp
Index: Source/core/animation/TimingInputTest.cpp
diff --git a/Source/core/animation/AnimationTimingInputTest.cpp
b/Source/core/animation/TimingInputTest.cpp
similarity index 87%
rename from Source/core/animation/AnimationTimingInputTest.cpp
rename to Source/core/animation/TimingInputTest.cpp
index
3d7861d415e077fb33aca7ed0165397b860de233..c9dcfe8b44163c574a7cf1ac8e14dc09278353ee
100644
--- a/Source/core/animation/AnimationTimingInputTest.cpp
+++ b/Source/core/animation/TimingInputTest.cpp
@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 #include "config.h"
-#include "core/animation/Animation.h"
+#include "core/animation/TimingInput.h"
 
 #include "bindings/v8/Dictionary.h"
 #include "core/animation/AnimationTestHelper.h"
@@ -14,9 +14,9 @@
 
 namespace WebCore {
 
-class AnimationAnimationTimingInputTest : public ::testing::Test {
+class AnimationTimingInputTest : public ::testing::Test {
 protected:
-    AnimationAnimationTimingInputTest()
+    AnimationTimingInputTest()
         : isolate(v8::Isolate::GetCurrent())
         , scope(isolate)
         , context(v8::Context::New(isolate))
@@ -29,19 +29,12 @@ protected:
     v8::Local<v8::Context> context;
     v8::Context::Scope contextScope;
 
-    void populateTiming(Timing& timing, Dictionary timingInputDictionary)
-    {
-        Animation::populateTiming(timing, timingInputDictionary);
-    }
-
     Timing applyTimingInputNumber(String timingProperty, double
timingPropertyValue)
     {
         v8::Handle<v8::Object> timingInput = v8::Object::New(isolate);
         setV8ObjectPropertyAsNumber(timingInput, timingProperty,
timingPropertyValue);
         Dictionary timingInputDictionary =
Dictionary(v8::Handle<v8::Value>::Cast(timingInput), isolate);
-        Timing timing;
-        populateTiming(timing, timingInputDictionary);
-        return timing;
+        return TimingInput::convert(timingInputDictionary);
     }
 
     Timing applyTimingInputString(String timingProperty, String
timingPropertyValue)
@@ -49,13 +42,11 @@ protected:
         v8::Handle<v8::Object> timingInput = v8::Object::New(isolate);
         setV8ObjectPropertyAsString(timingInput, timingProperty,
timingPropertyValue);
         Dictionary timingInputDictionary =
Dictionary(v8::Handle<v8::Value>::Cast(timingInput), isolate);
-        Timing timing;
-        populateTiming(timing, timingInputDictionary);
-        return timing;
+        return TimingInput::convert(timingInputDictionary);
     }
 };
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputStartDelay)
+TEST_F(AnimationTimingInputTest, TimingInputStartDelay)
 {
     EXPECT_EQ(1.1, applyTimingInputNumber("delay", 1.1).startDelay);
     EXPECT_EQ(-1, applyTimingInputNumber("delay", -1).startDelay);
@@ -67,13 +58,13 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputStartDelay)
     EXPECT_EQ(0, applyTimingInputString("delay", "rubbish").startDelay);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputEndDelay)
+TEST_F(AnimationTimingInputTest, TimingInputEndDelay)
 {
     EXPECT_EQ(10, applyTimingInputNumber("endDelay", 10).endDelay);
     EXPECT_EQ(-2.5, applyTimingInputNumber("endDelay", -2.5).endDelay);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputFillMode)
+TEST_F(AnimationTimingInputTest, TimingInputFillMode)
 {
     Timing::FillMode defaultFillMode = Timing::FillModeAuto;
 
@@ -87,7 +78,7 @@ TEST_F(AnimationAnimationTimingInputTest, TimingInputFillMode)
     EXPECT_EQ(defaultFillMode, applyTimingInputNumber("fill", 2).fillMode);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputIterationStart)
+TEST_F(AnimationTimingInputTest, TimingInputIterationStart)
 {
     EXPECT_EQ(1.1, applyTimingInputNumber("iterationStart",
1.1).iterationStart);
     EXPECT_EQ(0, applyTimingInputNumber("iterationStart", -1).iterationStart);
@@ -97,7 +88,7 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputIterationStart)
     EXPECT_EQ(0, applyTimingInputString("iterationStart",
"rubbish").iterationStart);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputIterationCount)
+TEST_F(AnimationTimingInputTest, TimingInputIterationCount)
 {
     EXPECT_EQ(2.1, applyTimingInputNumber("iterations", 2.1).iterationCount);
     EXPECT_EQ(0, applyTimingInputNumber("iterations", -1).iterationCount);
@@ -111,7 +102,7 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputIterationCount)
     EXPECT_EQ(1, applyTimingInputString("iterations",
"rubbish").iterationCount);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputIterationDuration)
+TEST_F(AnimationTimingInputTest, TimingInputIterationDuration)
 {
     EXPECT_EQ(1.1, applyTimingInputNumber("duration", 1.1).iterationDuration);
     EXPECT_TRUE(std::isnan(applyTimingInputNumber("duration",
-1).iterationDuration));
@@ -127,7 +118,7 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputIterationDuration)
     EXPECT_TRUE(std::isnan(applyTimingInputString("duration",
"rubbish").iterationDuration));
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputPlaybackRate)
+TEST_F(AnimationTimingInputTest, TimingInputPlaybackRate)
 {
     EXPECT_EQ(2.1, applyTimingInputNumber("playbackRate", 2.1).playbackRate);
     EXPECT_EQ(-1, applyTimingInputNumber("playbackRate", -1).playbackRate);
@@ -137,7 +128,7 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputPlaybackRate)
     EXPECT_EQ(1, applyTimingInputString("playbackRate",
"rubbish").playbackRate);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputDirection)
+TEST_F(AnimationTimingInputTest, TimingInputDirection)
 {
     Timing::PlaybackDirection defaultPlaybackDirection =
Timing::PlaybackDirectionNormal;
 
@@ -149,7 +140,7 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputDirection)
     EXPECT_EQ(defaultPlaybackDirection, applyTimingInputNumber("direction",
2).direction);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputTimingFunction)
+TEST_F(AnimationTimingInputTest, TimingInputTimingFunction)
 {
     const RefPtr<TimingFunction> defaultTimingFunction =
LinearTimingFunction::create();
 
@@ -172,14 +163,13 @@ TEST_F(AnimationAnimationTimingInputTest,
TimingInputTimingFunction)
     EXPECT_EQ(*defaultTimingFunction, *applyTimingInputNumber("easing",
2).timingFunction);
 }
 
-TEST_F(AnimationAnimationTimingInputTest, TimingInputEmpty)
+TEST_F(AnimationTimingInputTest, TimingInputEmpty)
 {
-    Timing updatedTiming;
     Timing controlTiming;
 
     v8::Handle<v8::Object> timingInput = v8::Object::New(isolate);
     Dictionary timingInputDictionary =
Dictionary(v8::Handle<v8::Value>::Cast(timingInput), isolate);
-    populateTiming(updatedTiming, timingInputDictionary);
+    Timing updatedTiming = TimingInput::convert(timingInputDictionary);
 
     EXPECT_EQ(controlTiming.startDelay, updatedTiming.startDelay);
     EXPECT_EQ(controlTiming.fillMode, updatedTiming.fillMode);

[alancutter (OOO until 2018), 2014-03-04 23:27:34]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2014-03-04 23:28:12]

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

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

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

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

Failed to apply patch for Source/core/animation/AnimationTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/animation/AnimationTest.cpp
  Hunk #1 FAILED at 12.
  Hunk #2 succeeded at 40 (offset -1 lines).
  Hunk #3 succeeded at 123 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/animation/AnimationTest.cpp.rej

Patch:       Source/core/animation/AnimationTest.cpp
Index: Source/core/animation/AnimationTest.cpp
diff --git a/Source/core/animation/AnimationTest.cpp
b/Source/core/animation/AnimationTest.cpp
index
a281a20dc7237dcd9a0975e7452640538882d75f..72be1fa004a0c89b41b1261c9aee0080285a6826
100644
--- a/Source/core/animation/AnimationTest.cpp
+++ b/Source/core/animation/AnimationTest.cpp
@@ -12,6 +12,7 @@
 #include "core/animation/AnimationTestHelper.h"
 #include "core/animation/DocumentTimeline.h"
 #include "core/animation/KeyframeEffectModel.h"
+#include "core/animation/Timing.h"
 #include "platform/animation/TimingFunctionTestHelper.h"
 
 #include <gtest/gtest.h>
@@ -41,19 +42,14 @@ protected:
     {
     }
 
-    PassRefPtr<Animation> createAnimation(Element* element, Vector<Dictionary>
keyframeDictionaryVector, Dictionary timingInput)
+    template<typename T>
+    static PassRefPtr<Animation> createAnimation(Element* element,
Vector<Dictionary> keyframeDictionaryVector, T timingInput)
     {
-        return Animation::createUnsafe(element, keyframeDictionaryVector,
timingInput);
+        return Animation::create(element, EffectInput::convert(element,
keyframeDictionaryVector, true), timingInput);
     }
-
-    PassRefPtr<Animation> createAnimation(Element* element, Vector<Dictionary>
keyframeDictionaryVector, double timingInput)
+    static PassRefPtr<Animation> createAnimation(Element* element,
Vector<Dictionary> keyframeDictionaryVector)
     {
-        return Animation::createUnsafe(element, keyframeDictionaryVector,
timingInput);
-    }
-
-    PassRefPtr<Animation> createAnimation(Element* element, Vector<Dictionary>
keyframeDictionaryVector)
-    {
-        return Animation::createUnsafe(element, keyframeDictionaryVector);
+        return Animation::create(element, EffectInput::convert(element,
keyframeDictionaryVector, true));
     }
 
     v8::Isolate* m_isolate;
@@ -129,11 +125,11 @@ TEST_F(AnimationAnimationV8Test, CanOmitSpecifiedDuration)
     EXPECT_TRUE(std::isnan(animation->specifiedTiming().iterationDuration));
 }
 
-TEST_F(AnimationAnimationV8Test, ClipNegativeDurationToZero)
+TEST_F(AnimationAnimationV8Test, NegativeDurationIsAuto)
 {
     Vector<Dictionary, 0> jsKeyframes;
     RefPtr<Animation> animation = createAnimation(element.get(), jsKeyframes,
-2);
-    EXPECT_EQ(0, animation->specifiedTiming().iterationDuration);
+    EXPECT_TRUE(std::isnan(animation->specifiedTiming().iterationDuration));
 }
 
 TEST_F(AnimationAnimationV8Test, SpecifiedGetters)

[alancutter (OOO until 2018), 2014-03-05 22:54:43]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2014-03-05 22:55:09]

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

[commit-bot: I haz the power, 2014-03-06 05:49:53]

Change committed as 168575

[esprehn, 2014-03-06 06:23:09]

lgtm