Implemented additive animations for length

Implemented additive animations for length

Interpolations now store start and end composite operations and an
underlying fraction. To support addition, StyleInterpolation subclasses
should implement a constructor that sets the composite modes, and
override the new apply method which takes an InterpolableValue to
convert and apply as an argument.

AnimationStack::copyToActiveInterpolationMap now assembles an
InterpolationPipeline for each CSS property, which is a linked list of
InterpolationPipelineStages, each containing an Interpolation.

In StyleResolver::applyAnimatedProperties, each stage of the additive
pipeline is evaluated, their results added together, and the result
applied to the LayoutStyle.

BUG=437696

[caley, 2015-02-11 09:24:39]

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

[alancutter (OOO until 2018), 2015-02-12 05:55:45]

https://codereview.chromium.org/863863004/diff/100001/LayoutTests/web-animati...
File LayoutTests/web-animations-api/animations-additive-width.html (right):

https://codereview.chromium.org/863863004/diff/100001/LayoutTests/web-animati...
LayoutTests/web-animations-api/animations-additive-width.html:256: */
Uncomment and expect failure.

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
File Source/core/animation/Interpolation.cpp (right):

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/Interpolation.cpp:72: }
Shouldn't this just be "if start == add: UF = 1"?

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/Interpolation.cpp:87: m_cachedUnderlyingFraction =
blend(1.0, 0.0, fraction);
Just use 1 - fraction.

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/Interpolation.cpp:89: m_cachedUnderlyingFraction =
blend(0.0, 1.0, fraction);
Just use fraction.

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
File Source/core/animation/Interpolation.h (right):

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/Interpolation.h:34: bool isReplaceOnly() const { return
m_compositeStart == AnimationEffect::CompositeReplace && m_compositeEnd ==
AnimationEffect::CompositeReplace; }
We want to just check for UF == 0. Let's not have this function as the name may
be confusing and just check UF directly.

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
File Source/core/animation/InterpolationPipelineStage.h (right):

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/InterpolationPipelineStage.h:31: void
setNext(RefPtrWillBeRawPtr<InterpolationPipelineStage> next)
This should be a PassRefPtr.

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/InterpolationPipelineStage.h:38: }
Why not just set m_next = next?

https://codereview.chromium.org/863863004/diff/100001/Source/core/animation/I...
Source/core/animation/InterpolationPipelineStage.h:49: }
This is unneeded.

[alancutter (OOO until 2018), 2015-02-17 03:50:56]

Thanks for the extra dedication to your project George! Just to be clear there's
no requirement for you to complete this patch. If it's motivated by self
satisfaction for getting the job done that's alright. Please just let us know if
you do decide to stop working on it.

https://codereview.chromium.org/863863004/diff/160001/Source/core/animation/A...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/863863004/diff/160001/Source/core/animation/A...
Source/core/animation/AnimationStack.cpp:57: if (!pipeline.isEmpty() &&
interpolation->isReplaceOnly())
This should be checking for cachedUnderlyingFraction == 0 to match the logic in
applyAnimatedProperties().

https://codereview.chromium.org/863863004/diff/160001/Source/core/animation/I...
File Source/core/animation/Interpolation.h (right):

https://codereview.chromium.org/863863004/diff/160001/Source/core/animation/I...
Source/core/animation/Interpolation.h:69: using InterpolationPipelineMap =
WillBeHeapHashMap<CSSPropertyID, InterpolationPipeline>;
I'm a bit iffy about having InterpolationPipelineMap defined in Interpolation.h
but I'm not aware of a better place to put it other than its own file.

https://codereview.chromium.org/863863004/diff/160001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/863863004/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1098: }
Instead of duplicating the loop here just set startingIndex = 0 in this body and
startingIndex = 1 in the else body.

https://codereview.chromium.org/863863004/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1103: multipliedValue =
underlyingInterpolation->cachedValue().clone();
We must try to avoid the double IV clone in the common case of a single replace
Interpolation.
Here's my attempt to clone only when necessary, feel free to try making it
nicer:
    // FIXME: Remove these clone OwnPtrs once the Oilpan conversion is complete.
    OwnPtrWillBeRawPtr<InterpolableValue> underlyingValueClone;
    OwnPtrWillBeRawPtr<InterpolableValue> multipliedValueClone;
    InterpolableValue* underlyingValue =
&underlyingInterpolation->cachedValue();
    InterpolableValue* multipliedValue = nullptr;
    if (startIndex < pipeline.size()) {
        underlyingValueClone = underlyingInterpolation->cachedValue().clone();
        underlyingValue = underlyingValueClone.get();
        multipliedValueClone = underlyingInterpolation->cachedValue().clone();
        multipliedValue = multipliedValueClone.get();
    }

[alancutter (OOO until 2018), 2015-02-17 04:08:37]

https://codereview.chromium.org/863863004/diff/160001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/863863004/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1103: multipliedValue =
underlyingInterpolation->cachedValue().clone();
On 2015/02/17 03:50:56, alancutter wrote:
> We must try to avoid the double IV clone in the common case of a single
replace
> Interpolation.
> Here's my attempt to clone only when necessary, feel free to try making it
> nicer:
>     // FIXME: Remove these clone OwnPtrs once the Oilpan conversion is
complete.
>     OwnPtrWillBeRawPtr<InterpolableValue> underlyingValueClone;
>     OwnPtrWillBeRawPtr<InterpolableValue> multipliedValueClone;
>     InterpolableValue* underlyingValue =
> &underlyingInterpolation->cachedValue();
>     InterpolableValue* multipliedValue = nullptr;
>     if (startIndex < pipeline.size()) {
>         underlyingValueClone = underlyingInterpolation->cachedValue().clone();
>         underlyingValue = underlyingValueClone.get();
>         multipliedValueClone = underlyingInterpolation->cachedValue().clone();
>         multipliedValue = multipliedValueClone.get();
>     }

Actually scratch that. The simpler way to do it is just
    if (startingIndex >= pipeline.size()) {
        toStyleInterpolation(*underlyingInterpolation).apply(state,
underlyingInterpolation->cachedValue());
        continue;
    }
prior to cloning.

[alancutter (OOO until 2018), 2015-02-17 05:03:04]

After discussions with Shane and Doug there's a number of refactors to this
approach that we want to make. Namely removing InterpolationFactory and instead
creating the InterpolableValue from the underlying CSS value directly via the
first Interpolation in the apply list and moving all the additive logic out of
the StyleResolver.

It's probably best if we take over this patch now given the complexity of these
changes. Thanks a ton for working on this in your own free time nonetheless!

[caley, 2015-02-17 06:07:13]

On 2015/02/17 05:03:04, alancutter wrote:
> After discussions with Shane and Doug there's a number of refactors to this
> approach that we want to make. Namely removing InterpolationFactory and
instead
> creating the InterpolableValue from the underlying CSS value directly via the
> first Interpolation in the apply list and moving all the additive logic out of
> the StyleResolver.
> 
> It's probably best if we take over this patch now given the complexity of
these
> changes. Thanks a ton for working on this in your own free time nonetheless!

Alright, no problem!