Add foundation for removing AnimatableValues from StyleInterpolation

Add foundation for removing AnimatableValues from StyleInterpolation

This is the initial patch for upgrading StyleInterpolations away from
relying on AnimatableValues for non-trivial keyframe values like "inherit"
and breaking free from the dependency on StyleResolver when constructing
animations.
This design is also intended to support additive animations in the near future.

This change implements the DefaultAnimationType which covers animation of
all non-interpolable properties in element.animate().

BUG=450238, 437696
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197705

[alancutter (OOO until 2018), 2015-05-25 04:54:37]

alancutter@chromium.org changed reviewers:
+ ericwilligers@chromium.org, timloh@chromium.org

[alancutter (OOO until 2018), 2015-05-25 06:02:16]

Make test visually clear

[alancutter (OOO until 2018), 2015-05-25 06:12:08]

Remove commented out code

[alancutter (OOO until 2018), 2015-05-25 06:35:51]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-25 06:35:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153943003/100001

[Eric Willigers, 2015-05-27 04:26:35]

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/AnimationValue.h:24: : type(nullptr)
Can these initializations be indented. Also for other constructors.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/AnimationValue.h:40: { }
other.type = nullptr; ?

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/DefaultAnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/DefaultAnimationType.h:15: : m_property(property)
Can be indented. Same in DefaultNonInterpolableValue constructor.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/DefaultAnimationType.h:29: CSSPropertyID m_property;
Can be const.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/InterpolationIsInvalid.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:23: static void
chain(RefPtrWillBeRawPtr<InterpolationIsInvalid>& existing,
PassRefPtrWillBeRawPtr<InterpolationIsInvalid> extra)
Possibly rename arguments to  prefix, suffix

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:42: void
chainWith(PassRefPtrWillBeRawPtr<InterpolationIsInvalid> extra)
Possibly rename argument to  suffix

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:48: m_next->chainWith(extra);
Explicit while loop instead?

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.h:46: bool isInvalid(const
StyleResolverState& state) const { return !m_isInvalid ||
m_isInvalid->checkChain(state); }
m_isInvalid && m_isInvalid->checkChain(state);

Perhaps rename m_isInvalid to m_interpolationIsInvalid  or m_invalidator?
And InterpolationIsInvalid becomes InterpolationInvalidator

[alancutter (OOO until 2018), 2015-05-27 08:27:48]

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/AnimationValue.h:24: : type(nullptr)
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Can these initializations be indented. Also for other constructors.

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/AnimationValue.h:40: { }
On 2015/05/27 04:26:34, Eric Willigers wrote:
> other.type = nullptr; ?

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/DefaultAnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/DefaultAnimationType.h:15: : m_property(property)
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Can be indented. Same in DefaultNonInterpolableValue constructor.

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/DefaultAnimationType.h:29: CSSPropertyID m_property;
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Can be const.

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/InterpolationIsInvalid.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:23: static void
chain(RefPtrWillBeRawPtr<InterpolationIsInvalid>& existing,
PassRefPtrWillBeRawPtr<InterpolationIsInvalid> extra)
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Possibly rename arguments to  prefix, suffix

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:42: void
chainWith(PassRefPtrWillBeRawPtr<InterpolationIsInvalid> extra)
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Possibly rename argument to  suffix

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InterpolationIsInvalid.h:48: m_next->chainWith(extra);
On 2015/05/27 04:26:35, Eric Willigers wrote:
> Explicit while loop instead?

Done.

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.h (right):

https://codereview.chromium.org/1153943003/diff/220001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.h:46: bool isInvalid(const
StyleResolverState& state) const { return !m_isInvalid ||
m_isInvalid->checkChain(state); }
On 2015/05/27 04:26:35, Eric Willigers wrote:
> m_isInvalid && m_isInvalid->checkChain(state);
> 
> Perhaps rename m_isInvalid to m_interpolationIsInvalid  or m_invalidator?
> And InterpolationIsInvalid becomes InterpolationInvalidator

Done.

[Eric Willigers, 2015-05-27 09:16:22]

I suspect you forgot to upload InterpolationInvalidators.h

[alancutter (OOO until 2018), 2015-05-27 23:54:47]

On 2015/05/27 at 09:16:22, ericwilligers wrote:
> I suspect you forgot to upload InterpolationInvalidators.h

Done.

[Eric Willigers, 2015-05-28 00:34:49]

lgtm with nit

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InterpolationInvalidators.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:23: static void
chain(RefPtrWillBeRawPtr<InterpolationInvalidators>& prefix,
PassRefPtrWillBeRawPtr<InterpolationInvalidators> suffix)
It would be nice to have a unit test that uses chain, chainIsInvalid.

[alancutter (OOO until 2018), 2015-05-28 04:44:32]

alancutter@chromium.org changed reviewers:
+ dstockwell@chromium.org, shans@chromium.org

[alancutter (OOO until 2018), 2015-05-28 04:45:01]

+shans and +dstockwell as reviewers.

[shans, 2015-05-28 05:48:14]

General comment: There are a lot of code paths in here that seem to be never
invoked. Some of them do not seem to be required by the design. Please:
(1) remove code paths that are purely speculative
(2) test those that are not, so we don't check in untested unused code

I'm also very uncomfortable with the fact that you're inventing new memory
paradigms (consume, etc.) instead of using the existing ones. Please review
carefully the way you deal with allocation/deallocation and rewrite your code to
be more in line with Blink practice.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationType.h:33: return false;
Shouldn't the default implementation of this attempt two single conversions?

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationValue.h:47: interpolableValue =
other.interpolableValue ? other.interpolableValue->clone() : nullptr;
Are AnimationValues mutable? I can't think of a strong reason why the
interpolableValue would be mutated (as opposed to maybe just replaced depending
on implementation details).

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationValue.h:58: void consume(AnimationValue& other)
what is consume?

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InterpolationInvalidators.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:14: class
InterpolationInvalidators : public
RefCountedWillBeGarbageCollectedFinalized<InterpolationInvalidators> {
This needs either documentation, better names, or (preferably) both.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:42: void
chainWith(PassRefPtrWillBeRawPtr<InterpolationInvalidators> suffix)
Maybe use a Vector instead? This seems rather heavy-weight.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:26:
ASSERT_UNUSED(m_type, result.type == &m_type);
This doesn't need to be ASSERT_UNUSED.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:27:
ASSERT(result.nonInterpolableValue == m_nonInterpolableValue);
These probably aren't needed here. We can assert these properties up-front and
when the values are changed.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:73: if
(!std::isnan(m_lastFraction) && isBeforeFlip(fraction) ==
isBeforeFlip(m_lastFraction))
isBeforeFlip is a more obscure name than < 0.5...

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:125: if
(animationType->maybeConvertPairwise(*m_startKeyframe, *m_endKeyframe, state,
invalidators, startInterpolableValue, endInterpolableValue,
nonInterpolableValue)) {
This is quite messy, and strongly suggests that maybeConvertPairwise should
return the PairwiseConversionCache directly.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/NonInterpolableValue.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/NonInterpolableValue.h:22: #define
DECLARE_NON_INTERPOLABLE_VALUE_TYPE() \
This requires a very clear documented statement of intent and typical usage.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/StringKeyframe.cpp:156: const Vector<const
AnimationType*>* applicableTypesForProperty(CSSPropertyID property)
This can be deferred until we support anything that isAnimatableProperty.

[alancutter (OOO until 2018), 2015-06-01 07:35:29]

Thanks for the quick review.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationType.h:33: return false;
On 2015/05/28 05:48:13, shans wrote:
> Shouldn't the default implementation of this attempt two single conversions?

Yes. DefaultAnimationType does not make use of it at the moment so I left it out
of this patch.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationValue.h:47: interpolableValue =
other.interpolableValue ? other.interpolableValue->clone() : nullptr;
On 2015/05/28 05:48:13, shans wrote:
> Are AnimationValues mutable? I can't think of a strong reason why the
> interpolableValue would be mutated (as opposed to maybe just replaced
depending
> on implementation details).

They are mutable as a consequence of InterpolableValues being mutable.
InterpolableValue::interpolate() mutates its InterpolableValue out parameter.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/AnimationValue.h:58: void consume(AnimationValue& other)
On 2015/05/28 05:48:13, shans wrote:
> what is consume?

Renamed to swap() to match Vector::swap() and added comment.
Once Oilpan lands this can be removed as InterpolableValues will cease to be
OwnPtr'd.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InterpolationInvalidators.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:14: class
InterpolationInvalidators : public
RefCountedWillBeGarbageCollectedFinalized<InterpolationInvalidators> {
On 2015/05/28 05:48:13, shans wrote:
> This needs either documentation, better names, or (preferably) both.

ConversionInvalidator is probably more accurate since these objects indicate
when an AnimationType's conversion becomes invalid as a result of a change in
the environment.
Renamed and added comment.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:23: static void
chain(RefPtrWillBeRawPtr<InterpolationInvalidators>& prefix,
PassRefPtrWillBeRawPtr<InterpolationInvalidators> suffix)
On 2015/05/28 at 00:34:48, Eric Willigers wrote:
> It would be nice to have a unit test that uses chain, chainIsInvalid.

Done.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InterpolationInvalidators.h:42: void
chainWith(PassRefPtrWillBeRawPtr<InterpolationInvalidators> suffix)
On 2015/05/28 05:48:13, shans wrote:
> Maybe use a Vector instead? This seems rather heavy-weight.

This is actually designed to be lightweight in the common case as you can use a
nullptr to mean a conversion is never invalid. Using vectors will force us to
allocate every time.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:26:
ASSERT_UNUSED(m_type, result.type == &m_type);
On 2015/05/28 05:48:14, shans wrote:
> This doesn't need to be ASSERT_UNUSED.

This used to be ASSERT but it failed to compile in release.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:27:
ASSERT(result.nonInterpolableValue == m_nonInterpolableValue);
On 2015/05/28 05:48:14, shans wrote:
> These probably aren't needed here. We can assert these properties up-front and
> when the values are changed.

This assertion is to check that nothing's messed with
InvalidatableStyleInterpolation::m_cachedValue unexpectedly since the pairwise
conversion. Putting the assertion at the conversion isn't semantically
equivalent.
I may be mistaking where you meant for the assertion to be, can you elaborate?

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:73: if
(!std::isnan(m_lastFraction) && isBeforeFlip(fraction) ==
isBeforeFlip(m_lastFraction))
On 2015/05/28 05:48:14, shans wrote:
> isBeforeFlip is a more obscure name than < 0.5...

Inlined isBeforeFlip().

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:125: if
(animationType->maybeConvertPairwise(*m_startKeyframe, *m_endKeyframe, state,
invalidators, startInterpolableValue, endInterpolableValue,
nonInterpolableValue)) {
On 2015/05/28 05:48:13, shans wrote:
> This is quite messy, and strongly suggests that maybeConvertPairwise should
> return the PairwiseConversionCache directly.

Something I decided when making the AnimationType interface is to avoid having
them return a pointer to themselves redundantly. Doing it the suggested way
avoids having so many out parameters but would mean PairwiseConversionCache and
ConversionCache get exposed outside of InvalidatableStyleInterpolation.

I opted for the former for its semantic purity at the cost of messy function
signatures. It doesn't seem right to be required to say "animationType = this"
in every AnimationType subclass (assuming maybeConvertSingle() is updated to
match the convention).

WDYT is better?

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/NonInterpolableValue.h (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/NonInterpolableValue.h:22: #define
DECLARE_NON_INTERPOLABLE_VALUE_TYPE() \
On 2015/05/28 05:48:14, shans wrote:
> This requires a very clear documented statement of intent and typical usage.

Done.

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/StringKeyframe.cpp:156: const Vector<const
AnimationType*>* applicableTypesForProperty(CSSPropertyID property)
On 2015/05/28 05:48:14, shans wrote:
> This can be deferred until we support anything that isAnimatableProperty.

This code serves the AnimationTypes as per property singleton objects in line
with the overall design. This will allow different Interpolations to determine
whether they can composite together or not by sharing the same AnimationType
object between them.

Unfortunately it can't be replaced with something simple like
    if (!isAnimatableProperty(property))
        return InvalidatableStyleInterpolation::create(...);
because this is missing an owner of the AnimationType object. Adding one would
bring us back to the code that's currently here or something equally lengthy
that goes against the memory model that AnimationTypes have been designed for.

[alancutter (OOO until 2018), 2015-06-01 23:47:54]

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/270001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:125: if
(animationType->maybeConvertPairwise(*m_startKeyframe, *m_endKeyframe, state,
invalidators, startInterpolableValue, endInterpolableValue,
nonInterpolableValue)) {
On 2015/06/01 07:35:29, alancutter wrote:
> On 2015/05/28 05:48:13, shans wrote:
> > This is quite messy, and strongly suggests that maybeConvertPairwise should
> > return the PairwiseConversionCache directly.
> 
> Something I decided when making the AnimationType interface is to avoid having
> them return a pointer to themselves redundantly. Doing it the suggested way
> avoids having so many out parameters but would mean PairwiseConversionCache
and
> ConversionCache get exposed outside of InvalidatableStyleInterpolation.
> 
> I opted for the former for its semantic purity at the cost of messy function
> signatures. It doesn't seem right to be required to say "animationType = this"
> in every AnimationType subclass (assuming maybeConvertSingle() is updated to
> match the convention).
> 
> WDYT is better?

On second thought I can just define a return struct for these conversion
functions. It'll be a bit ugly at first due to the OwnPtrs but once Oilpan lands
the ugliness will go away.

[alancutter (OOO until 2018), 2015-06-02 07:48:41]

Refactored patch based on review. PTAL.

maybeConvertPairwise() no longer takes a ghastly list of out parameters, instead
returns a PairwiseAnimationConversion object. Similarly for maybeConvertSingle()
which returns an AnimationConversionHalf.
These objects replace InvalidatableStyleInterpolation::ConversionCache and live
in AnimationConversion.h.

With these changes AnimationValue::swap/consume() is no longer necessary.

[shans, 2015-06-04 00:15:16]

So the new way of doing things is:

StringKeyframe is tasked with creating Interpolations. It uses
StringKeyframe::applicableTypesForProperty(CSSPropertyID) to retrieve the
AnimationTypes that are valid for the given propertyID.

An AnimationType is: 
* a type reference
* a handle via which conversion to AnimationConversionHalf / 
  PairwiseAnimationConversion objects can be performed.
* a handle via which application of an InterpolableValue/NonInterpolableValue 
  into a StyleResolverState can be performed.

InvalidatableStyleInterpolation (eventually just 'StyleInterpolation') is 
responsible for building itself around the list of AnimationTypes for a given
CSSPropertyID. It's important that ISI has the full list of AnimationTypes 
because it may need to switch internal representations in the future (e.g.
because of additive animation).

The "fast path" for ISI is that it caches a pairwise conversion, hopefully
forever. But PairwiseAnimationConversions have a list of Invalidators that
can be used to retire the conversion when it's no longer appropriate. The
invalidator list is installed based on what the particular ISI might be
sensitive to (is it additive? Could changes in underlying value type cause
a need to rebuild the conversion?).

Questions not addressed by this patch:
--------------------------------------
* when/where is validateCache called?
* can validateCache ever fail? It seems that it can, because m_cachedConversion
  can be empty. But interpolations can't ever fail. What do?
* Why is the default fallback partially built into
  InvalidatableStyleInterpolation via convertSingleKeyframe? Shouldn't the
  DefaultAnimationType deal with this?
  
Naming
------

PairwiseAnimationConversion: this is actually the interpolation primitive now.
PrimitiveInterpolation? PrimitiveStyleInterpolation?

AnimationConversionHalf: Does this need to have independent existence outside of
DefaultAnimationType? Currently it's also used by
InvalidatableStyleInterpolation (but
here seems to be duplicating what DefaultAnimationType does) and by
maybeConvertSingle in
AnimationType (but this is only called by InvalidatableStyleInterpolation). If
it
doesn't, then we can come up with a much simpler name.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/AnimationConversion.h (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/AnimationConversion.h:92: struct AnimationConversionHalf :
public NoBaseWillBeGarbageCollectedFinalized<AnimationConversionHalf> {
Not sure about this name.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/AnimationValue.h:39: void copy(const AnimationValue&
other)
Should this not just be a copy constructor?

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:22:
maybeCachePairwiseConversion(nullptr);
should be doing the convertSingleKeyframe fallback in validateCache here too?

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:46:
m_cachedValue.clear();
Is this right? I thought we always needed a way to interpolate.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:54: result->type =
animationType;
This isn't the right way to do this! result was *just generated* by
animationType - put the type setting code into maybeConvertSingle instead.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableValueKeyframe.cpp (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/animatable/AnimatableValueKeyframe.cpp:71:
PassRefPtrWillBeRawPtr<Keyframe::PropertySpecificKeyframe>
AnimatableValueKeyframe::PropertySpecificKeyframe::neutralKeyframe(double
offset, PassRefPtr<TimingFunction> easing) const
Can you provide a high-level justification of why PSKs need to be refcounted
rather than owned now?

[dstockwell, 2015-06-10 00:52:45]

Could you add some class level comments for the new types? It would help in
understanding.

[alancutter (OOO until 2018), 2015-06-11 04:15:13]

On 2015/06/04 at 00:15:16, shans wrote:
> Questions not addressed by this patch:
> --------------------------------------
> * when/where is validateCache called?

It's called in InvalidatableStyleInterpolation::apply().

> * can validateCache ever fail? It seems that it can, because
m_cachedConversion
>   can be empty. But interpolations can't ever fail. What do?

It should never fail, removed cache check in apply().

> * Why is the default fallback partially built into
>   InvalidatableStyleInterpolation via convertSingleKeyframe? Shouldn't the
>   DefaultAnimationType deal with this?

DefaultAnimationType has been renamed to CSSValueAnimationType to be more clear.
A 50% flip can be between any AnimationType as we will still want to composite
correctly with these values in the near future.
The flip interpolation behaviour is represented by FlipInterpolationConversion
and decided upon by InvalidatableStyleInterpolation when all its animationTypes
fail to make a pairwise conversion.

>   
> Naming
> ------
> 
> PairwiseAnimationConversion: this is actually the interpolation primitive now.
> PrimitiveInterpolation? PrimitiveStyleInterpolation?

Done. Used InterpolationPrimitve instead as the *Interpolation namespace has
come to mean a subclass of Interpolation.
It might make sense to eventually merge InvalidatableStyleInterpolation with
InterpolationRecord and rename InterpolationPrimitive as Interpolation, WDYT?

> AnimationConversionHalf: Does this need to have independent existence outside
of
> DefaultAnimationType? Currently it's also used by
InvalidatableStyleInterpolation (but
> here seems to be duplicating what DefaultAnimationType does) and by
maybeConvertSingle in
> AnimationType (but this is only called by InvalidatableStyleInterpolation). If
it
> doesn't, then we can come up with a much simpler name.

Quite correct, it doesn't need to be defined separately. Moved into
FlipInterpolationPrimitive and renamed to Side.
As discussed before DefaultAnimationType was a bad name and has been renamed to
CSSValueAnimationType, it never was intended to represent the 50% flip
behaviour.


On 2015/06/10 at 00:52:45, dstockwell wrote:
> Could you add some class level comments for the new types? It would help in
understanding.

Done.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/AnimationConversion.h (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/AnimationConversion.h:92: struct AnimationConversionHalf :
public NoBaseWillBeGarbageCollectedFinalized<AnimationConversionHalf> {
On 2015/06/04 at 00:15:15, shans wrote:
> Not sure about this name.

Renamed:
AnimationConversion -> InterpolationPrimitive
PairwiseAnimationConversion -> PairwiseInterpolationPrimitive
DefaultAnimationConversion -> FlipInterpolationPrimitive

Moved AnimationConversionHalf to inside FlipInterpolationPrimitive (since its
sole purpose is to make passing data into the constructor cleaner) and renamed
as Side.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/AnimationValue.h:39: void copy(const AnimationValue&
other)
On 2015/06/04 at 00:15:15, shans wrote:
> Should this not just be a copy constructor?

This is used in "result.copy((fraction < 0.5) ? m_start : m_end)" in
FlipInterpolationPrimitive::interpolate() which would use a copy assignment
rather than a copy constructor. I think copy() is more clear in the caller code
than overloaded operator=.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:22:
maybeCachePairwiseConversion(nullptr);
On 2015/06/04 at 00:15:15, shans wrote:
> should be doing the convertSingleKeyframe fallback in validateCache here too?

If pairwise conversion fails here we hold off until we have a StyleResolverState
to try again. We only fall back to flipping between keyframes if we absolutely
can't do a pairwise conversion.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:46:
m_cachedValue.clear();
On 2015/06/04 at 00:15:15, shans wrote:
> Is this right? I thought we always needed a way to interpolate.

interpolate() may be called prior to apply(). Some conversions depend on the
environment provided by apply() and cannot be created beforehand.
Looking at this again it's not necessary to clear the cached value, removed the
else branch.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:54: result->type =
animationType;
On 2015/06/04 at 00:15:15, shans wrote:
> This isn't the right way to do this! result was *just generated* by
animationType - put the type setting code into maybeConvertSingle instead.

Done.

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableValueKeyframe.cpp (right):

https://codereview.chromium.org/1153943003/diff/350001/Source/core/animation/...
Source/core/animation/animatable/AnimatableValueKeyframe.cpp:71:
PassRefPtrWillBeRawPtr<Keyframe::PropertySpecificKeyframe>
AnimatableValueKeyframe::PropertySpecificKeyframe::neutralKeyframe(double
offset, PassRefPtr<TimingFunction> easing) const
On 2015/06/04 at 00:15:15, shans wrote:
> Can you provide a high-level justification of why PSKs need to be refcounted
rather than owned now?

Reconsidering this I think there should be no reason why the lifetime of
PropertySpecificKeyframes (m_keyframeGroups) shouldn't outlive the lifetime of
Interpolations (m_interpolationEffect). Removed this part of the patch and
changed m_start/endKeyframe into references on InvalidatableStyleInterpolation,
this removes a lot of noise from the patch. :)

[alancutter (OOO until 2018), 2015-06-19 00:56:18]

Separated Invalidators from InterpolationPrimitive.
It's possible for a pairwise conversion's decision to return nullptr to be
invalidated by a change in the environment. Example: Length will fail to
pairwise convert "inherit -> 100px" with an inherited value of auto but this
changes when the inherited value becomes 0px.

With Invalidators being separate from InterpolationPrimitives it no longer made
sense to define them together. Renamed Invalidator to ConversionInvalidator and
moved the definition into AnimationType. InvalidatableStyleInterpolation now
stores ConversionInvalidators along side its cached conversion.

PTAL.

[shans, 2015-06-22 11:43:22]

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:18: // A singleton class that provides
functions for:
We already know this is a class which provides functions, no need to tell us
again.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:20: // - Applying interpolation compatible
representations of values to a StyleResolverState.
Reference the functions that do these two things.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:45: }
nit: it's probably better just to result->type = this; in each subclass. Adding
an impl method seems really heavyweight in comparison.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/InterpolationPrimitive.h (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InterpolationPrimitive.h:19: class InterpolationPrimitive
: public NoBaseWillBeGarbageCollectedFinalized<InterpolationPrimitive> {
This isn't a primitive used in interpolation, this is a primitive type of
interpolation. Use PrimitiveInterpolation. Reasoning that all *Interpolations
are so far subclasses of Interpolation is invalid as:
(1) we made that rule up, and not deliberately.
(2) we're setting out to remove all other subclasses of Interpolation.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:43:
m_cachedConversion->interpolate(fraction, m_cachedValue);
comment inline that interpolation is deferred to validateCache if
m_cachedConversion is null.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:76:
m_cachedConversion->interpolate(m_currentFraction, m_cachedValue);
It's slightly weird to see interpolate called inside ensureValidCache.

Maybe ensureInterpolation?

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:77: }
Is this method going to blow out as we have more PrimitiveInterpolation types?

[alancutter (OOO until 2018), 2015-06-23 05:05:33]

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:18: // A singleton class that provides
functions for:
On 2015/06/22 at 11:43:21, shans wrote:
> We already know this is a class which provides functions, no need to tell us
again.

Done.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:20: // - Applying interpolation compatible
representations of values to a StyleResolverState.
On 2015/06/22 at 11:43:21, shans wrote:
> Reference the functions that do these two things.

Done.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/AnimationType.h:45: }
On 2015/06/22 at 11:43:21, shans wrote:
> nit: it's probably better just to result->type = this; in each subclass.
Adding an impl method seems really heavyweight in comparison.

Done.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/InterpolationPrimitive.h (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InterpolationPrimitive.h:19: class InterpolationPrimitive
: public NoBaseWillBeGarbageCollectedFinalized<InterpolationPrimitive> {
On 2015/06/22 at 11:43:21, shans wrote:
> This isn't a primitive used in interpolation, this is a primitive type of
> interpolation. Use PrimitiveInterpolation. Reasoning that all *Interpolations
> are so far subclasses of Interpolation is invalid as:
> (1) we made that rule up, and not deliberately.
> (2) we're setting out to remove all other subclasses of Interpolation.

Done.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:43:
m_cachedConversion->interpolate(fraction, m_cachedValue);
On 2015/06/22 at 11:43:21, shans wrote:
> comment inline that interpolation is deferred to validateCache if
> m_cachedConversion is null.

Done.

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:76:
m_cachedConversion->interpolate(m_currentFraction, m_cachedValue);
On 2015/06/22 at 11:43:21, shans wrote:
> It's slightly weird to see interpolate called inside ensureValidCache.
> 
> Maybe ensureInterpolation?

We may have already interpolated, this also checks whether it's still valid
given the latest style environment. Renamed to ensureValidInterpolation().

https://codereview.chromium.org/1153943003/diff/430001/Source/core/animation/...
Source/core/animation/InvalidatableStyleInterpolation.cpp:77: }
On 2015/06/22 at 11:43:22, shans wrote:
> Is this method going to blow out as we have more PrimitiveInterpolation types?

We will not be adding more PrimitiveInterpolation types, either we interpolate
smoothly or we flip between two AnimationValues.

[dstockwell, 2015-06-23 06:49:46]

lgtm

> covers animation of all non-interpolable properties in element.animate()

But not for display, animation etc. Can we add a test for those?

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/AnimationType.h:26: class ConversionInvalidator : public
NoBaseWillBeGarbageCollectedFinalized<ConversionInvalidator> {
ConversionChecker?

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/AnimationValue.h:39: void copy(const AnimationValue&
other)
copyFrom?

Why is this not a copy-constructor?

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/CSSValueAnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/CSSValueAnimationType.h:12: // A catch all animation type
for CSSValues that never supports pairwise conversion while always supporting
single conversion.
supports -> support

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/PrimitiveInterpolation.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/PrimitiveInterpolation.h:101: if
(!std::isnan(m_lastFraction) && (fraction < 0.5) == (m_lastFraction < 0.5))
comment about the post-oilpan cleanup

[alancutter (OOO until 2018), 2015-06-23 08:11:04]

> > covers animation of all non-interpolable properties in element.animate()
> 
> But not for display, animation etc. Can we add a test for those?

Done.

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/AnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/AnimationType.h:26: class ConversionInvalidator : public
NoBaseWillBeGarbageCollectedFinalized<ConversionInvalidator> {
On 2015/06/23 at 06:49:46, dstockwell wrote:
> ConversionChecker?

Done.

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/AnimationValue.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/AnimationValue.h:39: void copy(const AnimationValue&
other)
On 2015/06/23 at 06:49:46, dstockwell wrote:
> copyFrom?
> 
> Why is this not a copy-constructor?

Changed to copyFrom(). If this were a copy-constructor I'd have to manually
destruct and placement new the object in
FlipPrimitiveInterpolation::interpolate() to get the same effect.

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/CSSValueAnimationType.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/CSSValueAnimationType.h:12: // A catch all animation type
for CSSValues that never supports pairwise conversion while always supporting
single conversion.
On 2015/06/23 at 06:49:46, dstockwell wrote:
> supports -> support

Rewrote text.

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
File Source/core/animation/PrimitiveInterpolation.h (right):

https://codereview.chromium.org/1153943003/diff/450001/Source/core/animation/...
Source/core/animation/PrimitiveInterpolation.h:101: if
(!std::isnan(m_lastFraction) && (fraction < 0.5) == (m_lastFraction < 0.5))
On 2015/06/23 at 06:49:46, dstockwell wrote:
> comment about the post-oilpan cleanup

Done.

[shans, 2015-06-24 05:05:07]

lgtm

[alancutter (OOO until 2018), 2015-06-24 05:06:59]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2015-06-24 05:07:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org, dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1153943003/#ps470001
(title: "Review changes")

[commit-bot: I haz the power, 2015-06-24 05:07:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153943003/470001

[shans, 2015-06-24 05:08:10]

lgtm

lgtm

[commit-bot: I haz the power, 2015-06-24 06:15:35]

Committed patchset #25 (id:470001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197705