third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h - Issue 2019613002: Blink Compositor Animation: Make Animation and Curve methods non-virtual.

Unified Diff: third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h

Issue 2019613002: Blink Compositor Animation: Make Animation and Curve methods non-virtual. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h

diff --git a/third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h b/third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h

new file mode 100644

index 0000000000000000000000000000000000000000..c2d549bd13c3580738bbede9d475a539c9d06264

--- /dev/null

+++ b/third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h

@@ -0,0 +1,21 @@

+// Use of this source code is governed by a BSD-style license that can be

+// found in the LICENSE file.

+#ifndef CompositorAnimationsTestUtils_h

+#define CompositorAnimationsTestUtils_h

+#include "cc/animation/animation.h"

+#include "cc/animation/animation_curve.h"

+#include "cc/animation/keyframed_animation_curve.h"

+#include "cc/animation/timing_function.h"

+using TimeDeltaForTesting = base::TimeDelta;

jbroman 2016/05/27 14:52:55 This doesn't seem super helpful and makes the Sour

This doesn't seem super helpful and makes the Source/platform/ abstraction super leaky, even if only for testing. I don't know that referring to "CcAnimationForTesting" is any better than "cc::Animation" (except it's harder to read and adds a weird indirection that doesn't actually hide the interface). I'd much rather you provide actual testing utilities in platform; ideally ones that operated on a blink::CompositorAnimation (etc) rather than cc::Animation. For example: std::unique_ptr<CcAnimationForTesting> animation = convertToCompositorAnimation(*effect); EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->target_property()); // ... could instead be written as OwnPtr<CompositorAnimation> animation = convertToCompositorAnimation(*effect); EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->targetProperty()); // ... const CcKeyframedFloatAnimationCurveForTesting* keyframedFloatCurve = ToFloatCurve(animation.get()); DCHECK_EQ(2UL, keyframedFloatCurve->keyframes_for_testing().size()); const CcFloatKeyframeForTesting* keyframe1 = keyframedFloatCurve->keyframes_for_testing()[0].get(); const CcFloatKeyframeForTesting* keyframe2 = keyframedFloatCurve->keyframes_for_testing()[1].get(); EXPECT_EQ(TimeDeltaForTesting(), keyframe1->Time()); EXPECT_EQ(2.0f, keyframe1->Value()); could instead be written as // first two lines could be grouped into a floatCurveKeyframesForAnimation(const CompositorAnimation&). OwnPtr<CompositorFloatAnimationCurve> floatAnimationCurve = animation->floatCurveForTesting(); Vector<CompositorFloatKeyframe> keyframes = floatAnimationCurve->keyframesForTesting(); ASSERT_EQ(2UL, keyframes.size()); EXPECT_EQ(0.0, keyframes[0].time); EXPECT_EQ(2.0f, keyframes[0].value); or perhaps, if you prefer, you could move some of the checks into helpers using functions (returning bools or testing::AssertionResult) or gmock matchers If you're worried about whether these accessors faithfully represent the underlying cc::Animation, that should be tested in platform. Alternatively, if some of the logic of that test doesn't require core, perhaps it can be moved to blink_platform_unittests, where accessing underlying types is appropriate. If we do want to use base/cc types in core for testing, I'd rather see a discussion about allowing that than a ton of type aliases.

loyso (OOO) 2016/05/31 02:20:39 Done.

On 2016/05/27 14:52:55, jbroman wrote: > This doesn't seem super helpful and makes the Source/platform/ abstraction super > leaky, even if only for testing. I don't know that referring to > "CcAnimationForTesting" is any better than "cc::Animation" (except it's harder > to read and adds a weird indirection that doesn't actually hide the interface). > > I'd much rather you provide actual testing utilities in platform; ideally ones > that operated on a blink::CompositorAnimation (etc) rather than cc::Animation. > For example: > > > std::unique_ptr<CcAnimationForTesting> animation = > convertToCompositorAnimation(*effect); > EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->target_property()); > // ... > > could instead be written as > > OwnPtr<CompositorAnimation> animation = convertToCompositorAnimation(*effect); > EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->targetProperty()); > // ... > > > const CcKeyframedFloatAnimationCurveForTesting* keyframedFloatCurve = > ToFloatCurve(animation.get()); > DCHECK_EQ(2UL, keyframedFloatCurve->keyframes_for_testing().size()); > const CcFloatKeyframeForTesting* keyframe1 = > keyframedFloatCurve->keyframes_for_testing()[0].get(); > const CcFloatKeyframeForTesting* keyframe2 = > keyframedFloatCurve->keyframes_for_testing()[1].get(); > EXPECT_EQ(TimeDeltaForTesting(), keyframe1->Time()); > EXPECT_EQ(2.0f, keyframe1->Value()); > > could instead be written as > > // first two lines could be grouped into a floatCurveKeyframesForAnimation(const > CompositorAnimation&). > OwnPtr<CompositorFloatAnimationCurve> floatAnimationCurve = > animation->floatCurveForTesting(); > Vector<CompositorFloatKeyframe> keyframes = > floatAnimationCurve->keyframesForTesting(); > ASSERT_EQ(2UL, keyframes.size()); > EXPECT_EQ(0.0, keyframes[0].time); > EXPECT_EQ(2.0f, keyframes[0].value); > > or perhaps, if you prefer, you could move some of the checks into helpers using > functions (returning bools or testing::AssertionResult) or gmock matchers > > > If you're worried about whether these accessors faithfully represent the > underlying cc::Animation, that should be tested in platform. > > Alternatively, if some of the logic of that test doesn't require core, perhaps > it can be moved to blink_platform_unittests, where accessing underlying types is > appropriate. > > If we do want to use base/cc types in core for testing, I'd rather see a > discussion about allowing that than a ton of type aliases.

Done.

+using CcAnimationForTesting = cc::Animation;

+using CcTimingFunctionForTesting = cc::TimingFunction;

+using CcKeyframedFloatAnimationCurveForTesting = cc::KeyframedFloatAnimationCurve;

+using CcCubicBezierTimingFunctionForTesting = cc::CubicBezierTimingFunction;

+using CcFloatKeyframeForTesting = cc::FloatKeyframe;

+using CcAnimationCurveForTesting = cc::AnimationCurve;

+#endif // CompositorAnimationsTestUtils_h

alancutter (OOO until 2018) 2016/06/01 02:20:55 This is only included once, why have a separate fi

loyso (OOO) 2016/06/01 02:36:47 Irrelevant with a new patch set.

loyso (OOO) 2016/06/01 02:38:20 The actual reason was the folder, where is file is

« third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp ('K') | « third_party/WebKit/Source/platform/blink_platform.gypi ('k') | no next file » | no next file with comments »