New issue
Advanced search Search tips

Issue 725385 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
smcgruer@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Task

Blocked on:
issue 442163
Blocking:
issue 624686
issue 654309



Sign in to add a comment

Delete the AnimatableValue class heirarchy

Project Member Reported by alancutter@chromium.org, May 23 2017 
AnimatableValues are a legacy implementation of Blink's animation engine.
We have since moved onto using Interpolations and InterpolationTypes to support composite modes and keyframes that are responsive to their underlying values.

AnimatableValues still exist in the codebase however. They are still used for the following things:
 - CSS Transition reversal detection. See CSSAnimations::NewTransition::reversing_adjusted_start_value.
 - Compositor keyframe value construction. See AddKeyframeToCurve() in CompositorAnimations.cpp.
 - Animations unit tests e.g. KeyframeEffectModelTest.cpp and EffectStackTest.cpp.
 - Experimental CompositorWorklet code. See CustomCompositorAnimations.cpp.

Comment 1 by alancutter@chromium.org, May 23 2017

Blocking: 624686 654309

Comment 2 by alancutter@chromium.org, May 23 2017

Blockedon: 442163

Comment 3 by alancutter@chromium.org, May 24 2017 

Design doc for removing uses of AnimatableValues for reversing CSS Transitions:
https://docs.google.com/document/d/1cvuLElVHaeM9YvqxoedcdilPD9MVD4EtfNfl9R_rKbk/edit
Project Member

Comment 4 by bugdroid1@chromium.org, May 25 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bdf1977ad4c41816296f03ddfe9429184b8fd34

commit 2bdf1977ad4c41816296f03ddfe9429184b8fd34
Author: alancutter <alancutter@chromium.org>
Date: Thu May 25 05:13:26 2017

Replace AnimatableValues with ComputedStyle references for CSS Transitions

This change replaces AnimatableValues with ComputedStyle references
in our CSS Transition data.
This data is used for detecting and processing reversed transitions,
see spec for further detail:
https://drafts.csswg.org/css-transitions/#transition-reversing-adjusted-start-value

By replacing AnimatableValues we remove the dependency on the vast majority
of AnimatableValue types. This will allow us to remove large portions
of the class hierarchy.

Design doc:
https://docs.google.com/document/d/1cvuLElVHaeM9YvqxoedcdilPD9MVD4EtfNfl9R_rKbk/edit

BUG=725385

Review-Url: https://codereview.chromium.org/2895283004
Cr-Commit-Position: refs/heads/master@{#474559}

[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/animation/css/CSSAnimationUpdate.h
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/animation/css/CSSAnimations.h
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/css/CSSPropertyEquality.cpp
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/css/CSSPropertyEquality.h
[modify] https://crrev.com/2bdf1977ad4c41816296f03ddfe9429184b8fd34/third_party/WebKit/Source/core/style/ComputedStyle.cpp
Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0793c35f0db302a2a619f69670149b60a568e55

commit a0793c35f0db302a2a619f69670149b60a568e55
Author: alancutter <alancutter@chromium.org>
Date: Thu May 25 07:06:10 2017

Remove unused AnimatableValue types

After replacing AnimatableValues with ComputedStyle references in
https://codereview.chromium.org/2895283004 we no longer need
AnimatableValues outside of composited properties.

This patch removes all the unused AnimatableValue code.

BUG=725385

Review-Url: https://codereview.chromium.org/2893313004
Cr-Commit-Position: refs/heads/master@{#474595}

[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/BUILD.gn
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/EffectStackTest.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableClipPathOperation.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableClipPathOperation.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableColor.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableColor.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableDouble.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableDouble.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableDoubleAndBool.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableDoubleAndBool.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableFilterOperations.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableFilterOperations.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableFontVariationSettings.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableFontVariationSettings.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableImage.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableImage.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLength.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLength.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthBox.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthBox.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthBoxAndBool.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthBoxAndBool.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthPoint.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthPoint.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthPoint3D.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthPoint3D.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthSize.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableLengthSize.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatablePath.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatablePath.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableRepeatable.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableRepeatable.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableSVGPaint.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableSVGPaint.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableShadow.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableShadow.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableShapeValue.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableShapeValue.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableStrokeDasharrayList.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableTransform.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableTransform.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableUnknown.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/animatable/AnimatableValue.h
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableVisibility.cpp
[delete] https://crrev.com/345a41ad87d60ad3214422adb379a373fd0abed0/third_party/WebKit/Source/core/animation/animatable/AnimatableVisibility.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.h
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp
[modify] https://crrev.com/a0793c35f0db302a2a619f69670149b60a568e55/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp

Comment 6 by smcgruer@chromium.org, Oct 13 2017

Owner: smcgruer@chromium.org
Status: Started (was: Available)

Comment 7 by smcgruer@chromium.org, Oct 18 2017

Cc: -alancutter@chromium.org
Forgot to tag the CL, but https://chromium.googlesource.com/chromium/src/+/c3a93c7dee7978c37d749042e9ccd6ec621577c1 just landed which removes AnimatableValueKeyframe:

Remove AnimatableValueKeyframe

blink::AnimatableValueKeyframe is no longer used outside of tests. This
CL converts the remaining tests that use it to use StringKeyframe instead
and deletes AnimatableValueKeyframe.

Using a StringKeyframe rather than an AnimatableValueKeyframe does cause
some code churn, most notably:
  * The underlying interpolation becomes an InvalidatableInterpolation rather
    than a LegacyStyleInterpolation. This requires us to 'play nice' in
    certain places to make sure the interpolation result has been cached.
  * AnimatableValueKeyframe does no CSS 'type checking', which allowed the
    tests to abuse types (ignoring units, using invalid values for the property,
    or using AnimatableUnknown with interpolable CSS properties to make them
    noninterpolable). StringKeyframe does do CSS type checking, so all that code
    had to be fixed.

Bug: None
Change-Id: Ief3772390aaa72135fff23ccdfb85bc622bdb5cd
Reviewed-on: https://chromium-review.googlesource.com/717574
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509675}
third_party/WebKit/Source/core/animation/AnimationTestHelper.cpp[diff]
third_party/WebKit/Source/core/animation/AnimationTestHelper.h[diff]
third_party/WebKit/Source/core/animation/BUILD.gn[diff]
third_party/WebKit/Source/core/animation/CSSDefaultInterpolationType.cpp[diff]
third_party/WebKit/Source/core/animation/CSSDefaultInterpolationType.h[diff]
third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.h[diff]
third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp[diff]
third_party/WebKit/Source/core/animation/DocumentTimelineTest.cpp[diff]
third_party/WebKit/Source/core/animation/EffectStackTest.cpp[diff]
third_party/WebKit/Source/core/animation/InvalidatableInterpolation.h[diff]
third_party/WebKit/Source/core/animation/Keyframe.h[diff]
third_party/WebKit/Source/core/animation/KeyframeEffectModel.h[diff]
third_party/WebKit/Source/core/animation/KeyframeEffectModelTest.cpp[diff]
third_party/WebKit/Source/core/animation/LegacyStyleInterpolation.h[diff]
third_party/WebKit/Source/core/animation/animatable/AnimatableValueKeyframe.cpp[Deleted - diff]
third_party/WebKit/Source/core/animation/animatable/AnimatableValueKeyframe.h[Deleted - diff]
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp[diff]
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5324d4fb91b01fb36dee22be694473bd06556c92

commit 5324d4fb91b01fb36dee22be694473bd06556c92
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Oct 18 21:02:18 2017

Remove LegacyStyleInterpolation

This class was only instantiated in tests now, and can be replaced in
those tests with TransitionInterpolation. 

Bug: 725385

Change-Id: I11ae5099bdb8a9596d09c06b75842d142efae37b
Reviewed-on: https://chromium-review.googlesource.com/723660
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509872}
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/AnimationTestHelper.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/BUILD.gn
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/CSSInterpolationType.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/CSSNumberInterpolationType.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/EffectStackTest.cpp
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/InterpolableValueTest.cpp
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/Interpolation.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/InterpolationEffectTest.cpp
[delete] https://crrev.com/0c4fbe1c51a85b136fa74e1068fbc9ab7bb0f264/third_party/WebKit/Source/core/animation/LegacyStyleInterpolation.cpp
[delete] https://crrev.com/0c4fbe1c51a85b136fa74e1068fbc9ab7bb0f264/third_party/WebKit/Source/core/animation/LegacyStyleInterpolation.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/TransitionInterpolation.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/animation/UnderlyingValueOwner.h
[modify] https://crrev.com/5324d4fb91b01fb36dee22be694473bd06556c92/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff5b6944f36e0e4fcc62d8c65ce07b2b17809291

commit ff5b6944f36e0e4fcc62d8c65ce07b2b17809291
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Thu Oct 19 01:08:32 2017

Remove unused class InterpolableAnimatableValue

Bug: 725385
Change-Id: I9937c4b13e64a439d798dc5450cfa5b6242307bf
Reviewed-on: https://chromium-review.googlesource.com/723789
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509956}
[modify] https://crrev.com/ff5b6944f36e0e4fcc62d8c65ce07b2b17809291/third_party/WebKit/Source/core/animation/InterpolableValue.cpp
[modify] https://crrev.com/ff5b6944f36e0e4fcc62d8c65ce07b2b17809291/third_party/WebKit/Source/core/animation/InterpolableValue.h

Comment 10 by alancutter@chromium.org, Oct 19 2017 

:D :D :D :D
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a4710d15c45319134e68f75b476fbe98b578810

commit 0a4710d15c45319134e68f75b476fbe98b578810
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Feb 27 21:48:41 2018

Remove blink::AnimatableUnknown

This class was only referenced in a single test, where it was used in a
function that was never called. Dead code be gone!

Bug: 725385
Change-Id: I93d2261cb387a542e2dd9b277e19526f77b7961a
Reviewed-on: https://chromium-review.googlesource.com/939601
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539554}
[modify] https://crrev.com/0a4710d15c45319134e68f75b476fbe98b578810/third_party/WebKit/Source/core/animation/BUILD.gn
[modify] https://crrev.com/0a4710d15c45319134e68f75b476fbe98b578810/third_party/WebKit/Source/core/animation/KeyframeEffectModelTest.cpp
[delete] https://crrev.com/86836ab20a6006edcb0c4b5fbb7f3d8e503193ed/third_party/WebKit/Source/core/animation/animatable/AnimatableUnknown.h

Comment 12 by smcgruer@chromium.org, Mar 26 2018

Status: Assigned (was: Started)

Comment 13 by smcgruer@chromium.org, Apr 25 2018

Cc: smcgruer@chromium.org
Labels: -Type-Bug Type-Task
Owner: ----
Status: Available (was: Assigned)
►

Sign in to add a comment